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.