Stable diagnostic affecting attribute with unstable API

For a while now I've been thinking about how to bring the features of on_unimplemented to stable Rust. There are many problems with its API surface, which make it unsuitable for stabilization as-is, and we freely expand it or change it when needed.

An idea I've had for these kind of attributes, that do not affect the final compilation but act as a hint to the diagnostics machinery, is to provide the attribute as insta-stable, but with no stable assurances of what API it actually conforms to or what the behavior they will have on the output actually is. This would act as a "get out of jail free" card, letting us evolve the API and output, even potentially deprecating old behavior freely and the only consequence would be potentially the same worse diagnostics that you'd get without the feature in the first place.

The idea is the attribute fully consumes the user provided input, acting as a permanent black hole as far as the compiler is concerned for successful compilations, but opportunistically using this input to modify the messaging in diagnostics.

I think this would be an acceptable way of bringing crate authors facilities to improve the UX of working with their libraries, without having to wait a really long time to get the functionality.

Thoughts?

4 Likes

As a library maintainer, this sounds acceptable to me. I would hope that new users trying out the library would be using recent library and compiler versions. We could even recommend it. Enhanced error messages not working on older builds seems the correct trade-off.

I don't actually have compiler experience with on_unimplemented, so can't comment on its semantics.

1 Like

I guess bad syntax in it could just be a deny-by-default lint, so people still fix it, but cap-lints lets it through?

Full anything seems surprising, compared to something more restricted like "more key values are ok", but maybe the practical difference is immaterial.

On on_unimplemented thought in particular: maybe some parts could move to being based on negative impls? Moving all the for-a-particular-type cases to those types instead of needing them on the trait itself might help avoid the 1000-line attribute problem it can sometimes hit today.

Or, relatedly, maybe attributes on functions for what to say if a parameter doesn't meet a trait bound?

3 Likes

I don't think there any case leveraging this, but I think #[rustc_on_unimplemented(on(not(_Self="()", label = "this is shown on everything but ()")))] would work today for that.

Yeah, relying on some things like specialization and special syntax, like

impl Deref for Type {
    type target = Ot;
    fn deref(&self) -> &Ot {
        compile_error!("any invocation of this method will result in a custom compile error")
    }
}

would be sweet, but I don't know how hard it would be to make it part of the language.

I would love that.

What I mean here is that, for example, we have this already:

So it would be really cool if, rather than doing something like

#[rustc_on_unimplemented(
    _Self = "&mut _",
    label = "Shared references can be cloned, but mutable references *cannot*!",
]
pub trait Clone { ... }

The error message could just pull the documentation off the negative impl.

And that way these hints would be shown in rustdocs too, where they could have extra things like examples of how to do things using different stuff even though the specific implementation isn't available.

I'm sure there are situations wanting more filtering that wouldn't be covered by this approach, so it's not a wholesale replacement, but I think it covers a bunch of the ones where the attribute is currently a bit messy.

Like this one would make a great negative impl, for example:

2 Likes

Thanks for working on this. I'd really like to use this outside the standard library.

I'd actually been thinking on how to implement this, but hadn't thought of this. Great idea.

I would prefer this over attributes.

Errors like "move occurs because s has type String, which does not implement the Copy trait" tend to trip up newcomers. Often their first thought is "I should implement Copy on String"., and an explicit impl !Copy for String {} should be able to generate a better message. Such impls will scale much better than giant attributes on traits themselves.

.

Yes but you can't do that for any type, so maybe we should improve the general error message (try to explain better what Copy is and why the type can't be copied, for example) than improve this specific case.

I think this confusion comes in part from another source, which you've accidentally illustrated: that isn't meant to be the primary error message. The primary error message is “use of moved value”, which is the first line of the error block, but the formatting tends to draw the error to the “framed” note/help texts, at the expense of the primary error message which is visually attached to the file path and line numbers. Many beginners ask for help and quote the help: and leave out the rest of the message.

When they see “does not implement Copy” but don't see “use of moved value”, they're more likely to think that Copy is the problem.

1 Like

What part of the error message is exposed in the IDE by rust-analyzer? I know I've learned to run cargo check when I get an error message I can't immediately diagnose, and that's easily lost on new developers (especially ones used to IDE-only development). (To that point, it might make sense for r-a to explicitly request the short form diagnostics (or a new flag), since those are more directly optimized for display inline (e.g. in the IDE) than the regular diagnostics or trying to extract the short diagnostic from the full one.)

3 Likes

This is a constant source of frustration when seeing newcomers get tripped out by the limited output that rust-analyzer provides. This is not ra's fault: VSCode doesn't provide good APIs for more elaborate output, and using the existing building block of "custom html pane" is a significant undertaking.

As a diesel maintainer I would be interested in having something like on_unimplemented available on a stable release. Especially if it allows us to improve errors like Rust tries to be too helpful with impl suggestions, sometimes being unhelpful · Issue #28894 · rust-lang/rust · GitHub.

I would be fine with having no guarantees, as long as that does not mean that the interface will completely change with every new rust version.

1 Like

I wouldn't anticipate that. rustc_on_unimplemented has changed maybe 3 times in the past 5 years, and every time it was in a backwards compatible way. I want to leave that escape hatch because its current API is very ad-hoc and should be reworked.

1 Like