New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SR-3438] Preprocessor directive not working when shadowing parameters #46026
Comments
CodaFi (JIRA User), fallout from your changes? |
@swift-ci create |
I've dumped a diff between the two ASTs and the interesting thing is they're identical (mod IfConfigStmt) except for a lone DeclRefExpr for 'arr' here pointing to the wrong place. |
I think the fix here is to always perform ASTScope lookup because Parse can no longer assume that it can see everything when forming DeclRefExprs. |
Aack. That makes sense but may also be a performance hit, and I'm not sure we actually ironed out all the correctness fixes. cc @DougGregor |
Another would be to attempt to rebuild the scope map as I traverse the AST in Condition Resolution and rebind DeclRefExprs there with an expression walker. |
ASTScope lookup is disabled and still very broken. That's the right long-term answer, but it's not an answer to this regression. |
In theory Sema (really the old NameLookup code) knows how to resolve DREs too if the parser decides to leave them unbound. In practice I remember there being some issues, but not what they were. |
The problem is that the really old name lookup code is itself quite buggy, and turning it on more often causes a different set of regressions. |
Started a patch here for this by teaching the condition resolver about scopes. It feels incredibly dirty, but it works. |
Let's time-box this: if we don't figure out an answer by Branch Day (1/16), we should revert |
Here's a thought: Given that we're no longer loading modules as of #6279, is there any reason to keep Condition Resolution as a pass - the performance impact is no longer as significant? I can remove it and StmtTransformer and keep the simplifications to condition validation and evaluation and that would kill two birds with one stone. |
Hm. It still has to crawl around on disk and read in module maps to figure out if things are importable. But we can keep it in mind. In SR-1560 I wrote:
@akyrtzi, @benlangmuir, maxs (JIRA User), what do you think the actual performance constraints on parsing are? |
I'm not sure I understand the whole context here, but responding to Jordan's comments: Parsing has to be really fast, since we use it to get syntax highlighting. Aside from the performance, I also don't like the idea of syntax-only parsing depending on the state of the filesystem and import paths. That would force SourceKit clients to give us accurate compiler arguments for things that they don't today. |
To add to what Ben said, parsing is not only used for syntax highlighting, it's also used to get the syntactic structure of document and for indentation. It is a very hot path and it's critical to be as fast as possible. |
Just FYI, Quick testing with diff: master...rintaro:lab-no-declref-in-parse func pat1(x: Int) -> Int {
var x = x // error: 'x' used within its own type
// error: could not infer type for 'x'
x += 1
return x
}
func pat2(x: Int) -> Int {
let y = x // error: use of local variable 'x' before its declaration
let x = 1 // note: 'x' declared here
return y + x
}
func pat3(condition: Bool) -> Int {
if condition {
let x = 1
return x // error: use of local variable 'x' before its declaration
}
let x = 2 // note: 'x' declared here
return x
}
func pat4(condition: Bool) -> Int {
let x = 1 // note: found this candidate
if condition {
let x = 2 // note: found this candidate
return x // error: ambiguous use of 'x'
}
return x
} |
If Parse is used to grab the syntactic structure of a buffer, then don't we have to eat this cost there? Otherwise there won't be an accurate picture of declarations introduced into a scope that contains if-configuration statements. |
Code completion uses NameLookup, and Parse duplicating the lookup logic is a terrible hack, so fixing these inconsistencies would be great regardless. If you really want to go for gold though, just finish off ASTScope 🙂 |
Even if ASTScope were up and running it would still need to invoke condition resolution to actually see which parts of the configuration dump identifiers into scope. I really think the way this is going to have to work is by incurring the cost of hitting the disk to check for the existence of modules. I've spent a few weeks exploring the alternative: walking the entire AST and rebinding any decl refs manually. It seems to be a massive complication on top of an already O(n^2) process. And this approach still won't ever pass the IDE tests without Parse evaluating conditions. If hitting the disk is the issue we can insert caching layers to catch duplicate unimported modules to keep the number of llvm::sys::fs::exists calls O(m) where m is the number of unique unimported module names. That should both remove an enormous amount of complexity and keep Parse nimble enough. |
I don't think this should be an issue for syntax-only parsing. We don't expect to be able to resolve names, and since you can't use #if to change the brace structure should it be fine?
Can you clarify what problems you're seeing? |
|
That `#if` changes the structure of the AST is, IMO, an architectural mistake. We should represent the source and then dynamically decide which path to walk |
That's also fair, but ruins things like |
@rintaro It looks like those diagnostics are due to the fallback path in Parse being incorrect. Is there a reason Parse still diagnoses variables bindings that reference themselves when Sema does the same? |
It's a bit of a mess. Parse should not be emitting those diagnostics-~~~~should not be doing any name lookup or semantic analysis, really~~~~-but the way in which Sema does the check for referencing a variable too early ("does it have a type already?") is broken. The new `ASTScope` ("scope map")-based unqualified lookup was meant to address this, but isn't ready to be turned on yet. |
Resolved by the removal of the condition resolution phase in the revert here. |
Environment
swift-DEVELOPMENT-SNAPSHOT-2016-12-15-a
Additional Detail from JIRA
md5: 1e922f2f1a9024762bd19cad6c87e673
relates to:
Issue Description:
This started happening with December 15, 2016 trunk toolchain (was working fine before that)
This should print empty array on macOS but it prints ["hi"] with following warning:
The text was updated successfully, but these errors were encountered: