-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Signed-off-by: xizheyin <xizheyin@smail.nju.edu.cn>
733b453
to
e0afd4a
Compare
diag.remove_arg("fmt"); | ||
diag.remove_arg("trait_name"); |
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.
afaict the derive also eagerly translates, so we could just change the derive to also remove all those args again after the calls
place_name: &'a str, | ||
is_partial: bool, | ||
place_name_method_call: &'a str, | ||
is_partial_method_call: bool, |
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.
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?
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.
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.
I set up a
debug_assert
check for the arg method to make sure thatargs
inDiag
aren'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:
Vec
, the rendering may iteratively write the same arg over and over again. In this case, I changed the auto-generation withderive(SubDiagnostic)
to manually implementingSubDiagnostic
and 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_arg
method.(Some formatting needs to be adjusted though)
r? @oli-obk