Skip to content

Use field ident spans directly instead of the full field span in diagnostics on local fields #127431

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

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions compiler/rustc_resolve/src/build_reduced_graph.rs
Original file line number Diff line number Diff line change
@@ -321,8 +321,14 @@ impl<'a, 'b, 'tcx> BuildReducedGraphVisitor<'a, 'b, 'tcx> {
// The fields are not expanded yet.
return;
}
let def_ids = fields.iter().map(|field| self.r.local_def_id(field.id).to_def_id());
self.r.field_def_ids.insert(def_id, self.r.tcx.arena.alloc_from_iter(def_ids));
let fields = fields
.iter()
.enumerate()
.map(|(i, field)| {
field.ident.unwrap_or_else(|| Ident::from_str_and_span(&format!("{i}"), field.span))
})
.collect();
self.r.field_names.insert(def_id, fields);
}

fn insert_field_visibilities_local(&mut self, def_id: DefId, fields: &[ast::FieldDef]) {
6 changes: 1 addition & 5 deletions compiler/rustc_resolve/src/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -1726,11 +1726,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
)) = binding.kind
{
let def_id = self.tcx.parent(ctor_def_id);
return self
.field_def_ids(def_id)?
.iter()
.map(|&field_id| self.def_span(field_id))
.reduce(Span::to); // None for `struct Foo()`
return self.field_idents(def_id)?.iter().map(|&f| f.span).reduce(Span::to); // None for `struct Foo()`
}
None
}
44 changes: 19 additions & 25 deletions compiler/rustc_resolve/src/late/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -1532,17 +1532,17 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
if !this.has_private_fields(def_id) {
// If the fields of the type are private, we shouldn't be suggesting using
// the struct literal syntax at all, as that will cause a subsequent error.
let field_ids = this.r.field_def_ids(def_id);
let (fields, applicability) = match field_ids {
Some(field_ids) => {
let fields = field_ids.iter().map(|&id| this.r.tcx.item_name(id));

let fields = this.r.field_idents(def_id);
let has_fields = fields.as_ref().is_some_and(|f| !f.is_empty());
let (fields, applicability) = match fields {
Some(fields) => {
let fields = if let Some(old_fields) = old_fields {
fields
.iter()
.enumerate()
.map(|(idx, new)| (new, old_fields.get(idx)))
.map(|(new, old)| {
let new = new.to_ident_string();
let new = new.name.to_ident_string();
if let Some(Some(old)) = old
&& new != *old
{
@@ -1553,17 +1553,17 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
})
.collect::<Vec<String>>()
} else {
fields.map(|f| format!("{f}{tail}")).collect::<Vec<String>>()
fields
.iter()
.map(|f| format!("{f}{tail}"))
.collect::<Vec<String>>()
};

(fields.join(", "), applicability)
}
None => ("/* fields */".to_string(), Applicability::HasPlaceholders),
};
let pad = match field_ids {
Some([]) => "",
_ => " ",
};
let pad = if has_fields { " " } else { "" };
err.span_suggestion(
span,
format!("use struct {descr} syntax instead"),
@@ -1723,12 +1723,9 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
&args[..],
);
// Use spans of the tuple struct definition.
self.r.field_def_ids(def_id).map(|field_ids| {
field_ids
.iter()
.map(|&field_id| self.r.def_span(field_id))
.collect::<Vec<_>>()
})
self.r
.field_idents(def_id)
.map(|fields| fields.iter().map(|f| f.span).collect::<Vec<_>>())
}
_ => None,
};
@@ -1791,7 +1788,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
(Res::Def(DefKind::Ctor(_, CtorKind::Fn), ctor_def_id), _) if ns == ValueNS => {
let def_id = self.r.tcx.parent(ctor_def_id);
err.span_label(self.r.def_span(def_id), format!("`{path_str}` defined here"));
let fields = self.r.field_def_ids(def_id).map_or_else(
let fields = self.r.field_idents(def_id).map_or_else(
|| "/* fields */".to_string(),
|field_ids| vec!["_"; field_ids.len()].join(", "),
);
@@ -2017,12 +2014,9 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
if let Some(Res::Def(DefKind::Struct | DefKind::Union, did)) =
resolution.full_res()
{
if let Some(field_ids) = self.r.field_def_ids(did) {
if let Some(field_id) = field_ids
.iter()
.find(|&&field_id| ident.name == self.r.tcx.item_name(field_id))
{
return Some(AssocSuggestion::Field(self.r.def_span(*field_id)));
if let Some(fields) = self.r.field_idents(did) {
if let Some(field) = fields.iter().find(|id| ident.name == id.name) {
return Some(AssocSuggestion::Field(field.span));
}
}
}
@@ -2418,7 +2412,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
match kind {
CtorKind::Const => false,
CtorKind::Fn => {
!self.r.field_def_ids(def_id).is_some_and(|field_ids| field_ids.is_empty())
!self.r.field_idents(def_id).is_some_and(|field_ids| field_ids.is_empty())
}
}
};
18 changes: 13 additions & 5 deletions compiler/rustc_resolve/src/lib.rs
Original file line number Diff line number Diff line change
@@ -991,7 +991,7 @@ pub struct Resolver<'a, 'tcx> {
extern_prelude: FxHashMap<Ident, ExternPreludeEntry<'a>>,

/// N.B., this is used only for better diagnostics, not name resolution itself.
field_def_ids: LocalDefIdMap<&'tcx [DefId]>,
field_names: LocalDefIdMap<Vec<Ident>>,

/// Span of the privacy modifier in fields of an item `DefId` accessible with dot syntax.
/// Used for hints during error reporting.
@@ -1406,7 +1406,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
prelude: None,
extern_prelude,

field_def_ids: Default::default(),
field_names: Default::default(),
field_visibility_spans: FxHashMap::default(),

determined_imports: Vec::new(),
@@ -2127,10 +2127,18 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
}
}

fn field_def_ids(&self, def_id: DefId) -> Option<&'tcx [DefId]> {
fn field_idents(&self, def_id: DefId) -> Option<Vec<Ident>> {
match def_id.as_local() {
Some(def_id) => self.field_def_ids.get(&def_id).copied(),
None => Some(self.tcx.associated_item_def_ids(def_id)),
Some(def_id) => self.field_names.get(&def_id).cloned(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of invoking queries at all the field_def_ids call sites, we now have a single clone here. This only ever happens in diagnostics code paths, so it's not perf critical anyway.

None => Some(
self.tcx
.associated_item_def_ids(def_id)
.iter()
.map(|&def_id| {
Ident::new(self.tcx.item_name(def_id), self.tcx.def_span(def_id))
})
.collect(),
),
}
}

Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ error[E0425]: cannot find value `field` in this scope
--> $DIR/field-and-method-in-self-not-available-in-assoc-fn.rs:11:9
|
LL | field: u32,
| ---------- a field by that name exists in `Self`
| ----- a field by that name exists in `Self`
...
LL | fn field(&self) -> u32 {
| ----- a method by that name is available on `Self` here
@@ -14,7 +14,7 @@ error[E0425]: cannot find value `field` in this scope
--> $DIR/field-and-method-in-self-not-available-in-assoc-fn.rs:12:15
|
LL | field: u32,
| ---------- a field by that name exists in `Self`
| ----- a field by that name exists in `Self`
...
LL | fn field(&self) -> u32 {
| ----- a method by that name is available on `Self` here
4 changes: 2 additions & 2 deletions tests/ui/resolve/issue-2356.stderr
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ error[E0425]: cannot find value `whiskers` in this scope
--> $DIR/issue-2356.rs:39:5
|
LL | whiskers: isize,
| --------------- a field by that name exists in `Self`
| -------- a field by that name exists in `Self`
...
LL | whiskers -= other;
| ^^^^^^^^
@@ -35,7 +35,7 @@ error[E0425]: cannot find value `whiskers` in this scope
--> $DIR/issue-2356.rs:84:5
|
LL | whiskers: isize,
| --------------- a field by that name exists in `Self`
| -------- a field by that name exists in `Self`
...
LL | whiskers = 4;
| ^^^^^^^^
2 changes: 1 addition & 1 deletion tests/ui/resolve/issue-60057.stderr
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ error[E0425]: cannot find value `banana` in this scope
--> $DIR/issue-60057.rs:8:21
|
LL | banana: u8,
| ---------- a field by that name exists in `Self`
| ------ a field by that name exists in `Self`
...
LL | banana: banana
| ^^^^^^
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ error[E0425]: cannot find value `config` in this scope
--> $DIR/typo-suggestion-for-variable-with-name-similar-to-struct-field.rs:7:16
|
LL | config: String,
| -------------- a field by that name exists in `Self`
| ------ a field by that name exists in `Self`
...
LL | Self { config }
| ^^^^^^ help: a local variable with a similar name exists: `cofig`
@@ -11,7 +11,7 @@ error[E0425]: cannot find value `config` in this scope
--> $DIR/typo-suggestion-for-variable-with-name-similar-to-struct-field.rs:11:20
|
LL | config: String,
| -------------- a field by that name exists in `Self`
| ------ a field by that name exists in `Self`
...
LL | println!("{config}");
| ^^^^^^ help: a local variable with a similar name exists: `cofig`
2 changes: 1 addition & 1 deletion tests/ui/resolve/unresolved_static_type_field.stderr
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ error[E0425]: cannot find value `cx` in this scope
--> $DIR/unresolved_static_type_field.rs:9:11
|
LL | cx: bool,
| -------- a field by that name exists in `Self`
| -- a field by that name exists in `Self`
...
LL | f(cx);
| ^^