Skip to content
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

Point at type ascription before macro invocation on expansion parse error #62816

Merged
merged 1 commit into from
Aug 4, 2019

Conversation

estebank
Copy link
Contributor

Fix #47666. Follow up to #62791.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2019
@estebank
Copy link
Contributor Author

The only relevant change is the last commit, which adds diagnostic information to Mac_ to keep the state of last_type_ascription at the invocation place location, to be able to ferry it when expanding the macro through ExpansionData.

@petrochenkov
Copy link
Contributor

I need to think whether a less hacky solution exists, I wouldn't want to merge this as is.

@estebank
Copy link
Contributor Author

Given the way this works it seems to me like the only available option is to collect the state of the parser at the invocation place. I understand the hesitance of merging the PR as is, but I think it is as good a starting point as we're gonna find without a larger refactor.

@edmilsonefs
Copy link

Hey! This is a ping from triage, we would like to know if you @petrochenkov could give us a few minutes to share your thoughts on it.

@petrochenkov
Copy link
Contributor

Low priority, deferred until the weekend (unless I deal with other reviews earlier).

(@estebank, could you rebase this perhaps in the meantime? #62791 has landed.)

@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 4, 2019

So, the general idea is to create a parser snapshot for every macro invocation, so when the macro is expanded we can parse its output using a parser from that snapshot rather than a fresh parser.

Except that the only part of the parser this PR snapshots is the prior_type_ascription field.
And the snapshots are kept directly in the AST and not in some side table because we don't have node IDs assigned this early.

Since we create a fresh parser when expanding any macro anyway (which is not an entirely cheap operation), I wonder, how much more expensive it would be to clone the existing one instead.
Let's land this PR in the meantime, the diff is not large, it won't be hard to rework or revert if necessary.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2019

📌 Commit c82e1f2 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2019
@bors
Copy link
Contributor

bors commented Aug 4, 2019

⌛ Testing commit c82e1f2 with merge f01b9f8...

bors added a commit that referenced this pull request Aug 4, 2019
Point at type ascription before macro invocation on expansion parse error

Fix #47666. Follow up to #62791.

r? @petrochenkov
@bors
Copy link
Contributor

bors commented Aug 4, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing f01b9f8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 4, 2019
@bors bors merged commit c82e1f2 into rust-lang:master Aug 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unfortunate error when typoing : for ::
5 participants