Add runtime check to avoid overwrite arg in Diag
#142724
Open
+119
−39
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
rustbot
added
A-query-system
This comment has been minimized.
This comment has been minimized.
xizheyin
force-pushed
the
avoid_overwrite_args
branch
from
733b453 to
e0afd4a
Compare
oli-obk
reviewed
Jun 20, 2025
oli-obk
reviewed
Jun 20, 2025
xizheyin
force-pushed
the
avoid_overwrite_args
branch
from
e0afd4a to
faa502e
Compare
rustbot
added
the
A-translation
|
|
xizheyin
commented
Jun 21, 2025
Comment on lines
+622
to
+638
|
|
||
| // For #[derive(Subdiagnostic)] | ||
| // | ||
| // - Store args of the main diagnostic for later restore. | ||
| // - add args of subdiagnostic. | ||
| // - Generate the calls, such as note, label, etc. | ||
| // - Remove the arguments for allowing Vec<Subdiagnostic> to be used. | ||
| // - Restore the arguments for allowing main and subdiagnostic share the same fields. | ||
| Ok(quote! { | ||
| #init | ||
| #formatting_init | ||
| #attr_args | ||
| #store_args | ||
| #plain_args | ||
| #calls | ||
| #remove_args | ||
| #restore_args |
There was a problem hiding this comment.
For #[derive(Subdiagnostic)], I made two changes:
- After the subdiagnostic is rendered, remove all args of this subdiagnostic, which allows for usage like
Vec<Subdiag>. - Store
diag.argsbefore setting arguments, so that you can restore the contents of the main diagnostic after deleting the arguments after subdiagnostic is rendered, to avoid deleting the main diagnostic's arg when they have the same name args.
oli-obk
reviewed
Jun 25, 2025
oli-obk
reviewed
Jun 25, 2025
|
just some nits, I think this is the right solution |
… restore snapshot when set subdiag arg Signed-off-by: xizheyin <xizheyin@smail.nju.edu.cn>
xizheyin
force-pushed
the
avoid_overwrite_args
branch
from
faa502e to
d2d17c6
Compare
|
It's done. :) I'll update |
|
@bors r+ |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jun 25, 2025
…li-obk Add runtime check to avoid overwrite arg in `Diag` ## Origin PR description At first, I set up a `debug_assert` check for the arg method to make sure that `args` in `Diag` aren't easily overwritten, and I added the `remove_arg()` method, so that if you do need to overwrite an arg, then you can explicitly call `remove_arg()` to remove it first, then call `arg()` to overwrite it. For the code before the rust-lang#142015 change, it won't compile because it will report an error ``` arg `instance`already exists. ``` This PR also modifies all diagnostics that fail the check to pass the check. There are two cases of check failure: 1. ~~Between *the parent diagnostic and the subdiagnostic*, or *between the subdiagnostics* have the same field between them. In this case, I renamed the conflicting fields.~~ 2. ~~For subdiagnostics stored in `Vec`, the rendering may iteratively write the same arg over and over again. In this case, I changed the auto-generation with `derive(SubDiagnostic)` to manually implementing `SubDiagnostic` and manually rendered it with `eagerly_translate()`, similar to rust-lang#142031 (comment), and after rendering it I manually deleted useless arg with the newly added `remove_arg` method.~~ ## Final Decision After trying and discussing, we made a final decision. For `#[derive(Subdiagnostic)]`, This PR made two changes: 1. After the subdiagnostic is rendered, remove all args of this subdiagnostic, which allows for usage like `Vec<Subdiag>`. 2. Store `diag.args` before setting arguments, so that you can restore the contents of the main diagnostic after deleting the arguments after subdiagnostic is rendered, to avoid deleting the main diagnostic's arg when they have the same name args.
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.
Origin PR description
At first, I set up a
debug_assertcheck for the arg method to make sure thatargsinDiagaren't easily overwritten, and I added theremove_arg()method, so that if you do need to overwrite an arg, then you can explicitly callremove_arg()to remove it first, then callarg()to overwrite it.For the code before the #142015 change, it won't compile because it will report an error
This PR also modifies all diagnostics that fail the check to pass the check. There are two cases of check failure:
Between the parent diagnostic and the subdiagnostic, or between the subdiagnostics have the same field between them. In this case, I renamed the conflicting fields.For subdiagnostics stored inVec, the rendering may iteratively write the same arg over and over again. In this case, I changed the auto-generation withderive(SubDiagnostic)to manually implementingSubDiagnosticand manually rendered it witheagerly_translate(), similar to fluent diagnostics can easily overwrite each others' fields/args #142031 (comment), and after rendering it I manually deleted useless arg with the newly addedremove_argmethod.Final Decision
After trying and discussing, we made a final decision.
For
#[derive(Subdiagnostic)], This PR made two changes:Vec<Subdiag>.diag.argsbefore setting arguments, so that you can restore the contents of the main diagnostic after deleting the arguments after subdiagnostic is rendered, to avoid deleting the main diagnostic's arg when they have the same name args.