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

Add rustfix support from rustc (rough rustc copy) #151

Merged
merged 2 commits into from
Jan 2, 2019

Conversation

phansch
Copy link
Contributor

@phansch phansch commented Dec 9, 2018

This adds rustfix integration that was already present in the compiletest of
rustc. We would like to use this in Clippy to be able to test that all
suggestions can be applied by rustfix and that the changes are compiling as
well. This would probably also benefit other tools that make use of lints.

Associated Clippy PR that uses this branch: rust-lang/rust-clippy#3519

Some open questions:

  1. Should rustfix support be hidden behind a cargo feature? I can imagine that only a small portion of compiletest-rs users would need rustfix as a dependency.
  2. I'm not really sure how this could be tested properly?

@phansch
Copy link
Contributor Author

phansch commented Dec 10, 2018

I noticed that this changes the stderr output to JSON in general:

https://travis-ci.org/rust-lang/rust-clippy/jobs/465644097#L991

If it's desired, I can try to change this back to the pretty output for terminals and just using JSON for rustfix. This would need to be done in Rust's compiletest, too and maybe it makes sense to do it in a second step.

@phansch phansch changed the title Add rustfix support from rustc Add rustfix support from rustc (rough rustc copy) Dec 17, 2018
@phansch
Copy link
Contributor Author

phansch commented Dec 17, 2018

Additionally noting that I left out some switches about NLL compare modes that were tied together with rustfix.

@laumann laumann self-requested a review December 19, 2018 05:01
@laumann
Copy link
Collaborator

laumann commented Dec 19, 2018

Hi @phansch Sorry for my tardiness on this!

Should rustfix support be hidden behind a cargo feature? I can imagine that only a small portion of compiletest-rs users would need rustfix as a dependency.

If it doesn't interfere with existing users' ability to just update, I think we're OK. When creating the stable feature, we just added it in a new patch version (although you're not supposed to do that I think).

I'm not really sure how this could be tested properly?

Well, I think rust-lang/rust-clippy#3519 speaks for itself :-)

I noticed that this changes the stderr output to JSON in general

My only question is: would this break existing users if they update? If so, I think we should take steps to prevent the output from changing. Otherwise, I don't see a problem and agree that we could handle it in a second step.

@laumann
Copy link
Collaborator

laumann commented Dec 19, 2018

Re testing: You could add new tests to the test-project here, just to verify that the new additions work. But I don't require it.

@laumann laumann removed their request for review December 19, 2018 05:13
@phansch
Copy link
Contributor Author

phansch commented Dec 19, 2018

Thanks for your feedback!

If it (rustfix) doesn't interfere with existing users' ability to just update, I think we're OK.

I will actually need to check if it compiles on stable, otherwise we would have to put it behind the stable feature 👍

would this (json output) break existing users if they update?

Yes. This is currently a bug (?) in rustc as well I think. I'm going to add some rustfix tests to the test project. That should also make it easier to try and preserve the human readable output and getting fixes into rust.

@phansch
Copy link
Contributor Author

phansch commented Dec 19, 2018

Regarding the JSON output: This actually only happens when the full stderr is dumped at the end:
selection_047

As far as I can tell, this is because this change will run the tests with error-format=json by default which turns the stderr output into JSON. It's then parsed back to human readable output for the stderr diff that is rendered first and the stderr at the end is printed as it is: JSON.

I don't think we would want to fix this, because it would require compiling twice (once with and once without JSON output) instead of once..

Regarding stable compilation: rustfix works just fine on stable

@laumann laumann merged commit 5c67257 into Manishearth:master Jan 2, 2019
@laumann
Copy link
Collaborator

laumann commented Jan 2, 2019

Thanks!

@phansch
Copy link
Contributor Author

phansch commented Jan 2, 2019

Thank you and happy new year 🎇

@phansch phansch deleted the add_rustfix_support branch January 2, 2019 06:45
@phansch
Copy link
Contributor Author

phansch commented Jan 2, 2019

@laumann if you could publish a new release, that would be great =)

@laumann
Copy link
Collaborator

laumann commented Jan 3, 2019

@phansch Yes, of course! And a happy New Year to you as well 🎉

@laumann
Copy link
Collaborator

laumann commented Jan 3, 2019

@phansch 0.3.18 is out!

bors added a commit to rust-lang/rust-clippy that referenced this pull request Jan 3, 2019
Integrate rustfix into Clippy test suite

Once the [PR to compiletest-rs](Manishearth/compiletest-rs#151) is reviewed and merged this fixes #2376.

I will create a separate tracking issue for adding `run-rustfix` to all tests.
@phansch
Copy link
Contributor Author

phansch commented Jan 3, 2019

@laumann Thanks for the release! That should make Clippy suggestions much more reliable in the future and paves the way for proper Clippy<->Rustfix integration 🎇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants