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

estebank · 2017-04-11T11:43:22Z

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

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: #35965, #38246. Follow up to #38328.

r? @jonathandturner

estebank · 2017-04-11T11:43:48Z

It's missing tests.

nagisa · 2017-04-11T12:29:17Z

Can we avoid manually tweaking spans like this? Historically this always have been a source of weird bugs.

estebank · 2017-04-11T18:09:12Z

@nagisa from what I gather, there are a few ways of accomplishing the end goal of smaller diagnostic spans, all with problems:

Change the current span to point at parts of the code that are relevant only for diagnostics. As you pointed out, this is incorrect.
Add a new diagnostic_span field to the every Item and element of the AST and HIR that contains the span to be used for diagnostics. This can be the same as the span field, but would point at the declaration for methods and ADTs. This would duplicate the number of spans in use in the compiler with the consequent memory increase. I'm going down that road and the diff keeps getting larger and larger. I'm not liking how it's going.
Make the name field Spanned, but we sometimes build items where the span is not available. This also duplicates the amount of spans.
What this PR is doing. Because it has to be invoked at the diagnostic emission point, the impact is incremental only in cases where we're fine with showing something else than what we're showing now. I didn't attempt to do any parsing to minimize the chance of errors, taking everything until the first brace seemed like a good heuristic for structs, enums and fns.

@nikomatsakis, I just checked what happens with macros with this change and there's no detrimental difference:

Before:

$ rustc file2.rs
error[E0072]: recursive type `Foo` has infinite size
 --> file2.rs:3:9
  |
3 |           struct $n {
  |  _________^ starting here...
4 | |             field: $n
5 | |         }
  | |_________^ ...ending here: recursive type has infinite size
...
9 |   mk_struct!(Foo);
  |   ---------------- in this macro invocation

After:

$ ./stage1/bin/rustc file2.rs
error[E0072]: recursive type `Foo` has infinite size
 --> file2.rs:3:9
  |
3 |         struct $n {
  |         ^^^^^^^^^ recursive type has infinite size
...
9 | mk_struct!(Foo);
  | ---------------- in this macro invocation

I'm also thinking that if, for example, a fn's signature spans multiple lines, then the compiler should then probably just use the full multiline span instead of using a multiline span:

Instead of:

error[E0072]: recursive type `A` has infinite size
 --> file2.rs:1:1
  |
1 |   struct
  |  _^ starting here...
2 | | A
  | |_^ ...ending here: recursive type has infinite size

given that you're already displaying a multiline span, just go with the current output:

error[E0072]: recursive type `A` has infinite size
 --> file2.rs:1:1
  |
1 |   struct
  |  _^ starting here...
2 | | A
3 | | {
4 | |     a: A,
5 | | }
  | |_^ ...ending here: recursive type has infinite size

```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 ```

arielb1 · 2017-04-12T16:00:08Z

Maybe there should be a way to run the LALR parser on spans and match on the concrete syntax tree?

sophiajt · 2017-04-13T00:03:32Z

@arielb1 and @nagisa - looks like you're both on it, feel free to take the review bit

arielb1 · 2017-04-18T09:08:21Z

I'll rather have one of the parser guys look at this.

r? @petrochenkov

arielb1 · 2017-04-18T09:10:29Z

Personally, I think we should have a "short span" for each item that probably consists of only the ident, and e.g. store it in metadata. I'm not afraid of performance - we don't have that much items.

nikomatsakis · 2017-04-18T13:23:35Z

Personally, I think we should have a "short span" for each item that probably consists of only the ident, and e.g. store it in metadata. I'm not afraid of performance - we don't have that much items.

I'm inclined to agree with this, though I've not looked over this PR, which seems like a potentially cute approach to the problem. (At least, having a "short span" for each item was the way I expected us to address this problem.)

nikomatsakis · 2017-04-18T19:56:51Z

Still, this PR seems to work -- I'd love to reduce our reliance on fancy ASCII art :) -- so I'd be happy to accept it, if we feel that it's sufficiently robust around macros etc (which I admit isn't entirely clear to me).

