Skip to content
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

Move the JSON error emitter to librustc_errors #66427

Merged
merged 4 commits into from
Nov 16, 2019

Conversation

Mark-Simulacrum
Copy link
Member

This is done both as a cleanup (it makes little sense for this emitter to be in libsyntax), but also as part of broader work to decouple Session from librustc itself.

Along the way, this also moves SourceMap to syntax_pos, which is also nice for the above reasons, as well as allowing dropping the SourceMapper trait from code. This had the unfortunate side-effect of moving FatalError to rustc_data_structures (it's needed in syntax_pos, due to SourceMap, but putting it there feels somehow worse).

@Mark-Simulacrum
Copy link
Member Author

cc @Centril in case you've already been doing something similar with your breaking up libsyntax work

cc @estebank (not sure if anyone else should be pinged) for the JSON emitter changing locations -- no functional changes though

@rust-highfive

This comment has been minimized.

@alexcrichton
Copy link
Member

r? @estebank

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2019
@Centril
Copy link
Contributor

Centril commented Nov 15, 2019

This had the unfortunate side-effect of moving FatalError to rustc_data_structures (it's needed in syntax_pos, due to SourceMap, but putting it there feels somehow worse).

Hmm; it seems to me that syntax_pos is better than rustc_data_structures as the latter feels like a place solely for "in-house generic data structures which could be useful outside rustc as well". Indeed, the module docs for that crate state that the crate is for:

Various data structures used by the Rust compiler. The intention is that code in here should be not be specific to rustc, so that it can be easily unit tested and so forth.

@Centril
Copy link
Contributor

Centril commented Nov 15, 2019

This is done both as a cleanup (it makes little sense for this emitter to be in libsyntax), but also as part of broader work to decouple Session from librustc itself.

The motivation behind this change seems quite sensible. 👍

@Mark-Simulacrum
Copy link
Member Author

Yeah, I'm not a fan of the rustc_data_structures locale -- I can definitely move it to syntax_pos. I mostly decided to just give up and move it "as far back as possible" -- maybe the right strategy is to create a rustc_utils crate or something. I think for now I'll move it to syntax_pos though when I get a chance to edit this branch.

This is a bit unfortunate, but code needs to be able to fatally error
early on (in particular, syntax_pos after we move SourceMap there). It's
also a tiny bit of code, which means it's ultimately not that bad.
This does not update the use sites or delete the now unnecessary
SourceMapper trait, to allow git to interpret the file move as a rename
rather than a new file.
SourceMap is now in the root of all rustc-specific crates, syntax_pos,
so there's no need for the trait object to decouple the dependencies
between librustc_errors and libsyntax as was needed previously.
@Mark-Simulacrum
Copy link
Member Author

Okay I split up the move of SourceMap and git now properly detects it as a rename of the file (though the intermediate commit won't build).

Also changed the first commit to move FatalError to syntax_pos into a module (very creatively named fatal_error...)

@Centril
Copy link
Contributor

Centril commented Nov 15, 2019

r? @Centril

@rust-highfive rust-highfive assigned Centril and unassigned estebank Nov 15, 2019
@Centril
Copy link
Contributor

Centril commented Nov 15, 2019

Looks great; thanks! @bors r+

@bors
Copy link
Contributor

bors commented Nov 15, 2019

📌 Commit c31a875 has been approved by Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2019
Centril added a commit to Centril/rust that referenced this pull request Nov 15, 2019
Move the JSON error emitter to librustc_errors

This is done both as a cleanup (it makes little sense for this emitter to be in libsyntax), but also as part of broader work to decouple Session from librustc itself.

Along the way, this also moves SourceMap to syntax_pos, which is also nice for the above reasons, as well as allowing dropping the SourceMapper trait from code. This had the unfortunate side-effect of moving `FatalError` to rustc_data_structures (it's needed in syntax_pos, due to SourceMap, but putting it there feels somehow worse).
Centril added a commit to Centril/rust that referenced this pull request Nov 15, 2019
Move the JSON error emitter to librustc_errors

This is done both as a cleanup (it makes little sense for this emitter to be in libsyntax), but also as part of broader work to decouple Session from librustc itself.

Along the way, this also moves SourceMap to syntax_pos, which is also nice for the above reasons, as well as allowing dropping the SourceMapper trait from code. This had the unfortunate side-effect of moving `FatalError` to rustc_data_structures (it's needed in syntax_pos, due to SourceMap, but putting it there feels somehow worse).
bors added a commit that referenced this pull request Nov 16, 2019
Rollup of 5 pull requests

Successful merges:

 - #66350 (protect creation of destructors by a mutex)
 - #66407 (Add more tests for fixed ICEs)
 - #66415 (Add --force-run-in-process unstable option to libtest)
 - #66427 (Move the JSON error emitter to librustc_errors)
 - #66441 (libpanic_unwind for Miri: make sure we have the SEH lang items when needed)

Failed merges:

r? @ghost
@bors bors merged commit c31a875 into rust-lang:master Nov 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants