Skip to content

Conversation

@davidungar
Copy link
Contributor

@davidungar davidungar commented Oct 17, 2019

Do not try to do lookups on OpaqueDecls that are in inactive clauses.
Also, fail the compilation for any lookups into inactive clauses.

@davidungar davidungar requested a review from DougGregor October 17, 2019 00:42
@davidungar
Copy link
Contributor Author

@swift-ci please test

@davidungar davidungar force-pushed the fix-opaque-decl-astscope branch from 15d613b to 2e8920b Compare October 17, 2019 05:24
@davidungar
Copy link
Contributor Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 15d613bc273e28eab7d890034d2747853bed2476

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 15d613bc273e28eab7d890034d2747853bed2476

<< sourceFile->getFilename() << "\n";
// The check is costly, and inactive lookups will end up here, so don't
// do the check unless we can't find the startingScope.
const bool isInInactiveClause =
Copy link
Member

Choose a reason for hiding this comment

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

I don’t understand why we need this part, of the parser isn’t going to add these decls in the first place for inactive regions?

Copy link
Contributor Author

@davidungar davidungar Oct 17, 2019

Choose a reason for hiding this comment

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

Tx for looking this over.

The idea is that lookups to things in inactive clauses may creep back in at some point. If they do, this part will output a more specific message for debugging, and also, as per your preference, will crash the compilation. It only runs when the ASTScope code cannot find the starting scope anyway, so it doesn't slow down compilation unless there's an error.

When someday, we rip out all the code that passed DeclContexts into UnqualifiedLookup, the whole did-not-find starting scope stuff will get ripped out.

Copy link
Member

Choose a reason for hiding this comment

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

Okay!

@davidungar davidungar merged commit 924da0c into swiftlang:master Oct 17, 2019
@davidungar
Copy link
Contributor Author

As per offline conversation with @DougGregor , am proceeding to merge.

@davidungar
Copy link
Contributor Author

Likely fixes rdar://56334618

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants