Use struct name as span instead of entire block #38328

estebank · 2016-12-12T21:12:21Z

error[E0072]: recursive type `ListNode` has infinite size
  --> ../../../src/test/compile-fail/E0072.rs:11:8
   |
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 of

error[E0072]: recursive type `ListNode` has infinite size
  --> <anon>:11:1
   |
11 |   struct ListNode { //~ ERROR E0072
   |  _^ starting here...
12 | |                   //~| NOTE recursive type has infinite size
13 | |     head: u8,
14 | |     tail: Option<ListNode>,
15 | | }
   | |_^ ...ending here: recursive type has infinite size
   |
   = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `ListNode` representable

Re #35965, #38246.

rust-highfive · 2016-12-12T21:12:34Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2016-12-12T21:12:41Z

cc @jonathandturner

estebank · 2016-12-12T21:14:49Z

Instead of replacing the current spans, I could add a new Optional field for the name's span and a method to fetch the appropriate one depending on the message. I haven't expanded this to all Items as I wanted to kick start a conversation with this and get feedback first.

nagisa · 2016-12-12T21:17:32Z

Would be nice to point at the field which causes the recursion instead :)

estebank · 2016-12-12T22:48:52Z

@nagisa That is definitely a part of #35965 that I've been looking at, but it'll take me some more time to actually clean up the code identifying the field from the detected Ty. I have a proof of concept to see how it'd look like once it's completed, without this current PR's changes and using string submatching to select the struct item:

That change is a bit more contained than this one. This PR will change the presentation of all places where a struct's span is used.

nagisa · 2016-12-12T23:54:35Z

This PR will change the presentation of all places where a struct's span is used.

I haven’t looked at the actual PR previously and only left a comment in passing. Now I see what you did here. It seems wrong. Firstly because when getting span of something (e.g. item) you expect the span to encompass the whole item, always. That’s universally true across compiler. For example, I imagine code generating debug info for structures and stuff may be using the full span and doing such change as yours would end up making the lines in debug info… weird.

estebank · 2016-12-13T01:53:07Z

@nagisa would you endorse adding a new field to the underlying structure holding something along the lines of name_span or def_span that would point to this place?

nagisa · 2016-12-13T02:02:52Z

I’d probably just make Name of item a Spanned<Name>, though given the extra bytes added to the item, I’m kind of reluctant to recommend anything.

``` error[E0072]: recursive type `ListNode` has infinite size --> ../../../src/test/compile-fail/E0072.rs:11:8 | 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 ```

oli-obk · 2016-12-13T10:16:57Z

since the name-span is only useful in case of errors and the name span is a sub-span of the item span, we could instead write a function that uses an item span and extracts the name span from that by looking into the codemap and reparse the item span.

estebank · 2016-12-14T21:43:32Z

@nagisa I've followed that suggestion (in the naive way, there's lots of room to clean up the patch) of using Spanned. I feel that if we're adding this here, there might be plenty of places where the diagnostic could point at the name instead of the block, but for now it seems like a big diff for a very small improvement.

@oli-obk I don't think there's any way to get the item name span reliably from the item span other than fully parsing the code therein, which feels to me like a worse solution that just storing that info in the first place.

As per nagisa's review comment. WIP.

nikomatsakis · 2016-12-19T20:41:34Z

My take:

I am 👍 on using the span for the item's name more generally -- both in this error but also in the errors that refer to unused methods and so forth. So making the item's name spanned seems like a good idea.

I don't think there's any way to get the item name span reliably from the item span other than fully parsing the code therein, which feels to me like a worse solution that just storing that info in the first place.

I agree that we can't automatically derive the name span from the other span. I guess you don't have to do a full parse -- in general you only have to skip the keyword (struct, fn, etc) and find the next identifier. But I'm not sure if that holds true when you factor in macros and so forth. Note also that the name may have a span that is actually distinct from the item itself. For example, what span does your patch give for this example?

macro_rules! mk_struct {
    ($n:ident) => {
        struct $n { field: $n }
    }
}

mk_struct!(Foo);

fn main() { }

I've only skimmed the patch, so I'm not sure whether the various uses of dummy_spanned make sense. Probably they do.