estebank · 2017-04-18T20:51:40Z

Personally, I think we should have a "short span" for each item that probably consists of only the ident, and e.g. store it in metadata.

Fair enough, but that will be a much larger PR that I won't be tackling

I'm not afraid of performance - we don't have that much items.

I'd like to measure memory usage before and after that change before categorically saying it has negligible impact on projects the size of rustc :)

having a "short span" for each item was the way I expected us to address this problem.

I started doing that while on the road, and after severals hours of treading water I tried this approach which got me a huge bang for the buck (literally two --stage 1 compilations in order to get the current output).

if we feel that it's sufficiently robust around macros

I'd be happy to add more test cases to make sure this behaves correctly, but I feel confident about this not breaking things as it needs to be added on each diagnostic creation/call explicitly and I haven't come up with any part of the grammar that would use { in the middle of a span we wouldn't want to break up.

petrochenkov · 2017-04-19T14:13:15Z

@estebank
I think spans for item names in AST/HIR (with fall-back to whole item spans) would be the best solution.

I'd like to measure memory usage before and after that change before categorically saying it has negligible impact on projects the size of rustc :)

Here are some stats from adding spans to path segments. The number of items is order of magnitude smaller than number of path segments, so new spans for items shouldn't be a problem.

This PR is a hack and the heuristics are imperfect, e.g. multiline diagnostics for

struct 
ListNode

are still bad. I also don't know how exactly spans interact with macros, @jseyfried is the specialist here.

However, multiline spans are horrible and turn something that is supposed to be useful - error messages, into something harmful - long and unreadable ascii soup, especially if there are many of them, and this PR mostly fixes the problem. So I'm ready to r+ this if the proper solution is going to be implemented in the future.

petrochenkov · 2017-04-19T14:23:44Z

@estebank

long and unreadable ascii soup, especially if there are many of them

[meta] I actually have similar sentiment with regards to new helps and notes that continue to be actively added to the compiler.
Quickly going through the error list and fixing everything becomes a problem.
There's so much help you can't see errors behind it!

estebank · 2017-04-19T17:05:25Z

The number of items is order of magnitude smaller than number of path segments, so new spans for items shouldn't be a problem.

Fair enough, @petrochenkov.

This PR is a hack and the heuristics are imperfect, e.g. multiline diagnostics for
 struct 
 ListNode
are still bad.

What would appropriate output for struct\nListNode be?

It used to be

error[E0072]: recursive type `ListNode` has infinite size
 --> file2.rs:1:1
  |
1 | struct
  | ^ recursive type has infinite size

Which I feel lacked all and any context, now it is

error[E0072]: recursive type `ListNode` has infinite size
 --> file2.rs:1:1
  |
1 |   struct
  |  _^ starting here...
2 | | ListNode
3 | | {
... |
9 | | }
  | |_^ ...ending here: recursive type has infinite size

which is admittedly verbose. And after cleaning up #41245 it should look like this:

error[E0072]: recursive type `ListNode` has infinite size
 --> file2.rs:1:1
  |
1 | / struct
2 | | ListNode
3 | | {
... |
9 | | }
  | |_^ recursive type has infinite size

or

error[E0072]: recursive type `ListNode` has infinite size
 --> file2.rs:1:1
  |
1 | / struct
2 | | ListNode
  | |________^ recursive type has infinite size

However, multiline spans are horrible and turn something that is supposed to be useful - error messages, into something harmful - long and unreadable ascii soup, especially if there are many of them

I agree that multiline spans are overused at the moment, mainly because I added a way to display them before adding better spans for the errors. Multiline spans should be sparsely seen under normal circumstances. This is an attempt to go in that direction. I particularly find the lifetime errors quite galling (from #41343):

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter 'a in generic type due to conflicting requirements
  --> <anon>:11:5
   |
11 |       fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |  _____^ starting here...
12 | |         vec.get(0)
13 | |     }
   | |_____^ ...ending here
   |
note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the body at 11:60...
  --> <anon>:11:61
   |
11 |       fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |  _____________________________________________________________^ starting here...
12 | |         vec.get(0)
13 | |     }
   | |_____^ ...ending here
