E0072 needs better underline

[sophiajt]

Currently, E0072 falls back to a multi-line

error[E0072]: recursive type `ListNode` has infinite size
  --> src/test/compile-fail/E0072.rs:11:1
   |
11 | struct ListNode { //~ ERROR E0072
   | ^ recursive type has infinite size
   |
   = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `ListNode` representable

Instead, can we underline a better span. It might take some experimentation to find the one that works with the ast, but perhaps something like:

error[E0072]: recursive type `ListNode` has infinite size
  --> src/test/compile-fail/E0072.rs:11:1
   |
11 | struct ListNode { //~ ERROR E0072
   |        ^^^^^^^^ recursive type has infinite size
   |
   = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `ListNode` representable

[KiChjang]

I think the best solution here is to have two spans, one underlining the struct ListNode and the other underlining where rustc thinks it is a recursive type.

[KiChjang]

So in other words, we're basically clarifying "some point" in the help error message and giving it a concrete location. I'm not sure why this isn't done before?

[sophiajt]

@KiChjang - could just be that no one had fixed it, yet.

[phungleson]

Hey guys, have been looking into this for last few days and do some trials and errors to understand the structure here.

rust/src/librustc/traits/error_reporting.rs

Lines 575 to 584 in 147371f

	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

I seem to get the basic ideas but it would be helpful if there is some documents or some similar examples to guide how to fix this?

[sophiajt]

@phungleson - yeah, some of these bonuses can definitely be tricky, and some of them don't even have a clear solution given the representations available in the compiler. From what I, @KiChjang, and others have seen so far, types seem to be the ones with the least amount of span information, and so this bonus might not have a clear solution.

Now, that doesn't mean we can't change things up a bit.

I was chatting with @nikomatsakis this morning, and there may be a fix we can do. It's a pretty major fix, especially compared to the other bonuses. Basically, the Name struct of Item would become something like a Spanned, where the .name would have a span and an id associated with it. As you can imagine, this fix will cause a ripple across the compiler.

If you want to give it a try, let me know. I'd also be happy to do this change, but it may take me a little while to get to it (preparing for RustConf for the next week)

[KiChjang]

Check what the span contains here. It's being passed to is_representable, but I don't seem to see it being actually used in any sort of error reporting.

Since you already have the AST node, it's just a matter of scanning the struct's fields/enum variants again to check which of the fields/variants have the exact same type of the struct/enum. You'll have to convert the field/variant into a Ty first though, and then do something similar to what these two functions are doing.

[phungleson]

@KiChjang the span represents the whole struct src/test/compile-fail/E0072.rs:11:1: 15:2

[KiChjang]

Hmm... in which case, the span provided is not so useful. I'd say remove it from the method signature altogether.

Furthermore, I think this is a little bit more involved - you cannot assume that the struct/enum type is the one that is being recursive. You may need to change Representability::SelfRecursive to be a 1-tuple struct instead, containing the type that is recursive, and then check the struct/enum in question and its fields/variants to see which one of them is recursive.

[phungleson]

Sure thanks @KiChjang, understand what you mean.

However let's discuss about the expected structure of the Node first based on what @jonathandturner suggested.

With this struct

struct ListNode {
    head: u8,
    tail: Option<ListNode>,
}

The current Node looks like this

NodeItem(
  Item {
    name: ListNode(70),
    attrs: [],
    id: 4,
    node: ItemStruct(
      Struct([StructField {name: head(71), ...}, StructField {name: tail(73), ...}], 10),
      Generics {...}
    ),
    vis: Inherited,
    span: src/test/compile-fail/E0072.rs:11:1: 15:2
  }
)

I assume what @jonathandturner meant by .name is name: ListNode(70) here.

A couple of questions:

Firstly, about structure of new Spanned Name, does is look good enough or anything you guys might want to change

name: Name {
  id: 5 // new node id
  span: src/test/compile-fail/E0072.rs:11:8: 11:15 // the exact place of the name
  data: ListNode(70) // a field to hold the old name data, can you guys suggest better name?
}

Secondly, should we create a new Name like ItemName or replace directly this Name with the aforementioned struct?

[sophiajt]

fwiw -- and I might be mistaken in exactly where it's best to put this -- but I was thinking the work would be to update struct Item from /Users/jturner/Source/rust/src/librustc/hir/mod.rs:

From:

pub struct Item {
    pub name: Name,
    pub attrs: HirVec<Attribute>,
    pub id: NodeId,
    pub node: Item_,
    pub vis: Visibility,
    pub span: Span,
}

To:

pub struct Item {
    pub name: Spanned<Name>,
    pub attrs: HirVec<Attribute>,
    pub id: NodeId,
    pub node: Item_,
    pub vis: Visibility,
    pub span: Span,
}

[phungleson]

Thanks @jonathandturner yes that should work as well, in this case:

1. If we use Spanned then we probably don't have id for name since Spanned only has node which is Name and span, is it ok?
2. And for the sake of my knowledge and make sure we target correct Item what the different between this Item and this Item

[sophiajt]

pinging @nikomatsakis ...

@phungleson: I'm not entirely sure I follow what you're saying... are you saying id and name are used for the same thing? If, by chance, you're not sure about the Spanned part, this is from the power of generics. Because we're saying this is a Spanned, the T in the Spanned definition becomes name. This lets us use node and get the value for the name back out again later.

For #2 - I'm not sure. Maybe @nikomatsakis might know.

[phungleson]

@jonathandturner ah sorry, let me clarify.

Yep I know Spanned and was thinking of using it but in previous comment you mentioned .name would have a span and an id associated with it. Spanned has .span but doesn't have .id so just want to confirm we will not have .name.id?

Other than that I am happy to make changes based on what you suggested.

[sophiajt]

@phungleson - yeah, I could see how what I said was a confusing way to say it. Sorry about that. What I meant was

pub struct Name(pub u32);

The pub u32 part of the name would be available for us to use (what I called the id, but is really use a number), as well as an associated Span.

[phungleson]

Cool thanks @jonathandturner, I will make the changes.

[phungleson]

@jonathandturner

I re-make a new PR to change to hir::Item#name: Spanned<Name>

#36466

It is still WIP and need some inputs from you and @KiChjang to move forward.

[phungleson]

@jonathandturner @KiChjang, sorry I was a bit busy last few months so this issue became rather old..

This is a very interesting issue so I want to pick this up again, is it ok?

[KiChjang]

It looks like no-one else have worked on this issue while you were away, so go right ahead and pick this back up!

[KiChjang]

@phungleson Oh whoops, looks like @estebank submitted a PR to solve this issue.

[KiChjang]

@estebank Is this still being worked on?

[estebank]

@KiChjang I've been offline for a couple of weeks and haven't been able to go back to this yet. Although it is still on my radar, it's unlikely I'll take another stab at this for at least a month. If you want to take it instead:

What I did:

• change the name in parts of the AST to Spanned<Name> in order for the name field to contain the span for only the name

What I was planning on doing when working on it again:

• add a name_span or short_span Option<Span> field to the AST node
• add a method short_span() that would do

   match self.name_span {
      Some(s) => s,
      None => self.span,
   }

By doing it that way the patch should be much smaller than my PR ended up being.

Take a look at the thread on #38328 for the discussion.

[Mark-Simulacrum]

Closing as per #41214 (comment).