Skip to content

Commit

Permalink
Rollup merge of #127431 - oli-obk:feed_item_attrs, r=compiler-errors
Browse files Browse the repository at this point in the history
Use field ident spans directly instead of the full field span in diagnostics on local fields

This improves diagnostics and avoids having to store the `DefId`s of fields
  • Loading branch information
jieyouxu authored Jul 8, 2024
2 parents e76b017 + 8c2ea71 commit bd4ab30
Show file tree
Hide file tree
Showing 9 changed files with 49 additions and 45 deletions.
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 @@ -1407,7 +1407,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 @@ -2128,10 +2128,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(),
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

0 comments on commit bd4ab30

Please sign in to comment.