Skip to content

/rust

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update spans that live in std macros to their use sites #35688

Closed
wants to merge 2 commits into from

Conversation

Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@jonathandturner @nikomatsakis
@jonathandturner
Copy link
Contributor

jonathandturner commented Aug 15, 2016 •
edited

This PR updates the error reporting to detect when a span used in the error message will be labeling a span in the <std macros>. If it detects this, it will update it to the use site, if possible.

Before:

screen shot 2016-08-15 at 10 20 33 am

After:

screen shot 2016-08-15 at 10 19 56 am

To help with the messages that may not become confusing because they will be referring to a definition inside of <std macros> we also add a note to help understand that we're using the use but the definition is in <std macros>

Another before:

screen shot 2016-08-15 at 11 55 01 am

And after:

screen shot 2016-08-15 at 11 54 41 am

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis Aug 15, 2016

@jonathandturner jonathandturner force-pushed the jonathandturner:repair_spans_in_std_macros branch from 8488b60 to 77e5f3c Aug 15, 2016

Jonathan Turner
Update failing test case
Loading status checks…
030287a
@jonathandturner

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jonathandturner commented Aug 15, 2016

@nikomatsakis - Realizing this will only update the error that is displayed but not the json errors. That will need to be updated before this can be merged.
@jonathandturner

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jonathandturner commented Aug 15, 2016

@nikomatsakis actually, scratch that. The same information is available in both the json and the presentation, I'm just using a different presentation of it.

Consumers of the json would still be able to recreate this output using the backtrace information. Using eg:

            "span" : {
               "line_end" : 13,
               "byte_start" : 498,
               "label" : null,
               "file_name" : "src/test/ui/codemap_tests/bad-format-args.rs",
               "text" : [
                  {
                     "highlight_start" : 5,
                     "highlight_end" : 19,
                     "text" : "    format!(\"\" 1);"
                  }
               ],

From:

{
   "message" : "expected token: `,`",
   "level" : "error",
   "rendered" : null,
   "children" : [],
   "code" : null,
   "spans" : [
      {
         "column_end" : 59,
         "is_primary" : true,
         "column_start" : 28,
         "expansion" : {
            "span" : {
               "line_end" : 13,
               "byte_start" : 498,
               "label" : null,
               "file_name" : "src/test/ui/codemap_tests/bad-format-args.rs",
               "text" : [
                  {
                     "highlight_start" : 5,
                     "highlight_end" : 19,
                     "text" : "    format!(\"\" 1);"
                  }
               ],
               "expansion" : null,
               "column_start" : 5,
               "suggested_replacement" : null,
               "byte_end" : 512,
               "line_start" : 13,
               "column_end" : 19,
               "is_primary" : false
            },
            "def_site_span" : {
               "file_name" : "<std macros>",
               "text" : [
                  {
                     "highlight_start" : 1,
                     "highlight_end" : 28,
                     "text" : "( $ ( $ arg : tt ) * ) => ("
                  },
                  {
                     "highlight_start" : 1,
                     "highlight_end" : 63,
                     "text" : "$ crate :: fmt :: format ( format_args ! ( $ ( $ arg ) * ) ) )"
                  }
               ],
               "label" : null,
               "byte_start" : 2660,
               "line_end" : 2,
               "is_primary" : false,
               "column_end" : 63,
               "line_start" : 1,
               "byte_end" : 2750,
               "expansion" : null,
               "column_start" : 1,
               "suggested_replacement" : null
            },
            "macro_decl_name" : "format!"
         },
         "suggested_replacement" : null,
         "byte_end" : 2746,
         "line_start" : 2,
         "text" : [
            {
               "text" : "$ crate :: fmt :: format ( format_args ! ( $ ( $ arg ) * ) ) )",
               "highlight_start" : 28,
               "highlight_end" : 59
            }
         ],
         "file_name" : "<std macros>",
         "line_end" : 2,
         "byte_start" : 2715,
         "label" : null
      }
   ]
}

@jonathandturner jonathandturner referenced this pull request Aug 15, 2016

Merged

Replace macro backtraces with labeled local uses #35702

src/librustc_errors/emitter.rs
self.emit_messages_default(db);
let mut primary_span = db.span.clone();
let mut children = db.children.clone();
self.fix_multispans_in_std_macros(&mut primary_span, &mut children);

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Aug 16, 2016

Contributor

man, this is unfortunate.
src/librustc_errors/emitter.rs
let sub_result = self.get_multispan_max_line_num(&sub.span);
max = if sub_result > max { primary } else { max };
}
max
}

