Remove ASTScope starting DeclContext hack #33976
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…dedGenericParam fix We want to look for protocols at the top level, and not names visible from the function body. Previously we were passing the function's parent DeclContext and the source location of the parameter, but this will no longer work, so pass the SourceFile and an empty source location instead.
The parameters are visible inside the source range that begins with the 'in' keyword and ends with the closure's closing brace.
…Lookup() if necessary UnqualifiedLookupRequest takes a DeclContext and a SourceLoc. If the SourceLoc is valid, it locates the innermost ASTScope containing this location, and starts the lookup from that scope. Also, ASTScope currently walks up the scope tree to find the innermost scope that corresponds to the given DeclContext, in case the DeclContext is a parent of the 'natural' DeclContext for this source location. We want to remove this additional behavior and make ASTScope only depend on a source file and source location. This requires changing directReferencesForUnqualifiedTypeLookup() to handle the top-level lookup case explicitly. See the comment for more details.
This was used to validate against the old lookup code, but should no longer be needed.
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please test source compatibility Release |
davidungar
approved these changes
Sep 17, 2020
| @@ -841,39 +729,6 @@ Optional<bool> PatternEntryInitializerScope::resolveIsCascadingUseForThisScope( | |||
| return isCascadingUse; | |||
| } | |||
|
|
|||
| bool isLocWithinAnInactiveClause(const SourceLoc loc, SourceFile *SF) { | |||
There was a problem hiding this comment.
I'm a bit surprised you can do away with this.
Comment on lines
+1949
to
+1973
| // In a protocol or protocol extension, the 'where' clause can refer to | ||
| // associated types without 'Self' qualification: | ||
| // | ||
| // protocol MyProto where AssocType : Q { ... } | ||
| // | ||
| // extension MyProto where AssocType == Int { ... } | ||
| // | ||
| // For this reason, ASTScope maps source locations inside the 'where' | ||
| // clause to a scope that performs the lookup into the protocol or | ||
| // protocol extension. | ||
| // | ||
| // However, protocol and protocol extensions can also put bounds on 'Self', | ||
| // for example: | ||
| // | ||
| // protocol MyProto where Self : MyClass { ... } | ||
| // | ||
| // We must start searching for 'MyClass' at the top level, otherwise | ||
| // we end up with a cycle, because qualified lookup wants to resolve | ||
| // 'Self' bounds to build the set of declarations to search inside of. | ||
| // | ||
| // To make this work, we handle the top-level lookup case explicitly | ||
| // here, bypassing unqualified lookup and ASTScope altogether. | ||
| if (dc->isModuleScopeContext()) | ||
| loc = SourceLoc(); | ||
|
|
| @@ -1946,7 +1946,33 @@ static DirectlyReferencedTypeDecls | |||
| directReferencesForUnqualifiedTypeLookup(DeclNameRef name, | |||
| SourceLoc loc, DeclContext *dc, | |||
There was a problem hiding this comment.
Seems a bit odd that we still pass in a DC here, esp. since passing in both a location and a DC.
There was a problem hiding this comment.
I'm going to do a separate PR that redoes the user-facing unqualified lookup entry point to take a SourceFile instead of a DC. The first step was making sure the DC wasn't actually used internally :)
Comment on lines
+1987
to
+1988
| auto typeDecl = cast<TypeDecl>(result.getValueDecl()); | ||
| results.push_back(typeDecl); |
There was a problem hiding this comment.
Interesting! I'd like to understand this change. Was the old check not needed?
There was a problem hiding this comment.
Name lookup only returns TypeDecls if the TypesOnly flag is set, and we already rely on that being the case in other places in the compiler.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ASTScope takes a SourceFile and a SourceLoc, and finds the innermost scope containing the SourceLoc.
It used to also take a DeclContext, and if the DeclContext didn't match the innermost scope's DeclContext, walk up to the parent scope. Let's remove this hack and have the callers do the right thing instead.