Include enum path in variant suggestion #101357
Conversation
rustbot
added
the
T-compiler
|
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #101139) made this pull request unmergeable. Please resolve the merge conflicts. |
compiler-errors
force-pushed
the
variant-sugg-tweak
branch
from
11393ad
to
be53bd8
Compare
| let name = if tcx.get_diagnostic_item(sym::Option) == Some(adt_did) | ||
| || tcx.get_diagnostic_item(sym::Result) == Some(adt_did) |
There was a problem hiding this comment.
This seems like it might give incorrect suggestions when using #![no_implicit_prelude].
There was a problem hiding this comment.
I guess, but that could probably be said about plenty of other suggestions where we're currently rendering paths, making suggestions, etc. They're, at most, best effort.
I don't think this needs to be fixed, especially because it's basically impossible to determine what the proper shortest path to refer to an item is in any module, due to rustc_resolve not exposing import information in a way we can consume here, and it would be unnecessarily verbose to print the full untrimmed path (e.g. std::option::Option::Some).
There was a problem hiding this comment.
cc @estebank I remember some tests getting updated that now figure out the best import path to use, but I may be totally off here
There was a problem hiding this comment.
I believe the change you remember @oli-obk was about the ordering of suggestions (it filters core:: paths if the std:: is available too), not for this. I think that we eventually would want to rebuild suggestions so that we can ask for paths in a specific DefId or HirId so that it is always accurate for that scope, but we're far from there.
I'm ok with landing this as is.
|
@bors r+ |
bors
added
S-waiting-on-bors
| { | ||
| variant.name.to_string() | ||
| } else { | ||
| format!("{}::{}", tcx.def_path_str(adt_def.did()), variant.name) |
There was a problem hiding this comment.
Don't variants have a def id that can be passed into def_path_str?
| let name = if tcx.get_diagnostic_item(sym::Option) == Some(adt_did) | ||
| || tcx.get_diagnostic_item(sym::Result) == Some(adt_did) | ||
| { | ||
| variant.name.to_string() |
There was a problem hiding this comment.
If we use the printing machinery (that only uses the name if it is unique, otherwise the full path), then this suggestion would be mostly right in the face of local shadowing (but see previous comment about needing a function that takes a scope identifier for accurate paths, always).
…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#101357 (Include enum path in variant suggestion) - rust-lang#101434 (Update `SessionDiagnostic::into_diagnostic` to take `Handler` instead of `ParseSess`) - rust-lang#101445 (Suggest introducing an explicit lifetime if it does not exist) - rust-lang#101457 (Recover from using `;` as separator between fields) - rust-lang#101462 (Rustdoc-Json: Store Variant Fields as their own item.) - rust-lang#101471 (Report number of delayed bugs properly with `-Ztreat-err-as-bug`) - rust-lang#101473 (Add more size assertions for MIR types.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
(except for
ResultandOption, which we should have via the prelude)Fixes #101356