fn fix_multispan_in_std_macros(&mut self, span: &mut MultiSpan) -> bool {

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Aug 16, 2016

Contributor

Nit: This could use a comment explaining what it is aiming to do
src/librustc_errors/emitter.rs
let v = cm.macro_backtrace(sp.clone());
if let Some(use_site) = v.last() {
before_after.push((sp.clone(), use_site.call_site.clone()));
};

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Aug 16, 2016

Contributor

I guess this trailing semicolon is not needed ?
src/libsyntax_pos/lib.rs
@@ -221,6 +221,25 @@ impl MultiSpan {
&self.primary_spans
}

/// Replaces all occurances of one Span with another. Used to move Spans in areas that don't
/// display well (like std macros). Returns true of replacements occurred.

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Aug 16, 2016

Contributor

Nit: s/of/if/
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Aug 16, 2016

r=me with nits addressed
@jonathandturner

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jonathandturner commented Aug 16, 2016

Closing this one in lieu of the more complete #35702

@jonathandturner jonathandturner closed this Aug 16, 2016

bors added a commit that referenced this pull request Aug 18, 2016

@bors
Auto merge of #35702 - jonathandturner:add_backtrace_labels, r=nikoma… 
…tsakis

Replace macro backtraces with labeled local uses

This PR (which builds on #35688) follows from the conversations on how best to [handle the macro backtraces](https://internals.rust-lang.org/t/improving-macro-errors/3809).  The feeling there was that there were two different "groups" of users.

The first group, the macro users, rarely (and likely never) want to see the macro backtrace.  This is often more confusing to users as it will be talking about code they didn't write.

The second group, the macro writers, are trying to debug a macro.  They'll want to see something of the backtrace so that they can see where it's going wrong and what the steps were to get there.

For the first group, it seems clear that we don't want to show *any* macro backtrace.  For the second group, we want to show enough to help the macro writer.

This PR uses a heuristic.  It will only show any backtrace steps if they are in the same crate that is being compiled.  This keeps errors in foreign crates from showing to users that didn't need them.

Additionally, in asking around I repeated heard that the middle steps of the backtrace are rarely, if ever, used in practice.  This PR takes and applies this knowledge.  Now, instead of a full backtrace, the user is given the error underline inside of a local macro as well as the use site as a secondary label.  This effectively means seeing the root of the error and the top of the backtrace, eliding the middle steps.

Rather than being the "perfect solution", this PR opts to take an incremental step towards a better experience.  Likely it would help to have additional macro debugging tools, as they could be much more verbose than we'd likely want to use in the error messages themselves.

Some examples follow.

**Example 1**

Before:

<img width="1275" alt="screen shot 2016-08-15 at 4 13 18 pm" src="https://cloud.githubusercontent.com/assets/547158/17682828/3948cea2-6303-11e6-93b4-b567e9d62848.png">

After:

<img width="596" alt="screen shot 2016-08-15 at 4 13 03 pm" src="https://cloud.githubusercontent.com/assets/547158/17682832/3d670d8c-6303-11e6-9bdc-f30a30bf11ac.png">

**Example 2**

Before:

<img width="918" alt="screen shot 2016-08-15 at 4 14 35 pm" src="https://cloud.githubusercontent.com/assets/547158/17682870/722225de-6303-11e6-9175-336a3f7ce308.png">

After:

<img width="483" alt="screen shot 2016-08-15 at 4 15 01 pm" src="https://cloud.githubusercontent.com/assets/547158/17682872/7582cf6c-6303-11e6-9235-f67960f6bd4c.png">
169b612

@jonathandturner jonathandturner deleted the jonathandturner:repair_spans_in_std_macros branch Aug 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.