Remove old unqualified lookup implementation #33822
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
slavapestov
force-pushed
the
remove-old-unqualified-lookup
branch
from
868a7c3 to
ef4908c
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
davidungar
approved these changes
Sep 5, 2020
Comment on lines
494
to
518
|
|
||
| auto *DC = VD->getDeclContext(); | ||
| if (!DC->isTypeContext()) { | ||
| SmallVector<LookupResultEntry, 8> RelatedDecls; | ||
|
|
||
| auto descriptor = UnqualifiedLookupDescriptor(DeclNameRef(VD->getBaseName()), | ||
| DC->getModuleScopeContext()); | ||
|
|
||
| auto lookup = evaluateOrDefault(ctx.evaluator, | ||
| UnqualifiedLookupRequest{descriptor}, {}); | ||
|
|
||
| for (auto result : lookup) { | ||
| ValueDecl *RelatedVD = result.getValueDecl(); | ||
| if (RelatedVD->getAttrs().isUnavailable(VD->getASTContext())) | ||
| continue; | ||
|
|
||
| if (RelatedVD != VD) { | ||
| ++NamesSeen[RelatedVD->getName()]; | ||
| RelatedDecls.push_back(result); | ||
| } | ||
| } | ||
|
|
||
| // Now provide the results along with whether the name is duplicate or not. | ||
| ValueDecl *OriginalBase = VD->getDeclContext()->getSelfNominalTypeDecl(); | ||
| for (auto Related : RelatedDecls) { | ||
| ValueDecl *RelatedVD = Related.getValueDecl(); | ||
| bool SameBase = Related.getBaseDecl() && Related.getBaseDecl() == OriginalBase; | ||
| Fn(RelatedVD, SameBase, NamesSeen[RelatedVD->getName()] > 1); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| SmallVector<ValueDecl *, 4> results; | ||
|
|
||
| auto type = DC->getDeclaredInterfaceType(); | ||
| if (!type->is<ErrorType>()) { | ||
| DC->lookupQualified(type, DeclNameRef(VD->getBaseName()), | ||
| NL_QualifiedDefault, results); | ||
| } | ||
|
|
||
| SmallVector<ValueDecl *, 8> RelatedDecls; | ||
| for (auto result : results) { | ||
| if (result->getAttrs().isUnavailable(ctx)) |
There was a problem hiding this comment.
This whole function could be easier to understand with either a comment describing the overall strategy, or by breaking it up.
There was a problem hiding this comment.
You're right. I completely refactored this function, will push a new version shortly.
This function used to perform an unqualified lookup without a source location, to either find top-level members or members of a type. Since this form of unqualified lookup is no longer supported, let's instead explicitly call lookupInModule() or lookupQualified(), depending on whether we're looking inside a type or not.
Now that ASTScope is unconditionally enabled, we can remove the old unqualified lookup implementation. Note that unqualified lookup now either requires a source location, or must start from a module-scope context. All existing usages should now respect this invariant.
slavapestov
force-pushed
the
remove-old-unqualified-lookup
branch
from
ef4908c to
83f49b3
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
davidungar
reviewed
Sep 6, 2020
Comment on lines
+492
to
+522
|
|
||
| llvm::SmallDenseMap<DeclName, unsigned, 16> NamesSeen; | ||
| ++NamesSeen[VD->getName()]; | ||
|
|
||
|
|
||
| auto *DC = VD->getDeclContext(); | ||
| bool typeLookup = DC->isTypeContext(); | ||
|
|
||
| SmallVector<ValueDecl *, 4> results; | ||
|
|
||
| if (typeLookup) { | ||
| auto type = DC->getDeclaredInterfaceType(); | ||
| if (!type->is<ErrorType>()) { | ||
| DC->lookupQualified(type, DeclNameRef(VD->getBaseName()), | ||
| NL_QualifiedDefault, results); | ||
| } | ||
| } else { | ||
| namelookup::lookupInModule(DC->getModuleScopeContext(), | ||
| VD->getBaseName(), results, | ||
| NLKind::UnqualifiedLookup, | ||
| namelookup::ResolutionKind::Overloadable, | ||
| DC->getModuleScopeContext()); | ||
| } | ||
|
|
||
| SmallVector<ValueDecl *, 8> RelatedDecls; | ||
| for (auto result : results) { | ||
| if (result->getAttrs().isUnavailable(ctx)) | ||
| continue; | ||
|
|
||
| if (RelatedVD != VD) { | ||
| ++NamesSeen[RelatedVD->getName()]; | ||
| if (result != VD) { | ||
| ++NamesSeen[result->getName()]; |
There was a problem hiding this comment.
Looking pretty good--easy to understand.
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.
The only remaining blocker to removing the old unqualified lookup implementation were a few places where we would perform an unqualified lookup from a non-top-level DeclContext, without a SourceLoc. This is no longer supported, since ASTScope always uses a SourceLoc to locate the innermost scope. First, this PR refactors these lookups to either start from a top-level context if there's no change in functionality, or perform a qualified lookup into a type if needed. With that out of the way, we can stick the right assertion in UnqualifiedLookup and remove the old code.
The next steps here are removing parse-time local variable lookup, since ASTScope can always find declarations in local scope, and refactoring LookupVisibleDecls to consult ASTScope instead of walking the AST directly. Once that is done, we'll finally have one canonical source of truth for unqualified lookups, concluding the ASTScope journey.