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.

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

Error on Reserved Syntax for Raw Keywords: k#foo #84037

Closed
wants to merge 2 commits into from
Closed

Error on Reserved Syntax for Raw Keywords: k#foo #84037

wants to merge 2 commits into from

Conversation

Julian-Wollersberger
Copy link
Contributor

@Julian-Wollersberger Julian-Wollersberger commented Apr 9, 2021 •
edited

This PR makes the raw keyword syntax from rust-lang/rfcs#3098 a hard error on all editions. (In contrast to what the RFC currently says.)

error: raw keyword syntax is reserved for future use
   |
LL |     one_token!(k#ident);
   |                ^^^^^^^ help: insert a whitespace: `k #ident`

More specifically, it becomes a tokenization error, so macros cannot use it. k#ident is now lexed as one token, similar to r#ident, although this could be changed later without breaking anything.

Note that this is a breaking change, since macros previously interpreted it as three tokens, so something like quote!{ #k#other_variable } was valid. The crater run showed no real regressions though. You can easily get the old behavior by inserting a whitespace between k and #.

The actual raw keywords feature for things like k#async is not implemented here.

Previously, this PR was inteded to do a crater run for all reserved prefixes from rust-lang/rfcs#3101, but even the try build ran into breakage in the ecosystem, so only k#... was tested.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2021
@joshtriplett
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Apr 9, 2021

⌛ Trying commit 692220c974656fac8908e4edc45b731a22fcb220 with merge 183585b6408d129d017caa116edd19c6d5f7f716...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 9, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Julian-Wollersberger
Copy link
Contributor Author

Julian-Wollersberger commented Apr 9, 2021 •
edited

I fixed the build errors in rustdoc, but the try build already ran into breakage in the ecosystem. tracing-attributes contains:

quote! {
    #name = #kind#value
}

Instances like this one should be somewhat common with the quote! macro. It's easily fixed though, by inserting a whitespace between #kind and #value.

What's the best way forward here? Should I just leave the error for raw keywords k#foo and remove the others? Or try to fix tracing-attributes?
AFAIK, only the raw keywords syntax was considered to use in older editions anyway.

@joshtriplett
Copy link
Member

Yeah, let's prioritize getting a crater run that just checks if we can use k#keyword in every edition.

@Julian-Wollersberger Julian-Wollersberger changed the title [Crater] Error on Reserved Prefixes: k#foo, bar"baz", cha'r' [Crater] Error on Reserved Prefix: k#foo Apr 10, 2021
@Julian-Wollersberger
Copy link
Contributor Author

Ok, I removed the other errors and updated the PR description.
@joshtriplett can you start another try run?

@tmiasko
Copy link
Contributor

tmiasko commented Apr 10, 2021

@bors try

@bors
Copy link
Contributor

bors commented Apr 10, 2021

⌛ Trying commit 2bbdfdfac232fea427fdb311560411c39e0d6c39 with merge 2bf0c8e648350595527d984daafa09b33c338d13...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 10, 2021

☀️ Try build successful - checks-actions
Build commit: 2bf0c8e648350595527d984daafa09b33c338d13 (2bf0c8e648350595527d984daafa09b33c338d13)

@joshtriplett
Copy link
Member

Looks like one test failed due to the change in lexer structures. Can you fix that up, please?

@Julian-Wollersberger
Copy link
Contributor Author

Julian-Wollersberger commented Apr 10, 2021 •
edited

Done.

@rust-lang rust-lang deleted a comment from craterbot Apr 10, 2021
@joshtriplett
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Apr 10, 2021

⌛ Trying commit 85844b1980f1137c308f83cae30bb5b174391c48 with merge b0d0ea232bdae12fa8da8491a91ae1db029990ef...

@bors
Copy link
Contributor

bors commented Apr 10, 2021

☀️ Try build successful - checks-actions
Build commit: b0d0ea232bdae12fa8da8491a91ae1db029990ef (b0d0ea232bdae12fa8da8491a91ae1db029990ef)

@joshtriplett
Copy link
Member

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-84037 created and queued.
🤖 Automatically detected try build b0d0ea232bdae12fa8da8491a91ae1db029990ef
⚠️ Try build based on commit a836d9b, but latest commit is 85844b1980f1137c308f83cae30bb5b174391c48. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-84037 is completed!
📊 6 regressed and 18 fixed (156232 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 2, 2021
@joshtriplett
Copy link
Member

I reviewed the crater results, and it looks like zero failures as a result of the k#foo reservation. So, I think we can safely reserve k#foo in all editions.

@Julian-Wollersberger
Copy link
Contributor Author

I'll update this PR to more accurately implement the k#ident proposal, in the next week or so.

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 5, 2021
@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned lcnr May 5, 2021
@petrochenkov petrochenkov mentioned this pull request May 16, 2021
Merged
@Julian-Wollersberger Julian-Wollersberger changed the title [Crater] Error on Reserved Prefix: k#foo Error on Reserved Prefix: k#foo May 17, 2021
@Julian-Wollersberger Julian-Wollersberger changed the title Error on Reserved Prefix: k#foo Error on Reserved Syntax for Raw Keywords: k#foo May 17, 2021
@Julian-Wollersberger
Copy link
Contributor Author

Julian-Wollersberger commented May 17, 2021 •
edited

@petrochenkov this is ready for review now, and I guess also for lang team discussion. I also updated the PR description.
I redid the lexer changes, so it should accurately reflect the RFC (minus the r#$foo part). In essence, it does the same thing as r#ident now.

@scottmcm can you verify that this is what you had in mind in the RFC?

@rustbot label: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2021
@petrochenkov
Copy link
Contributor

@Julian-Wollersberger
Could you cooperate with @lrh2000 (#85359) and decide who is implementing what?
I didn't look at the changes in detail yet but these PRs appear to compete with each other.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2021
@crlf0710
Copy link
Member

crlf0710 commented Jun 5, 2021

Blocked on #85359 according to #85359 (comment)

@crlf0710 crlf0710 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 5, 2021
@bors
Copy link
Contributor

bors commented Jun 27, 2021

☔ The latest upstream changes (presumably #85359) made this pull request unmergeable. Please resolve the merge conflicts.

@Julian-Wollersberger
Copy link
Contributor Author

This is still blocked on the RFC 3098, but work on that seems to have stalled.
I'll close this PR for now, but please ping me if the RFC is merged. I'm still interested in implementing it.

@m-ou-se m-ou-se mentioned this pull request Sep 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet