Skip to content

Conversation

@cyrgani
Copy link
Contributor

@cyrgani cyrgani commented Oct 18, 2025

Before this PR, calling TokenStream::from_str or Literal::from_str with an invalid argument would always cause a compile error, even if the TokenStream is not used afterwards at all.
This PR changes this so it returns a LexError instead in some cases.

This is very theoretically a breaking change, but the doc comment on the impl already says

/// NOTE: some errors may cause panics instead of returning `LexError`. We reserve the right to
/// change these errors into `LexError`s later.

Fixes some cases of #58736.

@rustbot
Copy link
Collaborator

rustbot commented Oct 18, 2025

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rust-analyzer Relevant to the rust-analyzer team, which will review and decide on the PR/issue. labels Oct 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 18, 2025

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@cyrgani
Copy link
Contributor Author

cyrgani commented Oct 18, 2025

@rustbot 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 Oct 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 18, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@cyrgani cyrgani force-pushed the nonfatal-tokenstream-parse branch from e2c0009 to 80b67bb Compare October 19, 2025 09:03
@cyrgani
Copy link
Contributor Author

cyrgani commented Oct 19, 2025

@rustbot ready

@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 Oct 19, 2025
@cyrgani cyrgani force-pushed the nonfatal-tokenstream-parse branch from ba098bc to 0274d78 Compare October 21, 2025 14:28
@cyrgani
Copy link
Contributor Author

cyrgani commented Oct 23, 2025

This does not fix #58736 entirely yet:

TokenStream::from_str("0b2");

is still a compile error because it internally calls literal_from_str, which then emits the diagnostic.

@davidtwco
Copy link
Member

I'm not familiar enough with this part of the compiler

r? compiler

@rustbot rustbot assigned wesleywiser and unassigned davidtwco Oct 25, 2025
@cyrgani cyrgani force-pushed the nonfatal-tokenstream-parse branch from 0274d78 to 13d3d06 Compare October 29, 2025 22:19
@rustbot

This comment has been minimized.

@cyrgani cyrgani changed the title replace TokenStream::from_str panics with LexErrors reduce the amount of panics in {TokenStream, Literal}::from_str calls Oct 29, 2025
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 17, 2025
add larger test for `proc_macro` `FromStr` implementations

