-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Include enum path in variant suggestion #101357
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,7 @@ use rustc_middle::ty::subst::SubstsRef; | |
use rustc_middle::ty::{self, AdtDef, Ty, UpvarSubsts}; | ||
use rustc_middle::ty::{CanonicalUserType, CanonicalUserTypeAnnotation}; | ||
use rustc_span::def_id::LocalDefId; | ||
use rustc_span::{Span, Symbol, DUMMY_SP}; | ||
use rustc_span::{sym, Span, Symbol, DUMMY_SP}; | ||
use rustc_target::abi::VariantIdx; | ||
use rustc_target::asm::InlineAsmRegOrRegClass; | ||
use std::fmt; | ||
|
@@ -695,17 +695,32 @@ impl<'tcx> fmt::Display for Pat<'tcx> { | |
Ok(()) | ||
} | ||
PatKind::Variant { ref subpatterns, .. } | PatKind::Leaf { ref subpatterns } => { | ||
let variant = match self.kind { | ||
PatKind::Variant { adt_def, variant_index, .. } => { | ||
Some(adt_def.variant(variant_index)) | ||
} | ||
_ => self.ty.ty_adt_def().and_then(|adt| { | ||
if !adt.is_enum() { Some(adt.non_enum_variant()) } else { None } | ||
let variant_and_name = match self.kind { | ||
PatKind::Variant { adt_def, variant_index, .. } => ty::tls::with(|tcx| { | ||
let variant = adt_def.variant(variant_index); | ||
let adt_did = adt_def.did(); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
} else { | ||
format!("{}::{}", tcx.def_path_str(adt_def.did()), variant.name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't variants have a def id that can be passed into |
||
}; | ||
Some((variant, name)) | ||
}), | ||
_ => self.ty.ty_adt_def().and_then(|adt_def| { | ||
if !adt_def.is_enum() { | ||
ty::tls::with(|tcx| { | ||
Some((adt_def.non_enum_variant(), tcx.def_path_str(adt_def.did()))) | ||
}) | ||
} else { | ||
None | ||
} | ||
}), | ||
}; | ||
|
||
if let Some(variant) = variant { | ||
write!(f, "{}", variant.name)?; | ||
if let Some((variant, name)) = &variant_and_name { | ||
write!(f, "{}", name)?; | ||
|
||
// Only for Adt we can have `S {...}`, | ||
// which we handle separately here. | ||
|
@@ -730,8 +745,9 @@ impl<'tcx> fmt::Display for Pat<'tcx> { | |
} | ||
} | ||
|
||
let num_fields = variant.map_or(subpatterns.len(), |v| v.fields.len()); | ||
if num_fields != 0 || variant.is_none() { | ||
let num_fields = | ||
variant_and_name.as_ref().map_or(subpatterns.len(), |(v, _)| v.fields.len()); | ||
if num_fields != 0 || variant_and_name.is_none() { | ||
write!(f, "(")?; | ||
for i in 0..num_fields { | ||
write!(f, "{}", start_or_comma())?; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it might give incorrect suggestions when using
#![no_implicit_prelude]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the change you remember @oli-obk was about the ordering of suggestions (it filters
core::
paths if thestd::
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.