Filter by machine applicability #97
Conversation
killercup
force-pushed
the
feature/filter-by-machine-applicability
branch
3 times, most recently
from
3ee72ab
to
0c11173
Compare
|
With luck rust-lang/rust#50486 can land and we don't have to pass down a -Z flag |
|
Looks reasonable to me! If possible though I'd prefer to wait for the Rust PR to land so we can avoid using an extra env var in the tests, but is that ok? |
|
@alexcrichton that means we can only use "upgraded" lints in tests. (Which is okay, and probably a good idea in the long run) |
|
Ah yeah I think that's ok, all the tests currently are "just the first thing that provided a suggestion" |
killercup
force-pushed
the
feature/filter-by-machine-applicability
branch
from
0c11173
to
9a29f57
Compare
killercup
force-pushed
the
feature/filter-by-machine-applicability
branch
from
9a29f57
to
7d5bb22
Compare
|
Alright, the rustc 1.28.0-nightly (71e87be38 2018-05-22) contains the stable applicability field. I haven't touched the test file content and used the yolo env var to keep the tests working. I expect we can quickly change the tests to use "stable" lints without much code change once rust-lang/rust#50723 lands. |
src/lib.rs
Outdated
| match (filter, &span.suggestion_applicability) { | ||
| (MachineApplicableOnly, Some(MachineApplicable)) => true, | ||
| (MachineApplicableOnly, _) => false, | ||
| (_, _) => true, |
There was a problem hiding this comment.
Could the left hand side of this match be Everything to get exhaustive matches to kick in?
cargo-fix/tests/all/broken_build.rs
Outdated
| @@ -111,6 +114,7 @@ fn broken_fixes_backed_out() { | |||
|
|
|||
| // Attempt to fix code, but our shim will always fail the second compile | |||
| p.expect_cmd("cargo-fix fix") | |||
| .env("__CARGO_FIX_YOLO", "true") | |||
There was a problem hiding this comment.
Hm does this mean that the lints we're testing against aren't currently classifed as "machine applicable"?
There was a problem hiding this comment.
Hm ok, could we perhaps start testing lints that are classified as machine applicable?
There was a problem hiding this comment.
Well, like I said, we don't have many -- just unreachable_pub, bare_trait_object, and abs_path_with_module. Do you want to rewrite the tests to always use those? I'd wait a bit for more applicability markers to land and then refactor the tests.
There was a problem hiding this comment.
Hm ok, the 2018 compatibility lints are at least machine applicable, right? If so can you make sure that they're already tested and/or add a test for that?
There was a problem hiding this comment.
Right -- everything but the upgrade_extern_crate case work without the env var. Updated.
There was a problem hiding this comment.
Nice!
As one final comment, could this be an extension method for just the test suite to do something like .fix_everything() instead of duplicating the env var? Other than that r=me!
killercup
force-pushed
the
feature/filter-by-machine-applicability
branch
from
52c9c36
to
2eaaa75
Compare
|
Alright, let's land this! (I'll be afk for a bit but will try to push a new release to crates.io in a bit) |
Finally -- we have a way to indicate that we trust a lint! This is currently an unstable feature in rustc and thus requires nightly.
The first lints that are currently as machine applicable land with rust-lang/rust#50454, so adding tests will have to wait for a bit -- see #95.
(this is currently based on #96)