Skip to content
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

Closed
ankitspd opened this issue Dec 16, 2016 · 26 comments
Closed

[SR-3438] Preprocessor directive not working when shadowing parameters #46026

ankitspd opened this issue Dec 16, 2016 · 26 comments
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself

Comments

@ankitspd
Copy link
Member

Previous ID SR-3438
Radar rdar://problem/29707925
Original Reporter @aciidb0mb3r
Type Bug
Status Resolved
Resolution Done
Environment

swift-DEVELOPMENT-SNAPSHOT-2016-12-15-a

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug, 3.1Regression
Assignee @CodaFi
Priority Medium

md5: 1e922f2f1a9024762bd19cad6c87e673

relates to:

  • SR-1560 Implement support for SE-0075: Adding a Build Configuration Import Test

Issue Description:

This started happening with December 15, 2016 trunk toolchain (was working fine before that)

func hello(_ arr: [String]) {
  #if os(macOS)
    let arr = arr.filter { _ in false }
  #endif
    print(arr)
}
hello(["hi"])

This should print empty array on macOS but it prints ["hi"] with following warning:

warning: initialization of immutable value 'arr' was never used; consider replacing with assignment to '_' or removing it
    let arr = arr.filter { _ in false }
    ~~~~^~~
    _
@belkadan
Copy link
Contributor

CodaFi (JIRA User), fallout from your changes?

@belkadan
Copy link
Contributor

@swift-ci create

@CodaFi
Copy link
Member

CodaFi commented Dec 16, 2016

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.

@CodaFi
Copy link
Member

CodaFi commented Dec 16, 2016

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.

@belkadan
Copy link
Contributor

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

@CodaFi
Copy link
Member

CodaFi commented Dec 16, 2016

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.

@DougGregor
Copy link
Member

ASTScope lookup is disabled and still very broken. That's the right long-term answer, but it's not an answer to this regression.

@belkadan
Copy link
Contributor

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.

@DougGregor
Copy link
Member

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.

@CodaFi
Copy link
Member

CodaFi commented Dec 17, 2016

Started a patch here for this by teaching the condition resolver about scopes. It feels incredibly dirty, but it works.

@belkadan
Copy link
Contributor

Let's time-box this: if we don't figure out an answer by Branch Day (1/16), we should revert canImport out of Swift 3.1. (That's still a good amount of time, even with most Apple folks gone between Christmas and New Year's Day.)

@CodaFi
Copy link
Member

CodaFi commented Dec 20, 2016

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.

@belkadan
Copy link
Contributor

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:

1. Currently, all #if directives are resolved while parsing code, while import decls are resolved right at the start of "type-checking" (semantic analysis) in a step called "name-binding". (The name is a bit outdated, but the function is still called performNameBinding IIRC.
2. Parsing must not resolve imports, because we use parsing to do simple and fast syntax highlighting that should be useful even in the presence of source errors.

@akyrtzi, @benlangmuir, maxs (JIRA User), what do you think the actual performance constraints on parsing are?

@benlangmuir
Copy link
Member

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.

@akyrtzi
Copy link
Member

akyrtzi commented Dec 20, 2016

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.

@rintaro
Copy link
Mannequin

rintaro mannequin commented Dec 21, 2016

Just FYI,

Quick testing with diff: master...rintaro:lab-no-declref-in-parse
It seems NameLookup in Sema has these problems, at least:

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
}

@CodaFi
Copy link
Member

CodaFi commented Dec 23, 2016

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.

@slavapestov
Copy link
Member

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 🙂

@CodaFi
Copy link
Member

CodaFi commented Jan 3, 2017

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.

@benlangmuir
Copy link
Member

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

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?

And this approach still won't ever pass the IDE tests without Parse evaluating conditions.

Can you clarify what problems you're seeing?

@belkadan
Copy link
Contributor

belkadan commented Jan 3, 2017

#if does change the structure of decls in the AST—decls/statements in the active branch are splatted into the enclosing context, while decls in disabled branches are kept separate.

@DougGregor
Copy link
Member

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

@belkadan
Copy link
Contributor

belkadan commented Jan 3, 2017

That's also fair, but ruins things like getMembers.

@CodaFi
Copy link
Member

CodaFi commented Jan 7, 2017

@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?

@DougGregor
Copy link
Member

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.

@CodaFi
Copy link
Member

CodaFi commented Aug 31, 2017

Resolved by the removal of the condition resolution phase in the revert here.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself
Projects
None yet
Development

No branches or pull requests

7 participants