Enable trimmed paths

[camsteffen]

changelog: Uses "trimmed paths" instead of fully qualified paths in output

This flips a magic switch in driver.rs so that we get Option instead of std::option::Option from cx.tcx.def_path_str, for example. This improves Clippy output all around. There aren't a whole lot of these cases. But I suspect there are a number of cases where def_path_str would be a better choice than the current implementation, and it might have been used more if this feature were already enabled.

---

Unfortunately this comes with a footgun. Any time we fetch a trimmed path (usually through Ty::to_string or cx.tcx.def_path_str), but then don't use it to emit a lint, rustc ICEs. This is especially problematic with our utility functions span_lint_* because these functions don't emit if the lint is allowed.

let msg = format!("you did a bad thing with with `{}`", cx.tcx.def_path_str(def_id));
// ICEBERGS AHEAD!!! Will ICE if SILLY_LINT is allowed!
span_lint(cx, SILLY_LINT, span, msg);

To solve this, I created a new util span_clippy_lint that works similarly to span_lint_and_then. It uses a Drop guard to add our special Clippy messages at the end.

span_clippy_lint(cx, SILLY_LINT, span, |diag| {
    // the lint is enabled at this point so we're safe!
    diag.build(format!("you did a bad thing with with `{}`", cx.tcx.def_path_str(def_id)))
});

And of course you can use anything in Diagnostic like span_suggestion or help in a call chain. TBH I like this API a lot more than our current util functions.

This still leaves a rough edge for Clippy contributors since we can't ever fetch a trimmed path outside of the "diag" closure, and our tests don't necessarily catch the problem if we do it wrong. So this is something all maintainers would need to be wary of when reviewing.

For now, I just converted some lints to span_clippy_lint to avoid ICE where needed. But I think we need to convert all the lints over and delete the span_lint_* functions to get rid of the footgun. Opening for feedback. @rust-lang/clippy

Closes #6385
Closes #7212

---

TODO

• Should we do this?
• Replace span_lint_* with span_clippy_lint everywhere
• Fix metadata collection
• Internal lint for trimmed path on "good path"
• Internal lint for trimmed path in code suggestions
• Remove span_lint_* and related internal lints

[rust-highfive]

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

[Manishearth]

I like this, we should definitely get some agreement from the team though, and perhaps be more explicit about documenting things reviewers need to be aware of.

[flip1995]

Unfortunately this comes with a footgun. Any time we fetch a trimmed path (usually through Ty::to_string or cx.tcx.def_path_str), but then don't use it to emit a lint, rustc ICEs.

It seems like this is not a bug, but a feature of the Rust compiler. 🤔 So we'll have to deal with that somehow.

TBH I like this API a lot more than our current util functions. [...] I think we need to convert all the lints over and delete the span_lint_* functions to get rid of the footgun.

What I really like about our utils functions is that they're just easy to understand and easy to use, because the arguments are really straight forward. You don't have to deal with a DiagnosticBuilder closure and know how diagnostics are actually build in rustc. This also means, you don't have to go through the massive DiagnosticBuilder documentation to find out how to add a suggestion / help / help with span / ..., you can just use the span_lint_and_* util and everything is done for you. I'd like to keep building lint output as easy as possible, so that beginners don't trip over that.

A reason why we can have so easy to use utils is that they are good enough for our purposes most of the time. (Also DRY: we'd see many of the same closures over and over again. We even have an internal lint to avoid this currently)

So this is something all maintainers would need to be wary of when reviewing.

That is a really bad idea in general. Does this only happen with the def_path_str query or are there other ways to build those paths that lead to an ICE? If it's only the def_path_str query, we can easily avoid maintainers missing this, by implementing an internal lint, that makes sure that def_path_str only gets called in the closure of the span_clippy_lint utils function.

I don't want to rely on humans remembering to check for specific queries to avoid ICEs in every review.

---

I definitely like the trimmed paths part of this PR.

[camsteffen]

What I really like about our utils functions is that they're just easy to understand and easy to use, because the arguments are really straight forward.

Funny, I feel that I had the opposite experience when I was a beginner. I found all the arguments with two spans and two messages hard to reason about what is what. I was particularly confused about the Option<Span> - "why do I need another Span and why is it in an Option?". .help and .span_help are easier to understand in this way. The DiagnosticBuilder breaks things down into smaller, self-documenting pieces.

I don't want to rely on humans remembering to check for specific queries to avoid ICEs in every review.

Yeah I was hesitant about this part. There is TrimmedDefPath::Always which simply disables the ICE. So we could use that, but I think it would be better to safely ensure we avoid the expensive query when not needed.

I think def_path_str and Ty as Display are the only (realistic) entry points from Clippy to trimmed paths. @estebank could you comment on that? So I think we could create an internal lint to keep us safe.

[xFrednet]

What I really like about our utils functions is that they're just easy to understand and easy to use, because the arguments are really straight forward

I agree with this one for simple use cases, which are the majority of our good-first-issue lints. However, for more complex use cases, it's hard to avoid the usage of span_lint_and_then. From personal experience and the impression I got from others, it seems like using the DiagnosticBuilder instead of our util functions is still fairly easy, even if it sadly sometimes requires duplicate code. Therefore, I don't have a strong opinion on the API. But I don't believe that it would solve all problems, since even in the cases of span_lint_and_then we often build the suggestion outside the closure.

(Slightly related: some functions for building suggestions have notes on them that they are slowish (See: Symbol::as_str()). We could probably improve Clippy's performance by first checking if a lint is enabled, before building the suggestion or having the lint logic in general)

---

Could we maybe make the flag conditional? If we set it to TrimmedDefPath::Always on stable and only on nightly to TrimmedDefPath::GoodPath we could at least avoid ICEs on stable. 🤔

[estebank]

Please be aware that the trimmed path is not the locally available name. It "just" looks to see if the identifier is globally unique and uses just the name if it is. This is a hack and likely not what people expect.

[camsteffen]

Please be aware that the trimmed path is not the locally available name.

Good point. I was aware of that, but you got me thinking some more. I guess we can't use def_path_str for code suggestions, but I think it is still good for lint messages. For machine-applicable suggestions, we have to use other means to determine the imported path (like a code snippet), if possible. And this is another thing we could enforce with an internal lint.

This is a hack

@estebank Are you saying it's a bad idea to port this "feature" to Clippy?

[iiYese]

This should be a welcomed feature. At the very least it should be flag enableable with something like cargo clippy --trimmed. In fact since cargo check already does this I would argue this should be the default behavior for consistency and verbosity can be shown with something like cargo clippy --full-path or cargo check --full-path. Currently clippy diagnostics are unreadable for any sufficiently sophisticated body of code.

[camsteffen]

Closing in favor of #7798