Skip to content

[SourceKit NameLookup] Don't walk into inactive clauses. #28038

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 6, 2019
Merged

[SourceKit NameLookup] Don't walk into inactive clauses. #28038

merged 2 commits into from
Nov 6, 2019

Conversation

davidungar
Copy link
Contributor

Don't do lookups in inactive #if clauses as long as ASTScopes do not support them.
Addresses rdar://56760957

@davidungar davidungar requested a review from nathawes November 2, 2019 19:42
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test

@davidungar
Copy link
Contributor Author

@nathawes This PR addresses the issue by not walking into inactive clauses. I realize that this fix may reduce the utility of SourceKit in these cases. Is this limitation acceptable? If not, I see two alternatives:

  1. Disable ASTScopes for these activities and fall back to the declaration-context-based lookup system. This change will work for the short term, but will require more thought when we completely replace the old context-based lookups. Or,

  2. Change ASTScopes to support lookups into inactive clauses. This change will not be easy because of the way that inactive clauses are currently handled in the AST. In addition, @DougGregor has expressed the intuition that the compiler should not be in the business of lookups into inactive clauses, but I don't know if he would change his mind for this use case.

So, please take a gander at this PR and let me know what you think.

Thanks!

@davidungar davidungar force-pushed the rdar-56760957-dont-do-ide-lookups-into-inactive-clauses branch from 40cac39 to b071e5f Compare November 3, 2019 18:41
@davidungar
Copy link
Contributor Author

@nathawes Never mind--I think this fix is too draconian. Let me see what I can come up with.

@davidungar davidungar force-pushed the rdar-56760957-dont-do-ide-lookups-into-inactive-clauses branch from b071e5f to 2078945 Compare November 3, 2019 19:35
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test

@davidungar
Copy link
Contributor Author

@nathawes OK, I've got something that just forestalls the problematic lookups. If it tests acceptably, I think it's good. Check it out and let me know. Tx.

@davidungar
Copy link
Contributor Author

@swift-ci please smoke test osx platform

@davidungar
Copy link
Contributor Author

@swift-ci please clean smoke test os x platform

DougGregor
DougGregor reviewed Nov 4, 2019
View reviewed changes
lib/IDE/SyntaxModel.cpp Outdated
if (auto *repr = ED->getExtendedTypeRepr())
NSR = repr->getSourceRange();
if (!inInactiveClause || ASTScope::areInactiveIfConfigClausesSupported()) {
if (auto *repr = ED->getExtendedTypeRepr())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retrieving the type representation should be purely syntactic; I don't think this change should be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler crashes without it. Help me out here: what would be a better fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DougGregor Thanks to @nathawes , I realized that I completely misunderstood your comment here. I've removed this change. Thanks for your help and patience.

@davidungar davidungar force-pushed the rdar-56760957-dont-do-ide-lookups-into-inactive-clauses branch from 2078945 to faa0d50 Compare November 5, 2019 19:37
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test

nathawes
nathawes approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@nathawes nathawes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for fixing it!

@davidungar davidungar force-pushed the rdar-56760957-dont-do-ide-lookups-into-inactive-clauses branch from faa0d50 to cd95ddb Compare November 5, 2019 22:54
@davidungar
Copy link
Contributor Author

@swift-ci please smoke test

@davidungar
Copy link
Contributor Author

@swift-ci please smoke test os x platform

@davidungar
Copy link
Contributor Author

@swift-ci please clean smoke test os x platform

@davidungar davidungar merged commit 069db6e into swiftlang:master Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DougGregor DougGregor DougGregor approved these changes

@nathawes nathawes nathawes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidungar @DougGregor @nathawes