This repository has been archived by the owner on Nov 24, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 62
Add --prepare-for
flag
#98
Merged
Merged
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
cea8fd1
WIP: Add `--prepare-for` flag
killercup bb18250
Fix typo
killercup fe2526e
WIP: Trigger 2018 breakage lints
killercup 43d5f18
No need for the allow(unused) anymore
killercup cd641cd
Use rust_2018_preview feature
killercup a956e9e
Let's not disable lints
killercup File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
//! Test that we can use cargo-fix to upgrade our code to work with the 2018 | ||
//! edition. | ||
//! | ||
//! We'll trigger the `absolute_path_starting_with_module` lint which should | ||
//! transform a `use ::foo;` where `foo` is local to `use crate::foo;`. | ||
|
||
use super::project; | ||
|
||
#[test] | ||
fn prepare_for_2018() { | ||
let p = project() | ||
.file( | ||
"src/lib.rs", | ||
r#" | ||
#![feature(crate_in_paths)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh gee this is gonna make for a not-so-great user experience :( If users don't have this enabled then rustfix will accidentally deduce that its fixes broke your crate, so they'll all be backed out. That's a bummer... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah… Without the feature flag, we don't 'fix' anything, though, so I don't see how we could ever revert fixes? |
||
|
||
mod foo { | ||
pub const FOO: &str = "fooo"; | ||
} | ||
|
||
mod bar { | ||
use ::foo::FOO; | ||
} | ||
|
||
fn main() { | ||
let x = ::foo::FOO; | ||
} | ||
"#, | ||
) | ||
.build(); | ||
|
||
let stderr = "\ | ||
[CHECKING] foo v0.1.0 (CWD) | ||
[FIXING] src/lib.rs (2 fixes) | ||
[FINISHED] dev [unoptimized + debuginfo] | ||
"; | ||
p.expect_cmd("cargo-fix fix --prepare-for 2018") | ||
.stdout("") | ||
.stderr(stderr) | ||
.run(); | ||
|
||
println!("{}", p.read("src/lib.rs")); | ||
assert!(p.read("src/lib.rs").contains("use crate::foo::FOO;")); | ||
assert!(p.read("src/lib.rs").contains("let x = crate::foo::FOO;")); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come these specific
-A
flags were required? I'd actually sort of expect that we don't pass any extra arguments other than the-W
for the lintThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want just the breakage links here -- but other lints might trigger. And I don't know how to disabled everything but one lint group in a clever way :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm is it bad though if other warnings are emitted? Or is it bad if we fix some otherwise fixable warnings along the way?
I'm worried about this being an ever-growing list :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for fixing as many things as possible, but it might send the wrong message in this specific context. E.g., if I run
cargo fix --prepare-for 2018
and it removes a bunch of (unused)mut
s I might end up thinking that this is required for the new edition.I'll try and play around with filtering by the lint group that triggered a lint. If that's possible we can just add the
-W rust-2018-breakage
and skip fixing other lints.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can run without edition lints, and if any diagnostics appear, ask the user to fix them first. If none appear, do what you do now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True yeah we could filter, but I'd imagine that for most maintained codebases they're already warning-free, so I wouldn't expect something like this to come up much in practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't even mean filtering, just abort if there are any (and tell the user the reason for the abort)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have seen many new rustaceans who have crates that are full of warnings, because they're used to that from other languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sure yeah that's a possibility, although I'm still not sure why we would go out of our way to worry about other lints. Everything should work A-OK if we purely pass
-W rust-2018-breakage
. The user will be informed, as they always are, of lints that couldn't be fixed. Anything that comes up which is fixable as part of the breakage lint is already fixed.