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

The #[diagnostic] attribute namespace #3368

Merged
merged 10 commits into from
May 26, 2023
234 changes: 234 additions & 0 deletions text/3366-diagnostic-attribute-namespace.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,234 @@
- Feature Name: The `#[diagnostic]` attribute namespace
- Start Date: 2023-01-06
- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)


# Summary
[summary]: #summary

This RFC proposed to add a stable `#[diagnostic]` attribute namespace, which contains attributes to influence error messages emitted by the compiler. In addition it proposed to add a `#[diagnostic::on_unimplemented]` attribute to influence error messages emitted by unsatisfied traits bounds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This RFC proposed to add a stable `#[diagnostic]` attribute namespace, which contains attributes to influence error messages emitted by the compiler. In addition it proposed to add a `#[diagnostic::on_unimplemented]` attribute to influence error messages emitted by unsatisfied traits bounds.
This RFC proposed to add a stable `#[diagnostic]` attribute namespace, which contains attributes to influence error messages emitted by the compiler. Attributes in this namespace are versioned and crates that use the `#[diagnostic]` attributes must declare a version at the root level (e.g., `#[diagnostic::v1]`). In addition it proposes to add a `#[diagnostic::on_unimplemented]` attribute to influence error messages emitted by unsatisfied traits bounds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, that's something I've missed after updating the RFC to incorporate a version mechanism. Should I push an update now or should I wait for the proposed design meeting to happen?


# Motivation
[motivation]: #motivation

Rust has the reputation to generate helpful error messages when something goes wrong. Nevertheless there are always cases of error messages that can be improved. One common example of such error messages in the rust ecosystem are those that are generated by crates using the type system to verify certain invariants at compile time. While these crates provide additional guarantees about these invariants, they sometimes generate large incomprehensible error messages when something goes wrong. These error messages do not always indicate clearly what went wrong. Well known examples of crates with such issues include bevy, axum or diesel. In some situations such a specific error message always indicates a certain problem. By providing authors of such crates tools to control the error messages emitted by the compiler they can improve the situation on their own.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

This feature has two possible groups of users:

* Users that develop code and consume error messages from the compiler
* Users that write crates involving complex type hierarchies

The first user group will interact with the proposed feature through the error messages emitted by the compiler. As of this I do not expect any major documentation requirements for this group of users. Although we might want to indicate that a certain error message was provided via the described feature set, rather than by the compiler itself to prevent users for filling issues about bad error messages in the compilers issue tracker.

The second user group interacts with the described feature through attributes. These attributes allow them to hint the compiler to emit specific error messages in certain cases. The `#[diagnostic]` attribute namespace provides a general framework for what can and can't be done by such an attribute. As of this users won't interact directly with the attribute namespace itself.

The `#[diagnostic::on_unimplemented]` attribute allows to hint the compiler to emit a specific error message if a certain trait is not implemented. This attribute should provide the following interface:

```rust
#[diagnostic::on_unimplemented(
message="message",
label="label",
note="note"
)]
trait MyIterator<A> {
fn next(&mut self) -> A;
}


fn iterate_chars<I: MyIterator<char>>(i: I) {
// ...
}
fn main() {
iterate_chars(&[1, 2, 3][..]);
}
```

which might result in the compiler emitting the following error message:

```
error[E0277]: message
--> <anon>:14:5
|
14 | iterate_chars(&[1, 2, 3][..]);
| ^^^^^^^^^^^^^ label
|
= note: note
= help: the trait `MyIterator<char>` is not implemented for `&[{integer}]`
= note: required by `iterate_chars`
```

I expect the new attributes to be documented on the existing [Diagnostics](https://doc.rust-lang.org/reference/attributes/diagnostics.html) attributes page on the rust reference similar to existing attributes like for example `#[deprecated]`


# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

## The `#[diagnostic]` attribute namespace

This RFC proposes to introduce a new `#[diagnostic]` attribute namespace. This namespace is supposed to contain different attributes, which allow users to hint the compiler to emit specific error messages in certain cases like type mismatches, unsatisfied trait bounds or similar situations. By collecting such attributes in a common namespace it is easier for users to find useful attributes and it is easier for the language team to establish a set of common rules for these attributes.

Attributes in this namespace are generally expected to be formed like:
```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.
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :-/

Copy link
Contributor

Choose a reason for hiding this comment

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

could hygiene pick the macro crate's attribute version instead of the user crate's version? Regular macros would have the same issue.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


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
Copy link
Member

Choose a reason for hiding this comment

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

It seems like warnings should be fine to support from this namespace too.


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
* pass-through information from the source of the diagnostic in a way that users can rely on. E.g. Such an attribute should not allow users to keep the compilation successful and dump information about `extern` blocks to generate C header files

The compiler is allowed to:

* Ignore the hints provided by:
+ A specific attribute
+ A specific option
* Change the set of supported attributes and options at any time
* Provide warning lints for malformed or otherwise not accepted attributes

The compiler must not:

* 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:
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 .


1. The new attribute lands on nightly without a feature gate. It gets ignored on stable/beta until its explicitly activated there.
2. The time as nightly only attribute is used to verify that:
* The attribute solves a real problem
* The implemented syntax matches the expectations
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Leave a comment saying that the usual concern rules still apply, everyone can raise a concern.



## The `#[diagnostic::on_unimplemented]` attribute

This section describes the syntax of the `on_unimplemented` attribute and additionally how it is supposed to work. The specification of this attribute is partially provided as example and motivation for the `#[diagnostic]` attribute namespace. In addition it is provided to give this RFC a concrete use, such that we not only define an empty attribute namespace.

```rust
#[diagnostic::on_unimplemented(
message="message",
label="label",
note="note",
)]
trait MyIterator<A> {
fn next(&mut self) -> A;
}
```


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. 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 of the generic parameter name from the trait definition and a type path against which the parameter should be checked. This 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`.

The `any` and `all` options allow 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. 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),
Copy link
Member

Choose a reason for hiding this comment

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

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

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",
)]
Comment on lines +132 to +143
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

// this attribute will not have any affect as
// the attribute above will always match before
#[diagnostic::on_unimplemented(
if(all(A = i32, Self = i32)),
note = "That's emitted if A and Self is a i32"
)]
#[diagnostic::on_unimplemented(
if(not(A = String)),
// and implicitly `A` is not a `i32` as that case is
// matched earlier
note = "That's emitted if A is not a `String`"
)]
#[diagnostic::on_unimplemented(
message="message",
label="label",
note="That's emitted if neither of the condition above are meet",
)]
trait MyIterator<A> {
fn next(&mut self) -> A;
}
```

# Drawbacks
[drawbacks]: #drawbacks

A possible drawback is that this feature adds additional complexity to the compiler implementation. The compiler needs to handle an additional attribute namespace with at least one additional attribute.

Another drawback is that crates might hint lower quality error messages than the compiler itself. Technically the compiler would be free to ignore such hints, practically I would assume that it is impossible to judge the quality of such error messages in an automated way.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

This proposal tries to improve error messages generated by rustc. It would give crate authors a tool to influence what error message is emitted in a certain situation, as they might sometimes want to provide specific details on certain error conditions. Not implementing this proposal would result in the current status quo. Currently the compiler always shows a "general" error message, even if it would be helpful to show additional details.

There are alternatives for the naming of the `#[diagnostic]` namespace:

* Use a `#[rustc]` namespace for these attributes. This would signifies that these are rustc specific extensions. We likely want to encourage other rust implementations to utilise these information as well, therefore a more general attribute namespace seems to be a better solution.

There are alternative designs for the proposed `on_unimplemented` attribute:

* The `on()` based filtering might be replaceable by placing the attribute on negative trait impls. This would turn a filter like
Copy link
Contributor

@lcnr lcnr May 9, 2023

Choose a reason for hiding this comment

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

on() has not been defined in the above description of on_unimplemented? it seems like the above description changed that to if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. That should be address in cb8f2fc

```rust
#[diagnostic::on_unimplemented(
on(Self = `String`, message = "Strings do not implement `IntoIterator` directly")
)]
trait IntoIterator {}
```

into the following negative trait impl:
```rust
#[diagnostic::on_unimplemented(message = "Strings do not implement `IntoIterator` directly")]
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. To express complex filtering logic this would likely need some sort of `specialization` for at least negative trait implementations. A second 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.


# Prior art
[prior-art]: #prior-art

* [rustc_on_unimplemented](https://rustc-dev-guide.rust-lang.org/diagnostics.html#rustc_on_unimplemented) already provides the described functionality as rustc internal attribute. It is used for improving error messages for various standard library API's. [This repo](https://github.com/weiznich/rust-foundation-community-grant/) contains several examples on how this attribute can be used in external crates to improve their error messages.
* [GHC](https://ghc.gitlab.haskell.org/ghc/doc/users_guide/exts/type_errors.html) provides a Haskell mechanism for specifying custom compile time errors

Notably all of the listed similar features are unofficial language extensions.

# Unresolved questions
[unresolved-questions]: #unresolved-questions

Clarify the procedure of various potential changes prior stabilisation of the attribute namespace:

* Exact syntax of the `on_unimplemented` attribute
+ Can `Self` be accepted in that position or do we need another name?
+ Is `if()` a valid identifier in the proposed position?

# Future possibilities
[future-possibilities]: #future-possibilities

* More attributes like `#[diagnostics::on_type_error]`
* Extend the `#[diagnostics::on_unimplemented]` attribute to incorporate the semantics of `#[do_not_recommend]` or
provide a distinct `#[diagnostics::do_not_recommend]` attribute
* Un-RFC `#[do_not_recommend]`?
* Apply `#[diagnostics::on_unimplemented]` to types as well
* Extend the `if()` filter syntax to allow more complex filter expressions
* 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.