Skip to content
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

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

Merged
merged 1 commit into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
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
Expand Up @@ -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]) {
Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_resolve/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
44 changes: 19 additions & 25 deletions compiler/rustc_resolve/src/late/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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"),
Expand Down Expand Up @@ -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,
};
Expand Down Expand Up @@ -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(", "),
);
Expand Down Expand Up @@ -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));
}
}
}
Expand Down Expand Up @@ -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())
}
}
};
Expand Down
18 changes: 13 additions & 5 deletions compiler/rustc_resolve/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
),
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/resolve/issue-2356.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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;
| ^^^^^^^^
Expand Down Expand Up @@ -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;
| ^^^^^^^^
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/resolve/issue-60057.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
| ^^^^^^
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -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`
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/resolve/unresolved_static_type_field.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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);
| ^^
Expand Down
Loading