Some macro errors now include file names into the standard library (JSON).

[ehuss]

The error message for the following has changed so that that JSON spans include file names into the standard library:

struct S;

pub fn f() {
    let x = vec![1,2,3];
    let x = vec![S; x.len()];
}

With JSON output, previously this had a span that looked like this:

// Rust 1.43 output
{
  "byte_end": 60,
  "byte_start": 33,
  "column_end": 61,
  "column_start": 34,
  "expansion": {
    "def_site_span": {
      "byte_end": 198,
      "byte_start": 0,
      "column_end": 55,
      "column_start": 1,
      "expansion": null,
      "file_name": "<::alloc::macros::vec macros>",
      "is_primary": false,
      "label": null,
      "line_end": 3,
      "line_start": 1,
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "text": [
        {
          "highlight_end": 78,
          "highlight_start": 1,
          "text": "($ elem : expr ; $ n : expr) => ($ crate :: vec :: from_elem ($ elem, $ n)) ;"
        },
        {
          "highlight_end": 66,
          "highlight_start": 1,
          "text": "($ ($ x : expr), *) => (< [_] > :: into_vec (box [$ ($ x), *])) ;"
        },
        {
          "highlight_end": 55,
          "highlight_start": 1,
          "text": "($ ($ x : expr,) *) => ($ crate :: vec ! [$ ($ x), *])"
        }
      ]
    },
    "macro_decl_name": "vec!",
    "span": {
      "byte_end": 79,
      "byte_start": 62,
      "column_end": 30,
      "column_start": 13,
      "expansion": null,
      "file_name": "src/lib.rs",
      "is_primary": false,
      "label": null,
      "line_end": 5,
      "line_start": 5,
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "text": [
        {
          "highlight_end": 30,
          "highlight_start": 13,
          "text": "    let x = vec![S; x.len()];"
        }
      ]
    }
  },
  "file_name": "<::alloc::macros::vec macros>",
  "is_primary": true,
  "label": "the trait `std::clone::Clone` is not implemented for `S`",
  "line_end": 1,
  "line_start": 1,
  "suggested_replacement": null,
  "suggestion_applicability": null,
  "text": [
    {
      "highlight_end": 61,
      "highlight_start": 34,
      "text": "($ elem : expr ; $ n : expr) => ($ crate :: vec :: from_elem ($ elem, $ n)) ;"
    }
  ]
},

Pay attention to the "file_name" field, it now includes a span that references macros.rs instead of <::alloc::macros::vec macros>. There are other differences, such as missing text rendering. The new output is:

// Rust 1.44 (nightly) output
{
  "byte_end": 1284,
  "byte_start": 1262,
  "column_end": 31,
  "column_start": 9,
  "expansion": {
    "def_site_span": {
      "byte_end": 1420,
      "byte_start": 1204,
      "column_end": 2,
      "column_start": 1,
      "expansion": null,
      "file_name": "/rustc/38114ff16e7856f98b2b4be7ab4cd29b38bed59a/src/liballoc/macros.rs",
      "is_primary": false,
      "label": null,
      "line_end": 46,
      "line_start": 38,
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "text": []
    },
    "macro_decl_name": "vec!",
    "span": {
      "byte_end": 79,
      "byte_start": 62,
      "column_end": 30,
      "column_start": 13,
      "expansion": null,
      "file_name": "src/lib.rs",
      "is_primary": false,
      "label": null,
      "line_end": 5,
      "line_start": 5,
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "text": [
        {
          "highlight_end": 30,
          "highlight_start": 13,
          "text": "    let x = vec![S; x.len()];"
        }
      ]
    }
  },
  "file_name": "/rustc/38114ff16e7856f98b2b4be7ab4cd29b38bed59a/src/liballoc/macros.rs",
  "is_primary": true,
  "label": "the trait `std::clone::Clone` is not implemented for `S`",
  "line_end": 40,
  "line_start": 40,
  "suggested_replacement": null,
  "suggestion_applicability": null,
  "text": []
},

This is a problem because the path /rustc/38114ff16e7856f98b2b4be7ab4cd29b38bed59a doesn't exist, so there is nothing to point to. Additionally, tools may be expecting the previous behavior of a … macros> value in the "file_name".

Note that this seems particular to using a const expression in the example above (a literal such as 3 doesn't have the same effect).

This seems to have started with #66364 (cc @Centril).

rustc 1.44.0-nightly (38114ff16 2020-03-21)

[spastorino]

pre-triage: prioritized as P-medium given that this is unlikely to break important stuff badly.

@Centril should we assign to you?.

[Centril]

Let's not assign it to me, but let's cc @eddyb and @petrochenkov :)

[eddyb]

This is a problem because the path /rustc/38114ff16e7856f98b2b4be7ab4cd29b38bed59a doesn't exist, so there is nothing to point to.

This is #53486, maybe we should prioritize it?

Additionally, tools may be expecting the previous behavior of a … macros> value in the "file_name".

I hope not, it was always a hack that we've been wanting to get rid for years (#49511) and even used to be even more useless (e.g. <std macros> i.e. crate name, that's why it was macros with an s).

We shouldn't treat this as a regression unless we have some evidence of misuse in the wild.

[petrochenkov]

path /rustc/38114ff16e7856f98b2b4be7ab4cd29b38bed59a doesn't exist

Hmm, I'm pretty sure that I've seen paths like this before #66364, but probably in debuginfo rather than in spans.

It still seems to be better than no paths at all because some tools have an option to remap paths like this to a different directory with actual sources, or /rustc/38114ff16e7856f98b2b4be7ab4cd29b38bed59a can be symlinked to the actual source directory in the worst case.

[ehuss]

We shouldn't treat this as a regression unless we have some evidence of misuse in the wild.

Just a quick perusal of projects:

• Sublime: https://github.com/rust-lang/rust-enhanced/blob/7acf8a9914906efb2431ba475f2148f1c16e2ae4/rust/messages.py#L988-L1007
• RLS: https://github.com/rust-lang/rls/blob/782b7962546ab584097da6d5c80fcff9d4f19c7a/rls/src/actions/diagnostics.rs#L122-L130
• Rust-analyzer: https://github.com/rust-analyzer/rust-analyzer/blob/402bf53a09a3edbb0ff4c94bf22d94d484f81eb9/crates/ra_cargo_watch/src/conv.rs#L34-L36

I imagine there are others. I wouldn't call it "misuse". It is how stable rustc behaves, and tools have to deal with it.

The immediate issue for me, is that I need to find the invocation site. It was previously easy to skip over the macros> entries. Now, I guess I'll need to check if paths exist? I'm not sure, I haven't thought about it too much.

Nor do I understand under which scenarios this happens. If someone could explain in clear terms what the change is, that would be helpful. I can probably update the documentation if this really is a permanent change.

It was also useful for emitting the message "this error originates in a macro outside of the current crate", but now I don't see a way to detect that?

Regardless, this is a breaking change. I'm not so sensitive about them, I can adapt, but it will likely have some impact on other projects, too.

[spastorino]

hmm I wonder if we should bump the priority to P-high then. Thoughts?.

[Centril]

Regardless, this is a breaking change. I'm not so sensitive about them, I can adapt, but it will likely have some impact on other projects, too.

(I want to push back on the notion of "breaking change" here, as it is typically associated with "you broke some stability guarantees", which isn't the case here AFAIK, and it would be pretty dangerous if we would have given such guarantees. That said, it would be nice to make more robust improvements here, but I agree with @petrochenkov re. "better than nothing".)

For now I think P-medium is appropriate.

[eddyb]

The immediate issue for me, is that I need to find the invocation site.

Isn't that what the macro backtrace is for? "def_site_span" is in the macro, while "span" is the invocation, if I'm reading the JSON correctly.

Nor do I understand under which scenarios this happens. If someone could explain in clear terms what the change is, that would be helpful. I can probably update the documentation if this really is a permanent change.

The change is that the <... macros> hack has been completely removed and now Spans pointing to the definition of a macro are identical in the same-crate and cross-crate cases. This should work great for anything built locally by Cargo, where the source is available anyway.

No tools should've relied on macros> to detect macros, but rather the actual macro backtrace, which would've worked same-crate before the change. The only non-misuse of macros> detection is to ignore it as a non-real file.

Paths starting with e.g. /rustc/38114ff16e7856f98b2b4be7ab4cd29b38bed59a is an orthogonal problem, further exposed by this change, hence my suggestion that we should prioritize fixing that.

It was also useful for emitting the message "this error originates in a macro outside of the current crate", but now I don't see a way to detect that?

"crate" is likely the wrong granularity here, if you have the ability you should probably do this for "outside of the current workspace" instead.
The compiler itself can't, and relies on a flag (that's not exposed in the JSON) which indicates that the file wasn't loaded by this crate but by a dependency.

[pnkfelix]

@rustbot assign @eddyb

I'm not quite sure how this is going to be resolved (it may be that the tools are working as expected, or at least within the bounds of what users can expect), but I'm letting eddy take charge of resolution one way or another.

[ehuss]

I think the resolution here is that I'll wait for #70642 to land, and then I'll send a PR to update the documentation.

I think at a minimum this should be included in the release notes. It would be nice to try to communicate this to tool developers, but I don't know if there is a single way to do that, maybe an internals post?