-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Fix a few diagnostics #152328
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
Fix a few diagnostics #152328
Conversation
| #[help( | ||
| "if you meant to call a macro, remove the `pub` and add a trailing `!` after the identifier" | ||
| )] | ||
| HelpMacro, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this one I can't find a way to trigger this help message (though I didn't try very hard)
At least I added an assertion for the message above :)
| #[derive(Subdiagnostic)] | ||
| #[multipart_suggestion( | ||
| "you can wrap the call in an `unsafe` block if you can guarantee that the environment access only happens in single-threaded code", | ||
| "you can wrap the call in an `unsafe` block if you can guarantee {$guarantee}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, is this correct? guarantee is in the parent diagnostic. If it is, it seems quite error-prone e.g. with respect to refactoring (or complicates checking the syntax at compile time I guess).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct, subdiagnostics can use variables from their parent diagnostic, and this message (and quite a few others) do!
This was already the case, I also don't like it, and want to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also why subdiagnostics are currently exempted from the compile-time "do variables exist" check, again, was also already the case before my refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great to hear, it'll help reduce this magic flexibility for the better, surely.
|
Looks good to me, @bors r=jdonszelmann,lqd |
…zelmann,lqd Fix a few diagnostics When working on the inline diagnostics conversion (rust-lang#151366), I noticed that my script sometimes took the wrong message. Because it didn't happen very often, I just fixed it manually when a uitest fails. However I got paranoid that the script changed messages that were not covered by uitests, so I checked for all messages in the previous `messages.ftl` files, whether they occured at least once in the codebase. I found 3 messages that indeed were wrongly replaced by my script, fixed them, and added uitests to make sure this doesn't happen again :) r? @jdonszelmann (Anyone else, also feel free to review, just assigning to Jana because she's been reviewing the other PRs)
…uwer Rollup of 7 pull requests Successful merges: - #151455 (Fix `SourceFile::normalized_byte_pos`) - #152250 (Remove support for slugs in diagnostic messages) - #152322 (Replace some `feature(core_intrinsics)` with stable hints) - #152328 (Fix a few diagnostics) - #151640 (Cleanup offload datatransfer) - #152212 (Port some attributes to the attr parser) - #152309 (Fix bound var resolution for trait aliases)
|
Right, |
7c37cd6 to
d14c26f
Compare
|
@bors try jobs=test-various |
This comment has been minimized.
This comment has been minimized.
Fix a few diagnostics try-job: test-various
|
@bors r+ |
…uwer Rollup of 9 pull requests Successful merges: - #151455 (Fix `SourceFile::normalized_byte_pos`) - #152250 (Remove support for slugs in diagnostic messages) - #152322 (Replace some `feature(core_intrinsics)` with stable hints) - #152328 (Fix a few diagnostics) - #151640 (Cleanup offload datatransfer) - #152212 (Port some attributes to the attr parser) - #152309 (Fix bound var resolution for trait aliases) - #152339 (diagnostics: fix ICE in closure signature mismatch) - #152341 (`cfg_select!`: allow optional comma after `{ /* ... */ }`)
|
Oops, this is supposed to be |
|
I'm more surprised by the fact that old bors used to report when a commit that was already approved was reapproved. (It's not worth recreating the rollup to pick up the new reviewers, but thanks for thinking of our internet points 🙃) |
Rollup merge of #152328 - JonathanBrouwer:fix_diags, r=JonathanBrouwer Fix a few diagnostics When working on the inline diagnostics conversion (#151366), I noticed that my script sometimes took the wrong message. Because it didn't happen very often, I just fixed it manually when a uitest fails. However I got paranoid that the script changed messages that were not covered by uitests, so I checked for all messages in the previous `messages.ftl` files, whether they occured at least once in the codebase. I found 3 messages that indeed were wrongly replaced by my script, fixed them, and added uitests to make sure this doesn't happen again :) r? @jdonszelmann (Anyone else, also feel free to review, just assigning to Jana because she's been reviewing the other PRs)
When working on the inline diagnostics conversion (#151366), I noticed that my script sometimes took the wrong message.
Because it didn't happen very often, I just fixed it manually when a uitest fails.
However I got paranoid that the script changed messages that were not covered by uitests, so I checked for all messages in the previous
messages.ftlfiles, whether they occured at least once in the codebase. I found 3 messages that indeed were wrongly replaced by my script, fixed them, and added uitests to make sure this doesn't happen again :)r? @jdonszelmann (Anyone else, also feel free to review, just assigning to Jana because she's been reviewing the other PRs)