Update E0072 bonus to new error format

[phungleson]

WIP

Fixes #35965
Part of #35233

r? @jonathandturner

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jonathandturner (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[KiChjang]

I think you misunderstood what I said. HIR items are NOT supposed to contain spans.

[phungleson]

Ah no worries, I just kept last changes so that it is easier to continue the discussion.

Okay so continue, when we display the error, we only have hir::Item. Specifically at this section

rust/src/librustc/traits/error_reporting.rs

Lines 571 to 585 in f1f40f8

	pub fn recursive_type_with_infinite_size_error(self,
	type_def_id: DefId)
	-> DiagnosticBuilder<'tcx>
	{
	assert!(type_def_id.is_local());
	let span = self.map.span_if_local(type_def_id).unwrap();
	let mut err = struct_span_err!(self.sess, span, E0072,
	"recursive type `{}` has infinite size",
	self.item_path_str(type_def_id));
	err.span_label(span, &format!("recursive type has infinite size"));
	err.help(&format!("insert indirection (e.g., a `Box`, `Rc`, or `&`) \
	at some point to make `{}` representable",
	self.item_path_str(type_def_id)));
	err
	}

It seems that we can get the hir::Item by doing something like this self.map.find(4), not sure if we can get ast::Item at that place?

[KiChjang]

You don't even need to go through all this. recursive_type_with_infinite_size_error is only called in librustc_typeck/check/mod.rs, and it already contains an item_id that you can use.

So instead of passing in a DefId to recursive_type_with_infinite_size_error, pass in an ast::NodeId, and then use self.map.expect_item(item_id), and you'll get the ast::Item.

[phungleson]

Nice thanks, I will try to make the changes that way.

In that case do we need to change ast::Item#ident or ast::Item#ident#name to become Spanned?

[KiChjang]

I'm inclined to make a span field on ast::Ident instead. Pinging @jonathandturner here to see what he thinks.

[sophiajt]

@KiChjang - that's an interesting approach. I don't work much in the front end, so pinging @nikomatsakis for comment.

[phungleson]

While waiting for @nikomatsakis

@KiChjang if we add a span to ast::Ident then we likely to change the signature of this method.

pub const fn with_empty_ctxt(name: Name) -> Ident {
    Ident { name: name, ctxt: SyntaxContext::empty() }
}

And this will require to change in many places where the method is called.

Should it be ok?

[KiChjang]

This doesn't seem to be a problem. When you change Name to Spanned<Name>, you're also making a breaking change that requires a lot of changes in the code where it originally expects Name.

[KiChjang]

This is a parser [breaking change], btw.

[bors]

☔ The latest upstream changes (presumably #36551) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with a rebase!