nikomatsakis · 2016-12-19T20:42:09Z

(On the playground, that example cites the macro definition; I imagine that after your change it will ... probably? ... cite the macro use site.)

bors · 2016-12-21T02:55:42Z

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

nikomatsakis

Not sure if this feels quite right. The dummy_spanned calls worry me. What do you think about my suggestion? Perhaps there is a better way to avoid them?

nikomatsakis · 2016-12-21T17:57:03Z

src/librustc/hir/lowering.rs

@@ -798,7 +798,7 @@ impl<'a> LoweringContext<'a> {
                            path.span = span;
                            self.items.insert(import.id, hir::Item {
                                id: import.id,
-                                name: import.rename.unwrap_or(ident).name,
+                                name: dummy_spanned(import.rename.unwrap_or(ident).name),


why dummy_spanned here are not use span?

nikomatsakis · 2016-12-21T19:17:41Z

src/libsyntax/diagnostics/plugin.rs

@@ -227,7 +228,7 @@ pub fn expand_build_diagnostic_array<'cx>(ecx: &'cx mut ExtCtxt,

    MacEager::items(SmallVector::many(vec![
        P(ast::Item {
-            ident: name.clone(),
+            ident: codemap::dummy_spanned(name.clone()),


Hmm, why dummy_spanned here? I would think span would be a better choice.

nikomatsakis · 2016-12-21T19:39:47Z

src/libsyntax/ext/build.rs

@@ -1004,7 +1004,7 @@ impl<'a> AstBuilder for ExtCtxt<'a> {
        // FIXME: Would be nice if our generated code didn't violate
        // Rust coding conventions
        P(ast::Item {
-            ident: name,
+            ident: name.dummy_spanned(),


Similarly here. I feel like dummy_spanned is basically always bad, since it implies that we can't just cite an error at item.name.span, but rather we have to maybe fallback to item.span.

This perhaps suggests that rather than having us do item.name.span, we should add item.best_span or something, which is the span to use in an error (versus being the span of the entire item in the source).

nikomatsakis · 2016-12-27T22:18:05Z

@estebank ping :)

Thoughts on this?

Not sure if this feels quite right. The dummy_spanned calls worry me. What do you think about my suggestion? Perhaps there is a better way to avoid them?

estebank · 2016-12-28T23:19:19Z

Not sure if this feels quite right. The dummy_spanned calls worry me. What do you think about my suggestion? Perhaps there is a better way to avoid them?

@nikomatsakis, the dummy_spanneds pointed out in the review were mostly to get the PR to compile and get some feedback on the general approach :)

There're four options as I see them:

Use DUMMY_SPAN when an appropriate span is not available, add method as suggested to the node to get the error's span, selecting between the node's span and the name's span if the latter is not DUMMY_SPAN
Have a new MaybeSpanned class, that has an Option<Span>, add method as in the previous option
Add field name_span to items as Option<Span>.
Parse the full span in order to create the name span. I can certainly try this in a separate PR and to compare.

nikomatsakis · 2016-12-30T15:10:05Z

@estebank I think I prefer one of the two final variants. I am not a big fan of trying to parse the span, but I could see it working fairly well in practice as a heuristic. I am nervous around macros.

Otherwise, I think it might make sense to add a span field to Item -- but I wouldn't call it name_span, but rather something like best_span or err_span, and I would have it have type Span (not Option<Span>). At worst, it can be set to item.span. Basically, this would be the field to use when citing the item for errors etc, whereas span is the full span in the source.

(Alternatively, we could repurpose the existing span field and add a full_span alternative.)

I think my feeling is that we don't need to "specialize" to citing the name of an item -- we just want to know what is the best span to use when trying to locate "the item" itself. Sometimes this will be a name, but for some kinds of items it might be something different. For example, for an impl, I'd be inclined to use the impl header, or perhaps the impl keyword (after all, impls don't even have names! We just put in some dummy value.)

nikomatsakis · 2017-01-03T19:01:22Z

@estebank Are you planning to rework this substantially, then? I'm going to close this PR in the interim -- just to keep my queue tidy -- but feel free to re-open.

estebank · 2017-01-03T19:05:21Z

@nikomatsakis go ahead. Is it ok if I set you as reviewer directly of the new PR?

nikomatsakis · 2017-01-05T01:05:22Z

@estebank yep

…nkov Add a way to get shorter spans until `char` for pointing at defs ```rust error[E0072]: recursive type `X` has infinite size --> file.rs:10:1 | 10 | struct X { | ^^^^^^^^ recursive type has infinite size | = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `X` representable ``` vs ```rust error[E0072]: recursive type `X` has infinite size --> file.rs:10:1 | 10 | struct X { | _^ starting here... 11 | | x: X, 12 | | } | |_^ ...ending here: recursive type has infinite size | = help: insert indirection (e.g., a `Box`, `Rc`, or `&`) at some point to make `X` representable ``` Re: rust-lang#35965, rust-lang#38246. Follow up to rust-lang#38328. r? @jonathandturner

rust-highfive assigned nikomatsakis Dec 12, 2016

estebank mentioned this pull request Dec 12, 2016

Multiline highlights in diagnostic get noisy quickly #38246

Closed

estebank force-pushed the struct-underline branch from 7414ad6 to 0d19ec5 Compare December 14, 2016 21:39

estebank force-pushed the struct-underline branch 4 times, most recently from 8192a6d to 087155d Compare December 17, 2016 00:20

Replace Name and Symbol with Spanned<Name> and Spanned<Symbol>

8a383b1

As per nagisa's review comment. WIP.

estebank force-pushed the struct-underline branch from 087155d to 8a383b1 Compare December 17, 2016 19:48

nikomatsakis requested changes Dec 21, 2016

View reviewed changes

nikomatsakis closed this Jan 3, 2017

estebank mentioned this pull request Mar 7, 2017

E0072 needs better underline #35965

Closed

This was referenced Apr 7, 2017

Always show end line of multiline annotations #41136

Merged

Add a way to get shorter spans until char for pointing at defs #41214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use struct name as span instead of entire block #38328

Use struct name as span instead of entire block #38328

estebank commented Dec 12, 2016

rust-highfive commented Dec 12, 2016

estebank commented Dec 12, 2016

estebank commented Dec 12, 2016 •

edited

Loading

nagisa commented Dec 12, 2016

estebank commented Dec 12, 2016

nagisa commented Dec 12, 2016

estebank commented Dec 13, 2016

nagisa commented Dec 13, 2016

oli-obk commented Dec 13, 2016

estebank commented Dec 14, 2016

nikomatsakis commented Dec 19, 2016

nikomatsakis commented Dec 19, 2016

bors commented Dec 21, 2016

nikomatsakis left a comment

nikomatsakis Dec 21, 2016

nikomatsakis Dec 21, 2016

nikomatsakis Dec 21, 2016

nikomatsakis commented Dec 27, 2016

estebank commented Dec 28, 2016

nikomatsakis commented Dec 30, 2016

nikomatsakis commented Jan 3, 2017

estebank commented Jan 3, 2017

nikomatsakis commented Jan 5, 2017

Use struct name as span instead of entire block #38328

Use struct name as span instead of entire block #38328

Conversation

estebank commented Dec 12, 2016

rust-highfive commented Dec 12, 2016

estebank commented Dec 12, 2016

estebank commented Dec 12, 2016 • edited Loading

nagisa commented Dec 12, 2016

estebank commented Dec 12, 2016

nagisa commented Dec 12, 2016

estebank commented Dec 13, 2016

nagisa commented Dec 13, 2016

oli-obk commented Dec 13, 2016

estebank commented Dec 14, 2016

nikomatsakis commented Dec 19, 2016

nikomatsakis commented Dec 19, 2016

bors commented Dec 21, 2016

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Dec 21, 2016

Choose a reason for hiding this comment

nikomatsakis Dec 21, 2016

Choose a reason for hiding this comment

nikomatsakis Dec 21, 2016

Choose a reason for hiding this comment

nikomatsakis commented Dec 27, 2016

estebank commented Dec 28, 2016

nikomatsakis commented Dec 30, 2016

nikomatsakis commented Jan 3, 2017

estebank commented Jan 3, 2017

nikomatsakis commented Jan 5, 2017

estebank commented Dec 12, 2016 •

edited

Loading