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

clippy-driver: pass all args to rustc if --rustc is present #5178

Merged
merged 4 commits into from
Jun 23, 2020

Conversation

matthiaskrgr
Copy link
Member

@matthiaskrgr matthiaskrgr commented Feb 15, 2020

changelog: clippy-driver: pass all args to rustc if --rustc is present

src/driver.rs Outdated
// pass all succeeding args to rustc
let args_for_rustc = &orig_args[2..].to_vec();

let exitstatus = Command::new("rustc")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this doesn't help. rustc isn't necessarily in the path, and even if it were, doesn't necessarily have anything to do with the version of rustc clippy-driver was built against. If I wanted to know the version of the rustc executable, I could just run rustc --version.

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 15, 2020
@flip1995
Copy link
Member

What we could test for starters, is if the first argument is rustc, that we immediately return rustc_driver::run_compiler(&args, DefaultCallbacks, None, None), where args is orig_args without clippy_driver. This should implement clippy_driver rustc ... = rustc ...-linked-with-clippy. If that works, we can carefully go through the args modifications in this closure and determine what should be kept when rustc is passed.

@flip1995
Copy link
Member

I just realized, that the clippy-driver rustc idea doesn't work:

rust-clippy/src/driver.rs

Lines 353 to 355 in 06f0ab0

// Setting RUSTC_WRAPPER causes Cargo to pass 'rustc' as the first argument.
// We're invoking the compiler programmatically, so we ignore this/
let wrapper_mode = orig_args.get(1).map(Path::new).and_then(Path::file_stem) == Some("rustc".as_ref());

clippy-driver rustc is run, when running cargo clippy...

@flip1995
Copy link
Member

What do you think about clippy-driver --rustc?

@flip1995
Copy link
Member

This would be the patch for adding this #5178 (comment):

diff --git a/src/driver.rs b/src/driver.rs
index 097b796e..b280169d 100644
--- a/src/driver.rs
+++ b/src/driver.rs
@@ -299,6 +299,12 @@ pub fn main() {
         rustc_driver::catch_fatal_errors(move || {
             let mut orig_args: Vec<String> = env::args().collect();
 
+            if let Some(pos) = orig_args.iter().position(|arg| arg == "--rustc") {
+                orig_args.remove(pos);
+                orig_args[0] = "rustc".to_string();
+                return rustc_driver::run_compiler(&orig_args, &mut DefaultCallbacks, None, None);
+            }
+
             if orig_args.iter().any(|a| a == "--version" || a == "-V") {
                 let version_info = rustc_tools_util::get_version_info!();
                 println!("{}", version_info);

@matthiaskrgr
Copy link
Member Author

I just realized, that the clippy-driver rustc idea doesn't work

Urgh that sucks...
Imo the intuition with --arguments is that the order does not really matter, so clippy-driver --rustc --version should behave like clippy-driver --version --rustc which is not the case here :(
But I don't have any better ideas right now..

@flip1995
Copy link
Member

flip1995 commented Mar 9, 2020

The patch I posted in #5178 (comment) takes care of this and everything will be passed to rustc, when --rustc is passed, no matter in which position. The only thing, that didn't work as expected is clippy-driver --rustc some_file.rs, which should be rustc some_file.rs, but threw error: counldn't find crate for std.

@flip1995 flip1995 added S-needs-discussion Status: Needs further discussion before merging or work can be started S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 31, 2020
@bors
Copy link
Collaborator

bors commented May 17, 2020

☔ The latest upstream changes (presumably #5608) made this pull request unmergeable. Please resolve the merge conflicts.

@flip1995 flip1995 self-assigned this May 25, 2020
@flip1995
Copy link
Member

Friendly ping @matthiaskrgr. Do you think we can/should move forward with this?

@matthiaskrgr
Copy link
Member Author

looks like the error: counldn't find crate for std. problem is caused by missing --sysroot arg when calling rustc, this is actually injected a little bit later in the code: https://github.com/rust-lang/rust-clippy/blob/master/src/driver.rs#L386

@matthiaskrgr matthiaskrgr changed the title clippy-driver: pass all args after "rustc" to rustc clippy-driver: pass all args after "--rustc" to rustc May 31, 2020
@flip1995
Copy link
Member

flip1995 commented Jun 2, 2020

Can you add a test script, that compares output of some clippy-driver --rustc [some_args] commands with rustc [some_args]? Similar to the .github/driver.sh script. (Or you can just add tests for this there)

@matthiaskrgr
Copy link
Member Author

Added test for clippy-driver --rustc --version --verbose vs rustc --version --verbose to driver.sh

@flip1995
Copy link
Member

flip1995 commented Jun 2, 2020

This test would also be nice to have:

echo "fn main() {}" > tmp.rs
clippy-driver --rustc tmp.rs

and maybe a test where --rustc is not the first argument.

@matthiaskrgr
Copy link
Member Author

Hmm, clippy-driver --version --rustc prints rustc --version, this is a bug, right?

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, clippy-driver --version --rustc prints rustc --version, this is a bug, right?

Nope that is the expected behavior. When the --rustc arg is passed to the clippy-driver everything should be forwarded to the rustc used by Clippy. This is consistent behavior.

Can you add the --rustc arg to the Clippy help:

rust-clippy/src/driver.rs

Lines 200 to 210 in 67ec96c

fn display_help() {
println!(
"\
Checks a package to catch common mistakes and improve your Rust code.
Usage:
cargo clippy [options] [--] [<opts>...]
Common options:
-h, --help Print this message
-V, --version Print version info and exit

.github/driver.sh Outdated Show resolved Hide resolved
@matthiaskrgr matthiaskrgr changed the title clippy-driver: pass all args after "--rustc" to rustc clippy-driver: args to rustc if --rustc is present Jun 9, 2020
@matthiaskrgr matthiaskrgr changed the title clippy-driver: args to rustc if --rustc is present clippy-driver: pass all args to rustc if --rustc is present Jun 9, 2020
@matthiaskrgr matthiaskrgr force-pushed the rustc_arg_pass branch 2 times, most recently from d484f44 to 1c32cf3 Compare June 9, 2020 11:46
@matthiaskrgr
Copy link
Member Author

CI was failing because I was testing against a file that I forgot to git-commit... 😆

@matthiaskrgr matthiaskrgr added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 9, 2020
@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-needs-discussion Status: Needs further discussion before merging or work can be started S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 12, 2020
@flip1995
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 23, 2020

📌 Commit 7a62380 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Jun 23, 2020

🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened

bors added a commit that referenced this pull request Jun 23, 2020
Rollup of 9 pull requests

Successful merges:

 - #5178 (clippy-driver: pass all args to rustc if --rustc is present)
 - #5705 (Downgrade unnested_or_patterns to pedantic)
 - #5709 (Fix ICE in consts::binop)
 - #5710 (typo)
 - #5712 (Remove `bar` from blacklisted names)
 - #5713 (Use lints in Clippy that are enabled in rustc bootstrap)
 - #5716 (Fix typo in wildcard_imports)
 - #5724 (redundant_pattern_matching: avoid non-`const fn` calls in const contexts)
 - #5726 (Fix typo)

Failed merges:

r? @ghost

changelog: rollup
@bors bors merged commit 7c61be6 into rust-lang:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: The marked PR was approved and is only waiting bors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants