The #[diagnostic] attribute namespace
#3368
Conversation
|
Seems that the RFC is misleadingly named, since the bulk of it is the design of With regards to the |
ehuss
added
T-lang
I'm open to change the title of the RFC to something better fitting. Feel free to provide suggestions. Otherwise I do not feel that the design of the namespace is only tangentially touched. In my opinion this RFC tries to specify a set of common rules for this specific namespace. I even feel that this is more important than the exact design of the
The current design talks about new attributes only. It does not specify what should happen with these older attributes. I feel that's something that can be decided later on. That written: Both
That's a totally reasonable concern. Unfortunately there seems to be no way around to somehow specify the corresponding filter. The only possible workaround is to use a syntax that is already used in other places. The proposed syntax is at least very similar to the
The RFC already links my experiments with the |
|
Sidenote: I would expect |
|
|
||
| Each of the options `message`, `label` and `note` are optional. They are separated by comma. The tailing comma is optional. Specifying any of these options hints the compiler to replace the normally emitted part of the error message with the provided string. At least one of these options needs to exist. Each option can appear at most once. The error message can include type information for the `Self` type or any generic type by using `{Self}` or `{A}` (where `A` refers to the generic type name in the definition). These placeholders are replaced by the actual type name. | ||
|
|
||
| In addition the `on_unimplemented` attribute provides mechanisms to specify for which exact types a certain message should be emitted via an `if()` option. It accepts a set of filter options. A filter option consists on the generic parameter name from the trait definition and a type path against which the parameter should be checked. These type path could either be a fully qualified path or refer to any type in the current scope. As a special generic parameter name `Self` is added to refer to the `Self` type of the trait implementation. A filter option evaluates to `true` if the corresponding generic parameter in the trait definition matches the specified type. The provided `message`/`note`/`label` options are only emitted if the filter operation evaluates to `true`. |
There was a problem hiding this comment.
Is there a reason to change on to if from rustc_on_unimplemented? I don't care either way, I'm just intrigued at the rationale. on is already used in other attributes IIRC.
Also, allowing Self is something I'd want and is easy to add, but might have pushback from the lang team.
There was a problem hiding this comment.
I've went for if instead of on as I also moved the message/notice/label option out of the filter clause. Instead each attribute now has an optional filter in the if clause that determines whether a message/… should be emitted. I consider the multiple attributes approach to be easier to use in conjunction with something like #[cfg_attr] especially around filters containing types behind a feature flag. So if feels for fitting for my non-native speaker mind. If on is a better fit I'm happy to use on instead.
|
|
||
| The `any` and `all` option allows to combine multiple filter options. The `any` option matches if one of the supplied filter options evaluates to `true`, the `all` option requires that all supplied filter options evaluate to true. `not` allows to negate a given filter option. It evaluates to `true` if the inner filter option evaluates to `false`. These options can be nested to construct complex filters. | ||
|
|
||
| The `on_unimplemented` attribute can be applied multiple times to the same trait definition. Multiple attributes are evaluated in order. The first matching instance for each of the `message`/`label`/`note` options is emitted. |
There was a problem hiding this comment.
I feel like the bulk of the on_unimplemented behavior should be on its own RFC, because there are open questions about it. What happens when you have multiple cases that match and the attribute writer doesn't notice? Is that detected by the compiler and emits an error?
There was a problem hiding this comment.
Yeah, the discussion of on_implemented is so involved that it looks like it's proposed to be stabilized in this RFC. If it isn't, it needs to be trimmed down from irrelevant details and more prominently marked as just an example.
There was a problem hiding this comment.
I've opted to put everything into one RFC for now as the proposed #[diagnostic] attribute namespace is only useful with actual attributes that can be used by someone. So yes: This RFC proposes both, the namespace and the #[on_unimplemented] attribute. If it turns out that there is no conses around how the filter rules should be designed I would propose to only specify the #[on_unimplemented] attribute without the filtering functionality. So just the message/label/note options. I would expect that it is much easier to get these options approved.
There was a problem hiding this comment.
I think that's a reasonable approach: land #[diagnostics::on_unimplemented(note="", message="", label="")] with this RFC and a subsequent RFC for the evolution of on_unimplemented. This would also (I think) help t-lang to experience what evolving these would look like in the future and catch any potential problems or concerns with a concrete example.
There was a problem hiding this comment.
I would nevertheless wait for t-lang before removing the relevant RFC section.This would at least give some initial feedback on what needs to be addressed in a future RFC.
| impl !IntoIterator for String {} | ||
| ``` | ||
|
|
||
| This would simplify the syntax of the proposed attribute, but in turn block the implementation of type based filtering on the stabilization of `negative_impls`. On the other hand it would likely simplify writing more complex filters, that match only a certain generic set of types and it would prevent "duplicating" the filter-logic as this reuses the exiting trait system. The large disadvantage of this approach is that it couples error messages to the crates public API. Removing a negative trait impl is a breaking change, removing a `#[on_unimplemented]` attribute is only a change in the emitted compiler error. |
There was a problem hiding this comment.
You need to call out that the simplification of complex filtering logic also requires some level of specialization support. This would be fine because even if the current impl is unsound it doesn't affect this use case, but it might close doors and block future specialization behavior changes.
There was a problem hiding this comment.
That's a good point. Will add that 👍
There was a problem hiding this comment.
Another thing is that doing the attribute approach now doesn't preclude us from implementing a more integrated system in the future.
|
|
||
| This would simplify the syntax of the proposed attribute, but in turn block the implementation of type based filtering on the stabilization of `negative_impls`. On the other hand it would likely simplify writing more complex filters, that match only a certain generic set of types and it would prevent "duplicating" the filter-logic as this reuses the exiting trait system. The large disadvantage of this approach is that it couples error messages to the crates public API. Removing a negative trait impl is a breaking change, removing a `#[on_unimplemented]` attribute is only a change in the emitted compiler error. | ||
|
|
||
| * Allow `#[diagnostic::on_unimplemented]` to be placed on types instead of traits. This would allow third party crates to customize the error messages emitted for unsatisfied trait bounds with out of crate traits. This feels more like an extension tho the proposed attribute. |
There was a problem hiding this comment.
I don't see this as an alternative but more as future work.
There was a problem hiding this comment.
I'd love to have both (eventually). Current #[rustc_on_unimplemented] works well mostly because std is mostly defining the traits, not using traits defined by others.
There was a problem hiding this comment.
Will move that point to future possibilities 👍
| Any attribute in this namespace is not allowed to: | ||
|
|
||
| * Change the result of the compilation, which means applying such an attribute should never cause an error as long as they are syntactically valid | ||
|
|
There was a problem hiding this comment.
Does this promise that "new" syntax in the attribute will compile (but perhaps do nothing) backwards compatibly?
For example,
#[cfg_attr(FALSE, doc = include!(""))]
struct Blah;...does nothing, in any compiler version, but does not compile below Rust 1.54. (it gives an unstable error from 1.50, and a syntax error before that).
Personally I don't care much about error messages on non-latest rust, and would happily use new attributes and arguments as they become available. But I would dislike new syntax incompatible with my msrv, because I would have to resort to cfg_attr tricks to disable it.
In other words, if this namespace stabilizes in a given Rust version, is it guaranteed that it will remain syntactically valid for that version?
There was a problem hiding this comment.
I believe what's proposed is only forwards compatibility and not backwards compatibility: If diagnostics::on_type_error is stabilized in 1.N, using it in 1.(N-1) will result in a compilation error. The same for any extensions of the API of diagnostics::on_unimplemented. I would personally prefer an approach that is more lenient, but even if the language won't do that, once the feature is stabilized a third party crate can act as the glue to accomplish that, by doing version checking and translating from the provided proc macro to the supported API surface (including doing nothing).
There was a problem hiding this comment.
Yes, the proposed implementation would only allow forward compatibility. That's because as far as I understood the feedback from @joshtriplett in the various discussions that's the only way the language team would consider something like that. (Allowing backward compatibility would require not rejecting invalid syntax at all)
There was a problem hiding this comment.
@weiznich I'm happy enough with that solution because we can then provide a crate for those who care about lower MSRV, in the same spirit as standback.
| * Ignore the provided hints | ||
| * Use only parts of the provided hints, for example for the proposed `#[diagnostic::on_unimplemented]` only use the `message` option | ||
| * Change this behaviour at any time |
There was a problem hiding this comment.
I am happy with api evolution, but I'd like to be aware if/when this happens. I don't expect it to actually change much (after all, rustc_on_unimplemented has barely changed backwards compatibly), but I am a bit afraid of having a lot of these attributes in my codebase, only for future me to have that "oh this hasn't actually worked for a long time" moment.
Perhaps a lint diagnostic should be emitted for that?
There was a problem hiding this comment.
rustc_on_unimplemented hasn't ever broken backwards compatibility, somehow 😅
Perhaps a lint diagnostic should be emitted for that?
+1
There was a problem hiding this comment.
I personally would consider that a quality of implementation issue. Which means that's something where a specific compiler implementation can offer a lint for. Other than that: At least for my own use-cases I will likely have tests based on trybuild or something else. These will show whether the attributes are ignored or not.
| #[diagnostic::on_unimplemented( | ||
| if(Self = std::string::String), | ||
| note = "That's only emitted if Self == std::string::String" | ||
| )] | ||
| #[diagnostic::on_unimplemented( | ||
| if(A = String), // Refers to whatever `String` is in the current scope | ||
| note = "That's only emitted if A == String", | ||
| )] | ||
| #[diagnostic::on_unimplemented( | ||
| if(any(A = i32, Self = i32)), | ||
| note = "That's emitted if A or Self is a i32", | ||
| )] |
There was a problem hiding this comment.
Should this check whether a type has the name String or whether it is String? #[rustc_on_unimplemented] blindly checks the name of the type, but "Refers to whatever String is in the current scope" sounds like you want the latter.
There was a problem hiding this comment.
I would anticipate that on_unimplemented would receive the DefId of whatever path was passed in here, so it would be following regular visibility rules.
There was a problem hiding this comment.
I propose to use the regular visibility rules here instead of just matching the name as string as that's currently done. I feel that aligns the proposed attribute more with other parts of rust.
Co-authored-by: konsumlamm <[email protected]>
joshtriplett
added
the
I-lang-nominated
|
How will localization be implemented ? The compiler messages may get there one day (I hope), but this RFC doesn't offer a way to translate messages. |
Localization is definitively an important topic, but there is as far as I'm aware no RFC yet that specifies something about the localization requirements of the language itself. There is currently a rustc specific project to localize the error messages. I would say that's something different than having that at language level. Given that current situation I would answer this question with: Whatever an upcoming localization RFC specifies to be done for |
|
I don't think the The failure mode is "worse diagnostics", nothing else. And diagnostics have always been entirely the compiler team's concern. If every change to those attributes needs language team oversight, this may cause a lot of churn and friction with the volume, details and back-and-forth. I think we should avoid that. TLDR: imo this RFC should just add the new namespace and leave its contents to the compiler team's discretion. |
|
When an item has multiple |
semantics of the `#[diagnostic]` attribute namespace
regarding options and compiler warnings.
|
I've pushed an update to the PR. This mostly incorporates suggestions made by @joshtriplett regarding the semantics of the
That's already specified in the RFC:
So the first matching one wins, for each option. If the first only contains a I added an explicit statement that the compiler can lint cases where attributes are not applied due to the ordering. For example similarly as this is already handled for |
| 3. To enable a certain attribute in stable release the following steps are required: | ||
| * The attribute is documented in the rust reference | ||
| * T-lang is informed | ||
| * T-compiler performs a FCP, where a simple majority (>50%) accepts the attribute |
There was a problem hiding this comment.
Leave a comment saying that the usual concern rules still apply, everyone can raise a concern.
joshtriplett
removed
the
I-lang-nominated
| ```rust | ||
| #[diagnostic::attribute(option)] | ||
| ``` | ||
| where several `option` entries can appear in the same attribute. `option` is expected to be a valid attribute argument in this position. Attributes in this namespace are versioned. Any incompatible change to an existing stable attribute in this namespace requires bumping the version of the diagnostic attribute namespace. Crates using these attributes must specify the expected version of the diagnostic namespace via a `#![diagnostic::v{version_number}]` crate top-level attribute. If multiple such attributes are present the compiler will choose the highest version number and ignore the rest. The compiler is encouraged to provide a lint for outdated versions of the diagnostic attribute namespace. This mechanism exists so that a third party crate can provide a shim implementation of the corresponding attributes to translate between different versions of the diagnostic attribute namespace on different rust versions. |
There was a problem hiding this comment.
Today I was thinking about how to design a macro api, and I realized a (niche) downside of this versioning approach. It won't work (out of the box) when I want to insert it in my user's code and I own neither the trait nor the struct.
For example, say an user writes the following code:
#[make_secure] // my macro!
struct Password(String);I'd really like to make this expand to:
struct Password(String);
// imagine a world where negative_impls is stable
#[diagnostic::on_unimplemented(message = "secure structs may not be printed")]
impl !Debug for Password {}Is there a good workaround for this? Or will this cause the compiler to throw a "please enable diagnostics version 12 for diagnostics on negative impls" error to my user?
There was a problem hiding this comment.
I think that per attr versioning selection (a convention of accepting a specific field?) would need to be provided to cater to attribute proc macros, as you correctly point out :-/
There was a problem hiding this comment.
could hygiene pick the macro crate's attribute version instead of the user crate's version? Regular macros would have the same issue.
| Any attribute in this namespace may: | ||
|
|
||
| * Hint the compiler to emit a specific error message in a specific situation | ||
| * Only affect the messages emitted by a compiler in case of a failed compilation |
There was a problem hiding this comment.
It seems like warnings should be fine to support from this namespace too.
| * Change the semantic of an attribute or option, these kind of change requires a bump of the diagnostic attribute space version number | ||
| * Emit an hard error on malformed attribute, although emitting lints is fine | ||
|
|
||
| Adding a new attribute or option to the `#[diagnostic]` namespace is a decision of the compiler team. The envisioned procedure for this is as following: |
There was a problem hiding this comment.
Would the new attribute option be in the language reference/spec? Is the compiler team comfortable reasoning about how this might affect other (maybe hypothetical) compilers?
Put another way, is the justification for bypassing T-lang approval essentially that: it doesn't affect semantics of programs, including whether they compile, and that alternate compilers can ignore it?
It seems partially justified also by the alternative versioning scheme ("we'll just bump the version if we mess something up"), which I have concerns about (comment below).
There was a problem hiding this comment.
Put another way, is the justification for bypassing T-lang approval essentially that: it doesn't affect semantics of programs, including whether they compile, and that alternate compilers can ignore it?
rustc can ignore them, they are effectively a new kind of comments from the lang perspective, even if we as compiler developers will want to give users a reliable experience.
There was a problem hiding this comment.
The original proposal (before opening this RFC) was to introduce a #[rustc::] attribute namespace. That would make it much clearer that this these attributes are compiler specific. Some members of the compiler team and the language team raised the valid concern that these specific attributes around diagnostics might be relevant for other compilers as well. So a different named attribute namespace seems to be more fitting.
Now this RFC tries to specify the namespace in such a way that supporting anything in that namespace should be optional for other compilers. So if these attributes do not fit their model, they are free to ignore these attributes (or ignore certain options only). Otherwise the main argument for "bypassing" approval from the language team is to prevent spamming the team with otherwise trivial changes. Again that was something that was proposed by @joshtriplett .
| The `on_unimplemented` attribute can be applied multiple times to the same trait definition. Multiple attributes are evaluated in order. The first matching instance for each of the `message`/`label`/`note` options is emitted. The compiler may lint against ignored variants as this is the case for `match` arms for example. | ||
| ```rust | ||
| #[diagnostic::on_unimplemented( | ||
| if(Self = std::string::String), |
There was a problem hiding this comment.
If we ever had where clause equality syntax, do we think it would look like this? Does anyone in @rust-lang/lang know?
I mostly want there to be sharing of existing work on possible syntax ideas, for a higher probability of convergence in the long term. I especially don't want to block the RFC on finalizing syntax for a nonexistent feature ;)
| ```rust | ||
| #[diagnostic::attribute(option)] | ||
| ``` | ||
| where several `option` entries can appear in the same attribute. `option` is expected to be a valid attribute argument in this position. Attributes in this namespace are versioned. Any incompatible change to an existing stable attribute in this namespace requires bumping the version of the diagnostic attribute namespace. Crates using these attributes must specify the expected version of the diagnostic namespace via a `#![diagnostic::v{version_number}]` crate top-level attribute. If multiple such attributes are present the compiler will choose the highest version number and ignore the rest. The compiler is encouraged to provide a lint for outdated versions of the diagnostic attribute namespace. This mechanism exists so that a third party crate can provide a shim implementation of the corresponding attributes to translate between different versions of the diagnostic attribute namespace on different rust versions. |
There was a problem hiding this comment.
I don't think I'm comfortable with adding a new versioning scheme for a subset of the language. Or at least, I don't understand the motivation at all. Why can't we use editions to evolve the attributes?
This mechanism exists so that a third party crate can provide a shim implementation of the corresponding attributes to translate between different versions of the diagnostic attribute namespace on different rust versions.
What purpose does this serve? Supporting newer versions on old compilers? How would it work?
There was a problem hiding this comment.
Why can't we use editions to evolve the attributes?
because we want to remove old versions from the compiler entirely. Editions must be supported until the heat death of the universe.
What purpose does this serve? Supporting newer versions on old compilers? How would it work?
a crates.io crate could check the compiler version and change what attribute they generate, even if the crate's input attribute syntax is stable across compiler versions.
There was a problem hiding this comment.
For context: This versioning scheme was proposed by @joshtriplett. I personally would be fine with specifying that the compiler is not allowed to change the semantic of a certain attribute/option + it is allowed to ignore a certain attribute/option. If necessary that alone would allow to replace the a certain option by another one by just choosing a differently named option.
namespace This hopefully addresses both concerns raised by petrochenkov: > Does #[diagnostic::something] behave like #[rustfmt::skip] from name resolution / macro expansion point of view? By specifying that `#[diagnostic]` is a built-in tool attribute > How many of these attributes will exist - 2, 3, 5? > Why is it necessary to add a mechanism for grouping them instead of just using built-in attributes? By being explicit about that this namespace also serves an organizational need by defining a common set of rules. This hopefully allows the language team to delegate the decisions about the design of specific attributes to other teams.
|
@petrochenkov I've pushed 0d519ab to hopefully address your concerns. |
|
Concern: equality syntax (already noted above but apparently ignored) This RFC introduces syntax like this:
Apparently the The only other occurrence of type equality operators I can think of is in where clauses (as yet unsupported): rust-lang/rust#20041 |
|
@dhardy Thanks for bringing that up again. You might want to register your concern with rust-bot? I think it might already be resolved in that comment by a language team member stating that:
(Highlighting by me) Otherwise this RFC is structured in such a way that we always can change the syntax of an diagnostic attribute later if it turns out to be wrong for whatever reason. |
|
|
||
| The compiler must not: | ||
|
|
||
| * Change the semantic of an attribute or option |
There was a problem hiding this comment.
What does "Change the semantic of an attribute or option" mean in this context (namely, the context of "The compiler must not:")
I.e., from the rest of this text, it seems like these attribute are limited in scope such that they 1. are always just hints and 2. cannot do anything except replace (or add) some diagnostic output.
So what does it mean to say that the compiler cannot change those above semantics? Does it mean that the compiler cannot decide on its own to change what trait the diagnostic applies to? Or is it just a restatement that the compiler cannot decide to make the attribute start having effects that go beyond the limits specified by 1. and 2. above?
There was a problem hiding this comment.
Its for supposed to prevent that compiler starts at to emit what's passed as message option as label or that the compiler starts at one point to use the on_unimplemeted attribute for type mismatches (however that would be possible...)
Maybe I should add these cases as example to the RFC as well?
| ``` | ||
|
|
||
|
|
||
| Each of the options `message`, `label` and `note` are optional. They are separated by comma. The trailing comma is optional. Specifying any of these options hints the compiler to replace the normally emitted part of the error message with the provided string. At least one of these options needs to exist. Each option can appear at most once. These options can include type information for the `Self` type or any generic type by using `{Self}` or `{A}` (where `A` refers to the generic type name in the definition). These placeholders are replaced by the compiler with the actual type name. |
There was a problem hiding this comment.
I understand the idea of replacing the message and the label, in the sense that I think there is always a message, and there is one label (potentially at most one label? Can the label be omitted? Is this inserted in its stead in that case?). So replacing each of those is the sensible option here.
However, a given diagnostic can have zero, one, or more than one notes supplied, right?. What is replaced for the "more than one" case, here? And would it be better to just accumulate this note onto the set, or are we indeed better off sticking with replacement here?
There was a problem hiding this comment.
What the best approach is depends entirely on the situation, sadly (which tells me it should be configurable somehow). Currently rustc_on_unimplemented will replace these but add a note with the original message.
I can see for example errors that would be very verbose where the underlying problem is very simple, so as a crate author I'd like to be able to almost entirely clear the diagnostic and clearly state "this is wrong and this is how you fix it", avoiding that messaging being lost in the noise. That being said there would have to be a consideration of whether this kind of "obscuring" of the emitted error could constitute a problem if misused. All this being said, I believe these are specific questions to be had around the stabilization of #[diagnostic::on_unimplemented] and not the acceptance of #[diagnostic::*] as a construct.
|
(i've posted some feedback but have no blocking concerns to add, so I've checked off my box.) |
rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
Well, the |
|
|
||
| Any attribute in this namespace is not allowed to: | ||
|
|
||
| * Change the result of the compilation, which means applying such an attribute should never cause a compilation error as long as they are syntactically valid |
There was a problem hiding this comment.
Should this say “applying or removing such an attribute should never cause a compilation error”?
There was a problem hiding this comment.
I would argue that this is already somewhat implied by the first part of the sentence (cannot change the result of the compilation). That written, it might be meaningful to add it explicitly to the text as well.
|
@dhardy I'm not sure if this RFC is the correct place to decide on the syntax of the Other than that: The proposed filtering syntax tries to be as close as possible to the What's about adding this concern as unresolved question (There are already some syntax questions there). |
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
…rochenkov Diagnostic namespace This PR implements the basic infrastructure for accepting the `#[diagnostic]` attribute tool namespace as specified in rust-lang/rfcs#3368. Note: This RFC is not merged yet, but it seems like it will be accepted soon. I open this PR early on to get feedback on the actual implementation as soon as possible. This hopefully enables getting at least the diagnostic namespace to stable rust "soon", so that crates do not need to bump their MSRV if we stabilize actual attributes in this namespace. This PR only adds infrastructure accept attributes from this namespace, it does not add any specific attribute. Therefore the compiler will emit a lint warning for each attribute that's actually used. This namespace is added behind a feature flag, so it will be only available on a nightly compiler for now. cc `@estebank` as they've supported me in planing, specifying and implementing this feature.
Diagnostic namespace This PR implements the basic infrastructure for accepting the `#[diagnostic]` attribute tool namespace as specified in rust-lang/rfcs#3368. Note: This RFC is not merged yet, but it seems like it will be accepted soon. I open this PR early on to get feedback on the actual implementation as soon as possible. This hopefully enables getting at least the diagnostic namespace to stable rust "soon", so that crates do not need to bump their MSRV if we stabilize actual attributes in this namespace. This PR only adds infrastructure accept attributes from this namespace, it does not add any specific attribute. Therefore the compiler will emit a lint warning for each attribute that's actually used. This namespace is added behind a feature flag, so it will be only available on a nightly compiler for now. cc `@estebank` as they've supported me in planing, specifying and implementing this feature.
Summary
This RFC proposes to add a common
#[diagnostic]attribute namespace for attributes that can influence the error messages emitted by the compiler. It specifies a set of rules what attributes in this namespace are allowed to do, how these attributes must be handled by the compiler and what is disallowed in this namespace.In addition this RFC proposes a
#[diagnostic::on_unimplemented]attribute to influence error messages emitted by unsatisfied traits bounds.I would like to use this possibility to thank the rust foundation for supporting my work on this RFC through a project grant.
Rendered