x.py: don't enable -Zconfig-profile or -Zexternal-macro-backtrace with x.py clippy

[matthiaskrgr]

It seems we need this to make x.py clippy run again.

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

I would also like comments on the added checks indicating that they're (presuming this is true) limitations of clippy today.

I will note that I am not super happy with needing to do this -- it seems like a bug somewhere upstream that we have to? Is there a fix in the works? Ideally we would not have to diverge in cargo flags for clippy / rustc.

[Mark-Simulacrum]

Seems strange to gate this flag only on stage == 0, presumably it should be around the whole thing?

[matthiaskrgr]

Ideally we would not have to diverge in cargo flags for clippy / rustc.

I'm not sure how to avoid this as long as cargo comes from beta-channel but clippy comes from nightly channel, unless we explicitly force beta clippy to run.

[matthiaskrgr]

@rustbot modify labels to +S-waiting-on-review, -S-waiting-on-author

[Mark-Simulacrum]

I am actually rather surprised by us needing to do this, as AFAICT if the cmd is clippy we should always be in stage 0? When is that not the case today?

(and if Clippy is already in stage 0, then the right flags would be in RUSTFLAGS_BOOTSTRAP, right?)

[matthiaskrgr]

You are right, my code comment is simply wrong.
Looking at --verbose output, we are indeed in stage 0 all the time.
Looking at the error I was getting previously

error[E0061]: this function takes 1 argument but 0 arguments were supplied
  --> src/librustc_data_structures/box_region.rs:87:52
   |
87 |         let result = Pin::new(&mut self.generator).resume();
   |                                                    ^^^^^^- supplied 0 arguments
   |
help: expected the unit value `()`; create it with empty parentheses
   |
87 |         let result = Pin::new(&mut self.generator).resume(());
   |                                                           ^^

and the corresponding code

    #[cfg(bootstrap)]
    pub fn complete(&mut self) -> R {
        // Tell the generator we want it to complete, consuming it and yielding a result
        BOX_REGION_ARG.with(|i| i.set(Action::Complete));

        let result = Pin::new(&mut self.generator).resume();
        if let GeneratorState::Complete(r) = result { r } else { panic!() }
    }

    #[cfg(not(bootstrap))]
    pub fn complete(&mut self) -> R {
        // Tell the generator we want it to complete, consuming it and yielding a result
        BOX_REGION_ARG.with(|i| i.set(Action::Complete));

        let result = Pin::new(&mut self.generator).resume(());
        if let GeneratorState::Complete(r) = result { r } else { panic!() }
    }

we actually need to disable cfg=booststrap for x.py clippy in order to have it work from what it seems.

[nikomatsakis]

r? @Mark-Simulacrum

[Mark-Simulacrum]

Hm, so AFAIK x.py clippy today is just using the clippy it finds in the environment, right? Can we perhaps make sure that's a nightly clippy (e.g,. running cargo +nightly clippy) or so?

Basically I feel like this is going to some non-trivial effort to patch over the fact that we're using different versions of clippy and we don't know what we're using which feels like it's never going to work out quite as well as trying to detect what we're using. We should likely also be using the same cargo as the clippy we're using, right? vs. using the beta cargo with (for example) a nightly clippy?

[matthiaskrgr]

Hm, so AFAIK x.py clippy today is just using the clippy it finds in the environment, right?

Yeah.

Can we perhaps make sure that's a nightly clippy

I've been thinking about whether we could simply install the "clippy" component for x.py clippy just as x.py fmt installs its own rustfmt. I'll have to take a look how this is done for rustfmt...

I would prefer to run a nightly clippy on x.py clippy since that's more up to date and we can use it to spot false positives on new lints or other clippy bugs for example.

As far as I can remember, x.py clippy has been broken most of the time, except for maybe one week after nightly gets promoted to beta, then nightly and beta get out of "sync" and x.py clippy runs into errors again...
I've been trying to make it work for myself and wanted to upstream the changes but I'm not sure how much effort trying to implement a more generic solution is worth it.

[Mark-Simulacrum]

Here's a proposal:

We edit clippy-driver (or whatever the clippy we're using is) to provide the "channel" in the --version (e.g., nightly, beta, etc.). I think this is the easiest way for us to determine what "stage" we should be running in.

Then we modify the cargo function to, if we're running cargo clippy, get the version information and if it's a nightly clippy, force stage 1, if it's a beta clippy, force stage 0, and if it's a stable clippy use stage 0 but print a warning that it's not really supported to do that.

This would, I think, work well. However, it's quite a bit of work (and we'd need to wait for the channel printing to roll out) -- I would propose that in this PR we just set stage = 1 if we're running cargo clippy and document that you should be using nightly.

I think that should work, right? And we'd presumably not need any of the other changes then?

[matthiaskrgr]

We edit clippy-driver (or whatever the clippy we're using is) to provide the "channel" in the --version (e.g., nightly, beta, etc.). I think this is the easiest way for us to determine what "stage" we should be running in.

I think this already works today: clippy-driver -vV

rustc 1.43.0-nightly (6fd8798f4 2020-02-25)
binary: rustc
commit-hash: 6fd8798f4de63328d743eb2a9a9c12e202a4a182
commit-date: 2020-02-25
host: x86_64-unknown-linux-gnu
release: 1.43.0-nightly
LLVM version: 9.0

Then we modify the cargo function to,

Are you referring to cargo code or x.py/bootstrap.rs here?

[Mark-Simulacrum]

Huh, I'm pretty sure I ran that with the clippy I had (a stable clippy) and it didn't work.

x.py for the cargo function.

[joelpalmer]

Triaged

[bors]

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

[matthiaskrgr]

rebase for @Centril

[Dylan-DPC-zz]

@Mark-Simulacrum this is ready for review

[matthiaskrgr]

Sorry, this is not ready for review. I'll just close it to avoid confusion.