Skip to content

Add runtime check to avoid overwrite arg in Diag #142724

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xizheyin
Copy link
Contributor

@xizheyin xizheyin commented Jun 19, 2025

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 #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 fluent diagnostics can easily overwrite each others' fields/args #142031 (comment), and after rendering it I manually deleted useless arg with the newly added remove_arg method.

(Some formatting needs to be adjusted though)

r? @oli-obk

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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. labels Jun 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2025

Some changes occurred in match checking

cc @Nadrieril

HIR ty lowering was modified

cc @fmease

Some changes occurred in check-cfg diagnostics

cc @Urgau

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@xizheyin xizheyin force-pushed the avoid_overwrite_args branch from 733b453 to e0afd4a Compare June 19, 2025 18:12
… restore snapshot when set subdiag arg

Signed-off-by: xizheyin <xizheyin@smail.nju.edu.cn>
@xizheyin xizheyin force-pushed the avoid_overwrite_args branch from e0afd4a to faa502e Compare June 21, 2025 16:13
@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Jun 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2025

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Copy link
Contributor Author

@xizheyin xizheyin left a comment

Choose a reason for hiding this comment

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

This time there was a lot less code to modify.

For manual implementation of Subdiagnostic, feel free to call store_args, restore_args, remove_arg...

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For #[derive(Subdiagnostic)], I 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
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants