make JSON error byte position start at top of file

[zackmdavis]

The hi and lo offsets in a span are relative to a CodeMap, but this
doesn't seem to be terribly useful for tool consumers who don't have the
codemap, but might want the byte offset within an actual file?

I couldn't get @killercup's example to run, perhaps due to the limitations of the merely-stage-1 compiler that I built (error was libproc_macro-456500c7095d8fbe.so: cannot open shared object file: No such file or directory)??—but a dummy project confirms that the byte offsets have successfully been changed to be file-relative—

Before:

$ cargo run --message-format json
Compiling byte_json v0.1.0 (file:///home/ubuntu/byte_json)
{"message":{"children":[{"children":[],"code":null,"level":"note","message":"#[warn(dead_code)] on by default","rendered":null,"spans":[]}],"code":null,"level":"warning","message":"function is never used: `rah`","rendered":null,"spans":[{"byte_end":100,"byte_start":67,"column_end":2,"column_start":1,"expansion":null,"file_name":"src/foo.rs","is_primary":true,"label":null,"line_end":5,"line_start":3,"suggested_replacement":null,"text":[{"highlight_end":11,"highlight_start":1,"text":"fn rah() {"},{"highlight_end":21,"highlight_start":1,"text":" println!(\"rah!\")"},{"highlight_end":2,"highlight_start":1,"text":"}"}]}]},"package_id":"byte_json 0.1.0 (path+file:///home/ubuntu/byte_json)","reason":"compiler-message","target":{"crate_types":["bin"],"kind":["bin"],"name":"byte_json","src_path":"/home/ubuntu/byte_json/src/main.rs"}}
{"message":{"children":[{"children":[],"code":null,"level":"note","message":"#[warn(dead_code)] on by default","rendered":null,"spans":[]}],"code":null,"level":"warning","message":"function is never used: `alas`","rendered":null,"spans":[{"byte_end":137,"byte_start":102,"column_end":2,"column_start":1,"expansion":null,"file_name":"src/bar.rs","is_primary":true,"label":null,"line_end":3,"line_start":1,"suggested_replacement":null,"text":[{"highlight_end":12,"highlight_start":1,"text":"fn alas() {"},{"highlight_end":22,"highlight_start":1,"text":" println!(\"alas\");"},{"highlight_end":2,"highlight_start":1,"text":"}"}]}]},"package_id":"byte_json 0.1.0 (path+file:///home/ubuntu/byte_json)","reason":"compiler-message","target":{"crate_types":["bin"],"kind":["bin"],"name":"byte_json","src_path":"/home/ubuntu/byte_json/src/main.rs"}}
{"features":[],"filenames":["/home/ubuntu/byte_json/target/debug/byte_json"],"fresh":false,"package_id":"byte_json 0.1.0 (path+file:///home/ubuntu/byte_json)","profile":{"debug_assertions":true,"debuginfo":2,"opt_level":"0","overflow_checks":true,"test":false},"reason":"compiler-artifact","target":{"crate_types":["bin"],"kind":["bin"],"name":"byte_json","src_path":"/home/ubuntu/byte_json/src/main.rs"}}
Finished dev [unoptimized + debuginfo] target(s) in 0.36 secs
Running `target/debug/byte_json`
Hello, world!

After:

$ RUSTC=../rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc cargo run --message-format json
Compiling byte_json v0.1.0 (file:///home/ubuntu/byte_json)
{"message":{"children":[{"children":[],"code":null,"level":"note","message":"#[warn(dead_code)] on by default","rendered":null,"spans":[]}],"code":null,"level":"warning","message":"function is never used: `rah`","rendered":null,"spans":[{"byte_end":35,"byte_start":2,"column_end":2,"column_start":1,"expansion":null,"file_name":"src/foo.rs","is_primary":true,"label":null,"line_end":5,"line_start":3,"suggested_replacement":null,"text":[{"highlight_end":11,"highlight_start":1,"text":"fn rah() {"},{"highlight_end":21,"highlight_start":1,"text":" println!(\"rah!\")"},{"highlight_end":2,"highlight_start":1,"text":"}"}]}]},"package_id":"byte_json 0.1.0 (path+file:///home/ubuntu/byte_json)","reason":"compiler-message","target":{"crate_types":["bin"],"kind":["bin"],"name":"byte_json","src_path":"/home/ubuntu/byte_json/src/main.rs"}}
{"message":{"children":[{"children":[],"code":null,"level":"note","message":"#[warn(dead_code)] on by default","rendered":null,"spans":[]}],"code":null,"level":"warning","message":"function is never used: `alas`","rendered":null,"spans":[{"byte_end":35,"byte_start":0,"column_end":2,"column_start":1,"expansion":null,"file_name":"src/bar.rs","is_primary":true,"label":null,"line_end":3,"line_start":1,"suggested_replacement":null,"text":[{"highlight_end":12,"highlight_start":1,"text":"fn alas() {"},{"highlight_end":22,"highlight_start":1,"text":" println!(\"alas\");"},{"highlight_end":2,"highlight_start":1,"text":"}"}]}]},"package_id":"byte_json 0.1.0 (path+file:///home/ubuntu/byte_json)","reason":"compiler-message","target":{"crate_types":["bin"],"kind":["bin"],"name":"byte_json","src_path":"/home/ubuntu/byte_json/src/main.rs"}}
{"features":[],"filenames":["/home/ubuntu/byte_json/target/debug/byte_json"],"fresh":false,"package_id":"byte_json 0.1.0 (path+file:///home/ubuntu/byte_json)","profile":{"debug_assertions":true,"debuginfo":2,"opt_level":"0","overflow_checks":true,"test":false},"reason":"compiler-artifact","target":{"crate_types":["bin"],"kind":["bin"],"name":"byte_json","src_path":"/home/ubuntu/byte_json/src/main.rs"}}
Finished dev [unoptimized + debuginfo] target(s) in 2.59 secs
Running `target/debug/byte_json`
Hello, world!

Resolves#35164.

r? @jonathandturner

[killercup]

Nice! I'll build this branch and give it a try. I'd really like there to be a test for this as well :)

[killercup]

So just half an hour after compiling this branch I have good news: My example now outputs

Worked.
Worked.

🎉

[shepmaster]

Thank you @zackmdavis! We will have @jonathandturner or a equally qualified reviewer take a look soon.

[sophiajt]

@shepmaster - Since I haven't touched this area in a while (or done much with the compiler), may want to hand it to one of the compiler team folks.

[Mark-Simulacrum]

r? @jseyfried or @petrochenkov perhaps

[jseyfried]

LGTM, but r? @nrc

[zackmdavis]

(I wonder if it would be uncharitable for me to speculate that a diffusion of responsibility effect is making this PR subjectively feel more risky than it actually is? The current behavior, in which JSON errors can contain byte_start and byte_end values outside the byte range of the file, can't possibly be correct ...)

[aidanhs]

@zackmdavis diffusion of responsibility is one of the reasons the infra team goes through PRs to make sure none of them get left behind, pinging reviewers when appropriate.

Reviewers may cc each other for a whole host of reasons - perhaps someone familiar with the area will realise there are other related pieces that need fixing to make this fix even better.

In short - don't worry, we'll get there eventually!

[carols10cents]

friendly ping to keep this on your radar @nrc!

[nrc]

I approve of this change, however, we have a back compat guarantee and this is technically a breaking change. I see two paths forward - we leave the existing fields and add new ones for file_relative_byte_start, etc. Or we regard this as a bug fix and change these fields. Ideally we would do a Crater run of a PR with these fields removed, but I fear this will not catch any breakage. I would like to be re-assured that these fields weren't being used in their current form.

Nominated for discussion at the dev-tools meeting today.

Sorry I missed the review notification for this.

[nrc]

Discussed at the dev-tools meeting. We thought it would be fine to make this change, assuming that it is not widely used, and the only way to use the current offsets is to use unstable code (libsyntax). As far as we know, @killercup is the only user.

I would like to see a test please!

[zackmdavis]

I would like to see a test please!

Do we have an established way to test JSON error output? (Grepping around in the test/ directory doesn't look promising.) The comments-in-run-pass/-fail files" style isn't applicable, and a UI test (which I would assume would work given a compile-flags: --error-output json directive comment) seems unnecessarily brittle (equality of JSON objects is what matters, not byte-for-byte output).

[nrc]

@zackmdavis you could make a run-make test, that would let you specify the json error type and compare the results however you like. You might also be able to write a #[test] unit test.

[zackmdavis]

9ddda12 adds a run-make test.

(The test fails without this patch, with a reported byte start (respectively end) of 76 (respectively 82), reflecting the 52 bytes of main.rs coming earlier in the CodeMap than where the lint error occurs.)

[nrc]

Looks like tests are failing due to missing licenses:

[00:02:36] tidy error: /checkout/src/test/run-make/issue-35164/submodule/mod.rs: incorrect license
[00:02:36] tidy error: /checkout/src/test/run-make/issue-35164/main.rs: incorrect license

[nrc]

Looks good other than the licenses (r+)

[zackmdavis]

Force-pushed 6a58949, adding the licenses. I regret the error.

[Mark-Simulacrum]

@bors r=nrc

[bors]

📌 Commit 6a58949 has been approved by nrc

[bors]

🔒 Merge conflict

[zackmdavis]

(rebased)

[Mark-Simulacrum]

@bors r=nrc

[bors]

📌 Commit bb2b863 has been approved by nrc

[bors]

⌛ Testing commit bb2b863 with merge 68be86d...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 68be86d to master...