note: ...so that method type is compatible with trait (expected fn(&Foo, &std::vec::Vec<u32>) -> std::option::Option<&u32>, found fn(&Foo, &std::vec::Vec<u32>) -> std::option::Option<&u32>)
  --> <anon>:11:5
   |
11 |       fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |  _____^ starting here...
12 | |         vec.get(0)
13 | |     }
   | |_____^ ...ending here
note: but, the lifetime must be valid for the anonymous lifetime #1 defined on the body at 11:60...
  --> <anon>:11:61
   |
11 |       fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |  _____________________________________________________________^ starting here...
12 | |         vec.get(0)
13 | |     }
   | |_____^ ...ending here
note: ...so that method type is compatible with trait (expected fn(&Foo, &std::vec::Vec<u32>) -> std::option::Option<&u32>, found fn(&Foo, &std::vec::Vec<u32>) -> std::option::Option<&u32>)
  --> <anon>:11:5
   |
11 |       fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |  _____^ starting here...
12 | |         vec.get(0)
13 | |     }
   | |_____^ ...ending here

error: aborting due to previous error

If the couple of PRs I'm working on land, it would look like this instead:

error[E0495]: cannot infer an appropriate lifetime for lifetime parameter 'a in generic type due to conflicting requirements
  --> <anon>:11:5
   |
11 |     fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the body at 11:60...
  --> <anon>:11:61
   |
11 |       fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |  _____________________________________________________________^
12 | |         vec.get(0)
13 | |     }
   | |_____^
note: ...so that method type is compatible with trait (expected fn(&Foo, &std::vec::Vec<u32>) -> std::option::Option<&u32>, found fn(&Foo, &std::vec::Vec<u32>) -> std::option::Option<&u32>)
  --> <anon>:11:5
   |
11 |     fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: but, the lifetime must be valid for the anonymous lifetime #1 defined on the body at 11:60...
  --> <anon>:11:61
   |
11 |       fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |  _____________________________________________________________^
12 | |         vec.get(0)
13 | |     }
   | |_____^
note: ...so that method type is compatible with trait (expected fn(&Foo, &std::vec::Vec<u32>) -> std::option::Option<&u32>, found fn(&Foo, &std::vec::Vec<u32>) -> std::option::Option<&u32>)
  --> <anon>:11:5
   |
11 |     fn bar<'a>(&self, vec: &'a Vec<u32>) -> Option<&'a u32> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

Which I feel is a slight improvement. (This diagnostic is still opaque for other reasons.)

There's so much help you can't see errors behind it!

Throwing too much text at the user is definitely a problem, but there are many cases where the error without the help information would be too vague to be of much use. Should we open a ticket, internals thread or rust-lang RFC to track the task of trimming the output of different errors that show too much text unnecessarily?

By the way, multiline spans are visible now more often: they show up always on diagnostics regardless of length, showing the first 4 lines and the last 2 instead of showing only if the span is shorter than 9 lines. This could also be shortened to maybe the first 2 lines and last 2 instead.

nikomatsakis · 2017-04-19T20:26:53Z

My take so far is that @estebank is correct -- we should land this, and open a FIXME or something to thread "short spans" the 'right' way. I think cases like struct \n Foo are pretty unrealistic, but I'd be curious to know what happens with (e.g.) #[derive(...)] \n struct Foo { ... } -- or things like fn foo(...)\n{. It seems to me we could address that last one by searching not just for a { but also a ( (or <).

