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.

Signed-off-by: xizheyin <xizheyin@smail.nju.edu.cn>
@xizheyin xizheyin force-pushed the avoid_overwrite_args branch from 733b453 to e0afd4a Compare June 19, 2025 18:12
Comment on lines +668 to +669
diag.remove_arg("fmt");
diag.remove_arg("trait_name");
Copy link
Contributor

Choose a reason for hiding this comment

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

afaict the derive also eagerly translates, so we could just change the derive to also remove all those args again after the calls

Comment on lines -340 to +361
place_name: &'a str,
is_partial: bool,
place_name_method_call: &'a str,
is_partial_method_call: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

These renames are unfortunate. We do want these to be fairly independently nameable. These are just there because we're not guaranteed the main diagnostic will have these values set, right? The fields are effectively redundant, so we could

  • remove them (and rely on the main diagnostic to set them)
  • change the assert to only prevent overwriting args if the value is different

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it needs to be adjusted.

For the fields that completely overlap with parent diagnostic, I deleted the fields, which will be better. I will rearrange these fields later.

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) 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