Skip to content
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

Make #[diagnostic::on_unimplemented] format string parsing more robust #122402

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

weiznich
Copy link
Contributor

This commit fixes several issues with the format string parsing of the #[diagnostic::on_unimplemented] attribute that were pointed out by @ehuss.
In detail it fixes:

  • Appearing format specifiers (display, etc). For these we generate a warning that the specifier is unsupported. Otherwise we ignore them
  • Positional arguments. For these we generate a warning that positional arguments are unsupported in that location and replace them with the format string equivalent (so {} or {n} where n is the index of the positional argument)
  • Broken format strings with enclosed }. For these we generate a warning about the broken format string and set the emitted message literally to the provided unformatted string
  • Unknown format specifiers. For these we generate an additional warning about the unknown specifier. Otherwise we emit the literal string as message.

This essentially makes those strings behave like format! with the minor difference that we do not generate hard errors but only warnings. After that we continue trying to do something unsuprising (mostly either ignoring the broken parts or falling back to just giving back the literal string as provided).

Fix #122391

r? @compiler-errors

@rustbot rustbot added 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 Mar 12, 2024
@@ -60,3 +66,6 @@ trait_selection_unable_to_construct_constant_value = unable to construct a const

trait_selection_unknown_format_parameter_for_on_unimplemented_attr = there is no parameter `{$argument_name}` on trait `{$trait_name}`
.help = expect either a generic argument name or {"`{Self}`"} as format argument

trait_selection_wrapped_parser_error = {$description}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit unsure about that "hack". That's here to forward the values of ParserError as lint. I'm not sure if there is a better way of doing that. If that's the case I'm happy to change it.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine until these parsing errors become translatable.

Comment on lines +960 to +982
if self.is_diagnostic_namespace_variant && !parser.errors.is_empty() {
String::from(s)
} else {
constructed_message
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems subtle. Please document why we need this.

UNKNOWN_OR_MALFORMED_DIAGNOSTIC_ATTRIBUTES,
tcx.local_def_id_to_hir_id(item_def_id.expect_local()),
self.span,
WrappedParserError { description: e.description, label: e.label },
Copy link
Member

Choose a reason for hiding this comment

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

Kind of a hack, but I guess it's fine

}
}
}
if self.is_diagnostic_namespace_variant {
Copy link
Member

Choose a reason for hiding this comment

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

Please leave a comment here explaining why we need to report these.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we should perhaps do the same as above and just report them as struct_span_err. Seems useful to report even for the rustc_ variant.

@compiler-errors compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 20, 2024
@compiler-errors
Copy link
Member

This I think needs to get backported so it is stabilized at the same time as diagnostic::on_unimplemented.

This commit fixes several issues with the format string parsing of the
`#[diagnostic::on_unimplemented]` attribute that were pointed out by
@ehuss.
In detail it fixes:

* Appearing format specifiers (display, etc). For these we generate a
warning that the specifier is unsupported. Otherwise we ignore them
* Positional arguments. For these we generate a warning that positional
arguments are unsupported in that location and replace them with the
format string equivalent (so `{}` or `{n}` where n is the index of the
positional argument)
* Broken format strings with enclosed }. For these we generate a warning
about the broken format string and set the emitted message literally to
the provided unformatted string
* Unknown format specifiers. For these we generate an additional warning
about the unknown specifier. Otherwise we emit the literal string as
message.

This essentially makes those strings behave like `format!` with the
minor difference that we do not generate hard errors but only warnings.
After that we continue trying to do something unsuprising (mostly either
ignoring the broken parts or falling back to just giving back the
literal string as provided).

Fix rust-lang#122391
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2024

📌 Commit 5568c56 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#122402 (Make `#[diagnostic::on_unimplemented]` format string parsing more robust)
 - rust-lang#122644 (pattern analysis: add a custom test harness)
 - rust-lang#122733 (Strip placeholders from hidden types before remapping generic parameter)
 - rust-lang#122752 (Interpolated cleanups)
 - rust-lang#122771 (add some comments to hir::ModuleItems)
 - rust-lang#122793 (Implement macro-based deref!() syntax for deref patterns)
 - rust-lang#122810 (Remove `target_override`)
 - rust-lang#122827 (Remove unnecessary braces from `bug`/`span_bug`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2e41425 into rust-lang:master Mar 21, 2024
11 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2024
Rollup merge of rust-lang#122402 - weiznich:fix/122391, r=compiler-errors

Make `#[diagnostic::on_unimplemented]` format string parsing more robust

This commit fixes several issues with the format string parsing of the `#[diagnostic::on_unimplemented]` attribute that were pointed out by `@ehuss.`
In detail it fixes:

* Appearing format specifiers (display, etc). For these we generate a warning that the specifier is unsupported. Otherwise we ignore them
* Positional arguments. For these we generate a warning that positional arguments are unsupported in that location and replace them with the format string equivalent (so `{}` or `{n}` where n is the index of the positional argument)
* Broken format strings with enclosed }. For these we generate a warning about the broken format string and set the emitted message literally to the provided unformatted string
* Unknown format specifiers. For these we generate an additional warning about the unknown specifier. Otherwise we emit the literal string as message.

This essentially makes those strings behave like `format!` with the minor difference that we do not generate hard errors but only warnings. After that we continue trying to do something unsuprising (mostly either ignoring the broken parts or falling back to just giving back the literal string as provided).

Fix rust-lang#122391

r? `@compiler-errors`
@weiznich
Copy link
Contributor Author

@compiler-errors What is the usual procedure for the backport? Is there anything that I can do or does this happen "automatically"?

@apiraino
Copy link
Contributor

apiraino commented Mar 24, 2024

Beta backport approved as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

(sorry for the delay)

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 24, 2024
@cuviper cuviper mentioned this pull request Mar 28, 2024
@cuviper cuviper modified the milestones: 1.79.0, 1.78.0 Mar 28, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 28, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2024
[beta] backports

- Do not eat nested expressions' results in `MayContainYieldPoint` format args visitor rust-lang#122680
- Fix heading anchors in doc pages. rust-lang#122693
- Make `#[diagnostic::on_unimplemented]` format string parsing more robust rust-lang#122402

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 29, 2024
[beta] backports

- Do not eat nested expressions' results in `MayContainYieldPoint` format args visitor rust-lang#122680
- Fix heading anchors in doc pages. rust-lang#122693
- Make `#[diagnostic::on_unimplemented]` format string parsing more robust rust-lang#122402
-  Update ninja on Windows rust-lang#123178

r? cuviper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

on_unimplemented format strings either error or warn or ignore various formatting issues
6 participants