(Mainly, I'd like to see improvement, and this seems like a pretty simple approach to get eliminate the worst cases.)

petrochenkov · 2017-04-19T21:13:02Z

@estebank
Could you open an issue for "short spans"?
Otherwise @bors r+

bors · 2017-04-19T21:13:03Z

📌 Commit 439ff69 has been approved by petrochenkov

estebank · 2017-04-19T23:00:23Z

@petrochenkov would the scope of the ticket be covered by one of #33961, #35965 or #38246?

bors · 2017-04-20T04:36:20Z

⌛ Testing commit 439ff69 with merge bf38a4b...

frewsxcv · 2017-04-20T04:39:52Z

@bors retry - prioritizing rollup

…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

Rollup of 5 pull requests - Successful merges: #41214, #41369, #41377, #41378, #41390 - Failed merges:

petrochenkov · 2017-04-20T06:56:08Z

@estebank

would the scope of the ticket be covered by one of #33961, #35965 or #38246?

#33961 looks appropriate, #35965 and #38246 can be closed by this PR I think.

rust-highfive assigned sophiajt Apr 11, 2017

estebank force-pushed the less-multiline branch 3 times, most recently from 60631ed to 033332b Compare April 11, 2017 22:39

estebank force-pushed the less-multiline branch from 033332b to 439ff69 Compare April 12, 2017 08:11

estebank mentioned this pull request Apr 12, 2017

Reduce visual clutter of multiline start when possible #41245

Merged

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2017

arielb1 assigned petrochenkov and unassigned sophiajt Apr 18, 2017

frewsxcv mentioned this pull request Apr 20, 2017

Rollup of 6 pull requests #41412

Closed

frewsxcv mentioned this pull request Apr 20, 2017

Rollup of 5 pull requests #41413

Merged

bors added a commit that referenced this pull request Apr 20, 2017

Auto merge of #41413 - frewsxcv:rollup, r=frewsxcv

fa6b50f

Rollup of 5 pull requests - Successful merges: #41214, #41369, #41377, #41378, #41390 - Failed merges:

bors merged commit 439ff69 into rust-lang:master Apr 20, 2017

This was referenced May 30, 2017

E0072 needs better underline #35965

Closed

Multiline highlights in diagnostic get noisy quickly #38246

Closed

estebank deleted the less-multiline branch November 9, 2023 05:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

estebank commented Apr 11, 2017 •

edited

Loading

estebank commented Apr 11, 2017

nagisa commented Apr 11, 2017

estebank commented Apr 11, 2017

arielb1 commented Apr 12, 2017 •

edited

Loading

sophiajt commented Apr 13, 2017

arielb1 commented Apr 18, 2017

arielb1 commented Apr 18, 2017

nikomatsakis commented Apr 18, 2017

nikomatsakis commented Apr 18, 2017

estebank commented Apr 18, 2017

petrochenkov commented Apr 19, 2017 •

edited

Loading

petrochenkov commented Apr 19, 2017

estebank commented Apr 19, 2017

nikomatsakis commented Apr 19, 2017 •

edited

Loading

petrochenkov commented Apr 19, 2017

bors commented Apr 19, 2017

estebank commented Apr 19, 2017

bors commented Apr 20, 2017

frewsxcv commented Apr 20, 2017

petrochenkov commented Apr 20, 2017

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

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

Conversation

estebank commented Apr 11, 2017 • edited Loading

estebank commented Apr 11, 2017

nagisa commented Apr 11, 2017

estebank commented Apr 11, 2017

arielb1 commented Apr 12, 2017 • edited Loading

sophiajt commented Apr 13, 2017

arielb1 commented Apr 18, 2017

arielb1 commented Apr 18, 2017

nikomatsakis commented Apr 18, 2017

nikomatsakis commented Apr 18, 2017

estebank commented Apr 18, 2017

petrochenkov commented Apr 19, 2017 • edited Loading

petrochenkov commented Apr 19, 2017

estebank commented Apr 19, 2017

nikomatsakis commented Apr 19, 2017 • edited Loading

petrochenkov commented Apr 19, 2017

bors commented Apr 19, 2017

estebank commented Apr 19, 2017

bors commented Apr 20, 2017

frewsxcv commented Apr 20, 2017

petrochenkov commented Apr 20, 2017

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

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

estebank commented Apr 11, 2017 •

edited

Loading

arielb1 commented Apr 12, 2017 •

edited

Loading

petrochenkov commented Apr 19, 2017 •

edited

Loading

nikomatsakis commented Apr 19, 2017 •

edited

Loading