Skip to content

Improve error handling in symbols proc-macro #79944

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
merged 2 commits into from
Dec 14, 2020

Conversation

sivadeilra
Copy link

This improves how the symbols proc-macro handles errors.
If it finds an error in its input, the macro does not panic.
Instead, it still produces an output token stream. That token
stream will contain compile_error!(...) macro invocations.
This will still cause compilation to fail (which is what we want),
but it will prevent meaningless errors caused by the output not
containing symbols that the macro normally generates.

This solves a small (but annoying) problem. When you're editing
rustc_span/src/symbol.rs, and you get something wrong (dup
symbol name, misordered symbol), you want to get only the errors
that are relevant, not a burst of errors that are irrelevant.
This change also uses the correct Span when reporting errors,
so you get errors that point to the correct place in
rustc_span/src/symbol.rs where something is wrong.

This also adds several unit tests which test the symbols proc-macro.

This commit also makes it easy to run the symbols proc-macro
as an ordinary Cargo test. Just run cargo test. This makes it
easier to do development on the macro itself, such as running it
under a debugger.

This commit also uses the Punctuated type in syn for parsing
comma-separated lists, rather than doing it manually.

The output of the macro is not changed at all by this commit,
so rustc should be completely unchanged. This just improves
quality of life during development.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2020
@jyn514 jyn514 added A-contributor-roadblock Area: Makes things more difficult for new or seasoned contributors to Rust A-proc-macros Area: Procedural macros T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 11, 2020
@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned eddyb Dec 11, 2020
@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 Dec 12, 2020
This improves how the `symbols` proc-macro handles errors.
If it finds an error in its input, the macro does not panic.
Instead, it still produces an output token stream. That token
stream will contain `compile_error!(...)` macro invocations.
This will still cause compilation to fail (which is what we want),
but it will prevent meaningless errors caused by the output not
containing symbols that the macro normally generates.

This solves a small (but annoying) problem. When you're editing
rustc_span/src/symbol.rs, and you get something wrong (dup
symbol name, misordered symbol), you want to get only the errors
that are relevant, not a burst of errors that are irrelevant.
This change also uses the correct Span when reporting errors,
so you get errors that point to the correct place in
rustc_span/src/symbol.rs where something is wrong.

This also adds several unit tests which test the `symbols` proc-macro.

This commit also makes it easy to run the `symbols` proc-macro
as an ordinary Cargo test. Just run `cargo test`. This makes it
easier to do development on the macro itself, such as running it
under a debugger.

This commit also uses the `Punctuated` type in `syn` for parsing
comma-separated lists, rather than doing it manually.

The output of the macro is not changed at all by this commit,
so rustc should be completely unchanged. This just improves
quality of life during development.
@sivadeilra sivadeilra force-pushed the syms_proc_macro_testing branch from 9c153b9 to 201a833 Compare December 12, 2020 23:29
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Dec 12, 2020

📌 Commit 201a833 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2020
@bors
Copy link
Collaborator

bors commented Dec 13, 2020

⌛ Testing commit 201a833 with merge 99b6e5efcf6cdf32bc344025f0279e38c1bd4a1a...

@bors
Copy link
Collaborator

bors commented Dec 13, 2020

💔 Test failed - checks-actions

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

Needs ./x.py fmt.

@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 Dec 13, 2020
@sivadeilra
Copy link
Author

Needs ./x.py fmt.

Done.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 13, 2020

📌 Commit 1a5b9b0 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 13, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 14, 2020
… r=petrochenkov

Improve error handling in `symbols` proc-macro

This improves how the `symbols` proc-macro handles errors.
If it finds an error in its input, the macro does not panic.
Instead, it still produces an output token stream. That token
stream will contain `compile_error!(...)` macro invocations.
This will still cause compilation to fail (which is what we want),
but it will prevent meaningless errors caused by the output not
containing symbols that the macro normally generates.

This solves a small (but annoying) problem. When you're editing
rustc_span/src/symbol.rs, and you get something wrong (dup
symbol name, misordered symbol), you want to get only the errors
that are relevant, not a burst of errors that are irrelevant.
This change also uses the correct Span when reporting errors,
so you get errors that point to the correct place in
rustc_span/src/symbol.rs where something is wrong.

This also adds several unit tests which test the `symbols` proc-macro.

This commit also makes it easy to run the `symbols` proc-macro
as an ordinary Cargo test. Just run `cargo test`. This makes it
easier to do development on the macro itself, such as running it
under a debugger.

This commit also uses the `Punctuated` type in `syn` for parsing
comma-separated lists, rather than doing it manually.

The output of the macro is not changed at all by this commit,
so rustc should be completely unchanged. This just improves
quality of life during development.
@bors
Copy link
Collaborator

bors commented Dec 14, 2020

⌛ Testing commit 1a5b9b0 with merge 331e740...

@bors
Copy link
Collaborator

bors commented Dec 14, 2020

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 331e740 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 14, 2020
@bors bors merged commit 331e740 into rust-lang:master Dec 14, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 14, 2020
@sivadeilra sivadeilra deleted the syms_proc_macro_testing branch December 14, 2020 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new or seasoned contributors to Rust A-proc-macros Area: Procedural macros 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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants