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

on_unimplemented format strings either error or warn or ignore various formatting issues #122391

Closed
ehuss opened this issue Mar 12, 2024 · 2 comments · Fixed by #122402
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information.

Comments

@ehuss
Copy link
Contributor

ehuss commented Mar 12, 2024

The following are some unexpected behaviors I see with the formatting strings in the diagnostic::on_unimplemented attribute. I'm not sure that the on_unimplemented attribute should actually be using std's format parsing, but if it is, the following seem like issues with the current implementation.

Broken formatting is ignored

#[diagnostic::on_unimplemented(message = "Test } thing")]
trait ImportantTrait<A> {}

Anything after the } is ignored, and not included in the message output.

Expectation: There should be a warning that the string is malformed.

Empty or positional arguments cause an error

#[diagnostic::on_unimplemented(message = "Test {}")]
trait ImportantTrait<A> {}

or

#[diagnostic::on_unimplemented(message = "Test {1:}")]
trait ImportantTrait<A> {}

These generate an error. However, my understanding of the attribute is that it tries to only generate warnings for things it doesn't understand. I would expect this to also generate a warning.

Format options are ignored

#[diagnostic::on_unimplemented(message = "Test {Self:123}")]
trait ImportantTrait<A> {}

This does not generate a warning or error. I would expect it to generate a warning that anything after the : is ignored. This includes width, precision, debug formatting, etc.

Invalid format options do weird things

#[diagnostic::on_unimplemented(message = "Test {Self:!}")]
trait ImportantTrait<A> {}

The error message generated with this is the type followed by a !. I would expect this to generate a warning that the format arguments are not valid (similar to how format! works).

Meta

rustc --version --verbose:

rustc 1.78.0-nightly (4a0cc881d 2024-03-11)
binary: rustc
commit-hash: 4a0cc881dcc4d800f10672747f61a94377ff6662
commit-date: 2024-03-11
host: aarch64-apple-darwin
release: 1.78.0-nightly
LLVM version: 18.1.0

cc @weiznich

@ehuss ehuss added the C-bug Category: This is a bug. label Mar 12, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 12, 2024
weiznich added a commit to weiznich/rust that referenced this issue Mar 12, 2024
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
@weiznich
Copy link
Contributor

I filled #122402 to hopefully address this.

@RalfJung
Copy link
Member

Good catch! This should probably support a strict subset of std::fmt formatting (only named arguments and no format options), and warn whenever the string is either not valid for std::fmt or not in the supported subset. When it shows a warning, not sure what the behavior of the string should be -- maybe just take it as a literal without any interpolation?

@jieyouxu jieyouxu added D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. A-diagnostics Area: Messages for errors, warnings, and lints and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 12, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 21, 2024
…rors

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`
@bors bors closed this as completed in 5568c56 Mar 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue 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`
cuviper pushed a commit to cuviper/rust that referenced this issue Mar 28, 2024
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

(cherry picked from commit 5568c56)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants