Skip to content

Prepare to disable parser lookup #34158

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

Merged

Conversation

slavapestov
Copy link
Contributor

@slavapestov slavapestov commented Oct 2, 2020

This PR includes the final set of fixes before all remaining test failures are diagnostics changes. It also adds an -enable-parser-lookup flag to complement the existing -disable-parser-lookup; it becomes useful once the default is to disable parser lookup.

To deal with the test output differences, I created a second copy of each test where the output changes after disabling parser lookup. The primary copy now explicitly calls the frontend with -disable-parser-lookup and expects the new diagnostics; the *_parser_lookup.swift version calls the frontend with -enable-parser-lookup and has the old expectations.

This allows us to turn parser lookup on and off by default without disturbing tests. Once parser lookup is completely removed we can remove the *_parser_lookup.swift variants.

Note: Parser lookup remains enabled by default with this PR, except for the handful of tests where it is now explicitly disabled.

This PR builds upon #34135.

@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov slavapestov force-pushed the prepare-to-disable-parser-lookup branch from 10e85a8 to 5457b1a Compare October 2, 2020 15:48
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov slavapestov requested a review from davidungar October 2, 2020 21:33
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

…rnEntryDeclScope

Today, the reported source range of a GuardStmtScope is just that of
the statement itself, ending after the 'else' block. Similarly, a
PatternEntryDeclScope ends immediately after the initializer
expression.

Since these scopes introduce names until the end of the parent
BraceStmt (meaning they change the insertion point), we were
relying on the addition of child scopes to extend the source
range.

While we still need the hack for extending source ranges to deal
with string interpolation, at least in the other cases we can
get rid of it.

Note that this fixes a bug where a GuardStmtScope would end
before an inactive '#if' block if the inactive '#if' block was
the last element of a BraceStmt.
@slavapestov slavapestov force-pushed the prepare-to-disable-parser-lookup branch from 5457b1a to e5c813a Compare October 3, 2020 03:55
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

Another spot where we have to check for redeclaration manually now.
As before, we can use the existing mechanism though.
This is for re-enabling it once it is turned off by default.
I created a second copy of each test where the output changes
after disabling parser lookup. The primary copy now explicitly
calls the frontend with -disable-parser-lookup and expects the
new diagnostics; the *_parser_lookup.swift version calls the
frontend with -enable-parser-lookup and has the old expectations.

This allows us to turn parser lookup on and off by default
without disturbing tests. Once parser lookup is completely
removed we can remove the *_parser_lookup.swift variants.
@slavapestov slavapestov force-pushed the prepare-to-disable-parser-lookup branch from e5c813a to bd36100 Compare October 3, 2020 13:38
@slavapestov
Copy link
Contributor Author

@swift-ci Please smoke test

@slavapestov slavapestov merged commit 24c80a3 into swiftlang:main Oct 3, 2020
Copy link
Contributor

@davidungar davidungar left a comment

Choose a reason for hiding this comment

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

Took a quick look at it, even post-merge. Looks good!

Comment on lines +858 to +863
insertionPoint =
ifUniqueConstructExpandAndInsert<PatternEntryDeclScope>(
insertionPoint, patternBinding, i,
isLocalBinding, endLocForBinding)
.getPtrOr(insertionPoint);

Copy link
Contributor

Choose a reason for hiding this comment

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

Glad we spoke about this offline. Makes perfect sense.

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.

2 participants