Currently, there are only few tests that check the output of `TokenStream::from_str` and `Literal::from_str` (which is somewhat understandable as the rustc implementation just delegates these calls to the parser). In preparation for both the standalone backend (rust-lang#130856) which will probably need to reimplement this logic as well as for removing panics from these functions (rust-lang#58736), this PR adds a test which shows the various messy ways of how these functions report errors and the return values for successful parses.
Followup PRs such as rust-lang#147859 will change more and more of these "diagnostic + error"s into `LexErrors`.

The test structure with the extra module is used to allow reusing it later easily for the standalone backend.
jhpratt added a commit to jhpratt/rust that referenced this pull request Nov 17, 2025
add larger test for `proc_macro` `FromStr` implementations

Currently, there are only few tests that check the output of `TokenStream::from_str` and `Literal::from_str` (which is somewhat understandable as the rustc implementation just delegates these calls to the parser). In preparation for both the standalone backend (rust-lang#130856) which will probably need to reimplement this logic as well as for removing panics from these functions (rust-lang#58736), this PR adds a test which shows the various messy ways of how these functions report errors and the return values for successful parses.
Followup PRs such as rust-lang#147859 will change more and more of these "diagnostic + error"s into `LexErrors`.

The test structure with the extra module is used to allow reusing it later easily for the standalone backend.
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 17, 2025
add larger test for `proc_macro` `FromStr` implementations

Currently, there are only few tests that check the output of `TokenStream::from_str` and `Literal::from_str` (which is somewhat understandable as the rustc implementation just delegates these calls to the parser). In preparation for both the standalone backend (rust-lang#130856) which will probably need to reimplement this logic as well as for removing panics from these functions (rust-lang#58736), this PR adds a test which shows the various messy ways of how these functions report errors and the return values for successful parses.
Followup PRs such as rust-lang#147859 will change more and more of these "diagnostic + error"s into `LexErrors`.

The test structure with the extra module is used to allow reusing it later easily for the standalone backend.
JaclynCodes added a commit to JaclynCodes/rust that referenced this pull request Nov 17, 2025
add larger test for `proc_macro` `FromStr` implementations

Currently, there are only few tests that check the output of `TokenStream::from_str` and `Literal::from_str` (which is somewhat understandable as the rustc implementation just delegates these calls to the parser). In preparation for both the standalone backend (rust-lang#130856) which will probably need to reimplement this logic as well as for removing panics from these functions (rust-lang#58736), this PR adds a test which shows the various messy ways of how these functions report errors and the return values for successful parses.
Followup PRs such as rust-lang#147859 will change more and more of these "diagnostic + error"s into `LexErrors`.

The test structure with the extra module is used to allow reusing it later easily for the standalone backend.
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 29, 2025
@bors
Copy link
Collaborator

bors commented Dec 1, 2025

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

@rust-cloud-vms rust-cloud-vms bot force-pushed the nonfatal-tokenstream-parse branch from 91a861c to 24b96f1 Compare December 14, 2025 22:01
@rustbot

This comment has been minimized.

@cyrgani cyrgani 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 Dec 14, 2025
@rust-cloud-vms rust-cloud-vms bot force-pushed the nonfatal-tokenstream-parse branch from 24b96f1 to d847df3 Compare December 14, 2025 22:12
@JonathanBrouwer
Copy link
Contributor

JonathanBrouwer commented Dec 15, 2025

I think there is still work to do in improving the error messages, but this can happen in future PRs, this is already a big improvement.
One final concern I have: Are there any stability guarantees for the Display implementation of LexError? I'm assuming there wouldn't (or at least shouldn't) be but not exactly sure if/where this is documented.
This PR changes the Display implementation of the type & I'd like to leave the option of improving these messages later open.

@JonathanBrouwer
Copy link
Contributor

@rustbot 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 Dec 15, 2025
@JonathanBrouwer
Copy link
Contributor

Started a thread here to discuss this #t-libs > Questions for a PR I'm reviewing @ 💬

@rust-bors

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the nonfatal-tokenstream-parse branch from d847df3 to e21dfa8 Compare January 25, 2026 22:23
@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the nonfatal-tokenstream-parse branch 2 times, most recently from 6f82492 to 33ff2a3 Compare January 26, 2026 10:56
Copy link
Contributor

@JonathanBrouwer JonathanBrouwer left a comment

Choose a reason for hiding this comment

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

From a T-compiler perspective this code looks fine, but I'm not super familiar with this code and feel like T-libs should also have a say about it, so will re-assign.
r? libs

View changes since this review

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 27, 2026
@rustbot rustbot assigned joboet and unassigned JonathanBrouwer Jan 27, 2026
@@ -1378,6 +1378,13 @@ impl<'a, G: EmissionGuarantee> Diag<'a, G> {
drop(self);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will put the text from my comment above in a discussion so it is not missed:

One final concern I have: Are there any stability guarantees for the Display implementation of LexError? I'm assuming there wouldn't (or at least shouldn't) be but not exactly sure if/where this is documented.
This PR changes the Display implementation of the type & I'd like to leave the option of improving these messages later open.

@joboet
Copy link
Member

joboet commented Jan 27, 2026

I'm not very familiar with proc_macro.

r? @petrochenkov

@rustbot rustbot assigned petrochenkov and unassigned joboet Jan 27, 2026
@rust-bors

This comment has been minimized.

@rust-cloud-vms rust-cloud-vms bot force-pushed the nonfatal-tokenstream-parse branch from 33ff2a3 to 519a3f7 Compare January 27, 2026 21:29
@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

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

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rust-analyzer Relevant to the rust-analyzer team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.