Assert Spans are well-formed #122418
Closed
Assert Spans are well-formed #122418
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nnethercote (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
I'm pretty skeptical about this, with procedural macros you can pass an entirely arbitrary pair of spans to a combining function like Maybe it makes sense to check that it doesn't happen in "usual" well-behaving code, but it's always possible to construct such spans if the user tries hard enough. |
nnethercote
reviewed
Mar 13, 2024
| @@ -103,6 +103,10 @@ impl Span { | |||
| ctxt: SyntaxContext, | |||
| parent: Option<LocalDefId>, | |||
| ) -> Self { | |||
| if cfg!(debug_assertions) { | |||
| assert!(hi > lo, "low end of span is after high"); | |||
There was a problem hiding this comment.
Simpler would be just debug_assert!. Also, this should print the lo and hi values, so something like:
debug_assert!(hi > lo, "low end of span is after high: lo={lo:?}, hi={hi:?}");
I agree with @petrochenkov that this might not actually be practical, despite the existence of #47504. And the test failure is further evidence of that. The relevant part of the failing stacktrace:
11: 0x7f292995abb5 - core[301f0576711e1b37]::panicking::panic_fmt
12: 0x7f292d6fa2a2 - <rustc_parse[cd33da97dab93ae0]::lexer::StringReader>::next_token
12: 0x7f292d6fa2a2 - <rustc_parse[cd33da97dab93ae0]::lexer::StringReader>::next_token
13: 0x7f292d6af112 - <rustc_parse[cd33da97dab93ae0]::lexer::tokentrees::TokenTreesReader>::parse_token_trees
14: 0x7f292d6ae4b8 - <rustc_parse[cd33da97dab93ae0]::lexer::tokentrees::TokenTreesReader>::parse_all_token_trees
15: 0x7f292d6fe2e2 - rustc_parse[cd33da97dab93ae0]::maybe_file_to_stream
16: 0x7f292d6fde87 - rustc_parse[cd33da97dab93ae0]::maybe_source_file_to_parser
17: 0x7f292d6fd9bf - rustc_parse[cd33da97dab93ae0]::maybe_new_parser_from_source_str
18: 0x7f292a91decb - rustc_interface[a0975ad2d4cbfd75]::interface::parse_check_cfg
(Why is line 12 repeated? Not sure.)
parse_check_cfg is very early in the compiler's execution. Have you run any tests locally with this patch applied, e.g. with "x.py tests tests/ui"? That runs a good subset of the test suite.
|
I'll flip the review switch as I see some comments asking for a design rethinking (IIUC) @rustbot author |
rustbot
added
S-waiting-on-author
|
@ariscript any updates on the response to the review above? thanks |
|
I'm not entirely sure what can be done here then if the main premise of the PR is not the best idea. I'm not a huge expert with how the compiler works. I wanted to get involved with compiler development so I picked an easy-labeled issue. If this is a bad idea, it might be best to close the issue and PR. |
Yeah, I think this is harder than it first appears. I will remove the "easy" label, and I'll close this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Makes invalid spans easier to debug by asserting that the low end must be less than high.
First time writing a PR for rust, so let me know if I'm doing this right :)
Resolves #47504.