Implement x.py test src/tools/clippy --bless

[jyn514]

• Add clippy_dev to the rust workspace

  Before, it would give an error that it wasn't either included or
  excluded from the workspace:

  error: current package believes it's in a workspace when it's not:
  current: /home/joshua/rustc/src/tools/clippy/clippy_dev/Cargo.toml
  workspace: /home/joshua/rustc/Cargo.toml
  this may be fixable by adding `src/tools/clippy/clippy_dev` to the `workspace.members` array of the manifest located at: /home/joshua/rustc/Cargo.toml
  Alternatively, to keep it out of the workspace, add the package to the `workspace.exclude` array, or add an empty `[workspace]` table to the package's manifest.
  

• Change clippy's copy of compiletest not to special-case
  rust-lang/rust. Using OUT_DIR confused clippy_dev and it couldn't find
  the test outputs. This is one of the reasons why cargo dev bless used
  to silently do nothing (the others were that CARGO_TARGET_DIR and
  PROFILE weren't set appropriately).

• Run clippy_dev on test failure

I tested this by removing a couple lines from a stderr file, and they
were correctly replaced.

• Fix clippy_dev warnings

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

Does this actually work for blessing? Can we hook up x.py to run this?

[jyn514]

I don't know. I don't have time to look into it right now.

[jyn514]

Ok, I updated this to implement cargo test --bless src/tools/clippy.

[jyn514]

@Mark-Simulacrum do you know when you'll have a chance to look at this? In the meantime I've been cherry-picking the commit for #84039.

[Mark-Simulacrum]

Is there a reason for an explicit message here? compiletest blessing is silent -- is there a reason we can't pull that off here?

FWIW I find it a bit interesting that unlike compiletest this is a separate command etc -- maybe @Manishearth or someone familiar with clippy can say why? I'd expect this to be just a flag passed in to compile test when running clippy's tests.

[jyn514]

No particular reason, I'll remove it.

[camsteffen]

separate command

I think it's just because the compiletest bless functionality is newer and we just haven't migrated to it yet. But we also have an added feature where cargo dev bless only updates files that were updated after the last clippy build.

[Mark-Simulacrum]

@Manishearth can you either investigate or ping someone who could why we're seeing slightly different test outputs when blessing with this PR (Vec::new -> .., for example; see PR description).

[Manishearth]

cc @camsteffen@flip1995

unsure what's going on here, that looks like span_from_snippet failing but idk why

Also it could be divergences in compiletest. We tried to convince the compiler team to move compiletest out of tree but they weren't really in favor.

[flip1995]

AFAIK the compiletest Clippy uses doesn't have bless support, so we implemented in Clippy on our own. @xFrednet and @phansch worked on this command mainly, so they probably have insight into this. Also why cargo dev bless changes files that shouldn't be changed.

[jyn514]

can you either investigate or ping someone who could why we're seeing slightly different test outputs when blessing with this PR (Vec::new -> .., for example; see PR description).

It's also possible that I just need to rebase over master or something - I haven't spent too much time looking into this.

[Manishearth]

That sounds like it's not unlikely 😄

[xFrednet]

I'm sadly not totally up to date when it comes to the bless command due to some RL obligations. I believe @camsteffen has also worked on the bless command in Clippy, so he might know something about this 🙃

[flip1995]

@jyn514 I checked out your branch, rebased it on top of the current master and tried ./x.py test --bless src/tools/clippy after modifying some stderr files in the clippy dir.It didn't update any test files other than the modified.

[flip1995]

It would be great if you could also apply this patch:

diff --git a/src/tools/clippy/clippy_dev/src/new_lint.rs b/src/tools/clippy/clippy_dev/src/new_lint.rs
index d951ca0e630..4676c2affad 100644
--- a/src/tools/clippy/clippy_dev/src/new_lint.rs+++ b/src/tools/clippy/clippy_dev/src/new_lint.rs@@ -44,7 +44,7 @@ pub fn create(pass: Option<&str>, lint_name: Option<&str>, category: Option<&str
create_test(&lint).context("Unable to create a test for the new lint")
}
-fn create_lint(lint: &LintData) -> io::Result<()> {+fn create_lint(lint: &LintData<'_>) -> io::Result<()> {
let (pass_type, pass_lifetimes, pass_import, context_import) = match lint.pass {
"early" => ("EarlyLintPass", "", "use rustc_ast::ast::*;", "EarlyContext"),
"late" => ("LateLintPass", "<'_>", "use rustc_hir::*;", "LateContext"),
@@ -68,7 +68,7 @@ fn create_lint(lint: &LintData) -> io::Result<()> {
write_file(lint.project_root.join(&lint_path), lint_contents.as_bytes())
}
-fn create_test(lint: &LintData) -> io::Result<()> {+fn create_test(lint: &LintData<'_>) -> io::Result<()> {
fn create_project_layout<P: Into<PathBuf>>(lint_name: &str, location: P, case: &str, hint: &str) -> io::Result<()> {
let mut path = location.into().join(case);
fs::create_dir(&path)?;

[jyn514]

It would be great if you could also apply this patch:

Sure thing, done.

[camsteffen]

So no outstanding concerns here? Just checking since I was tagged.

[Mark-Simulacrum]

@jyn514 Can you update the PR description to reflect what I think is the current state, i.e., that this just works?

If so, r=me

[jyn514]

Oops, I updated the commit message but not the description. Now fixed.

@bors r=Mark-Simulacrum

[bors]

📌 Commit 8c25e27 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 8c25e27 with merge 267ddd8719e80bbcdf8613881b8f87788de74412...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web)(plain)

Click to see the possible cause of the failure (guessed by this bot)

 19 424M 19 84.6M 0 0 11.2M 0 0:00:37 0:00:07 0:00:30 11.1M
21 424M 21 90.5M 0 0 10.5M 0 0:00:40 0:00:08 0:00:32 10.0M
22 424M 22 96.5M 0 0 10.1M 0 0:00:41 0:00:09 0:00:32 9194k
24 424M 24 101M 0 0 10.2M 0 0:00:41 0:00:09 0:00:32 8766k
curl: (56) OpenSSL SSL_read: Connection reset by peer, errno 54
clang+llvm-10.0.0-x86_64-apple-darwin/bin/modularize: Lzma library error: No progress is possible
tar: Error exit delayed from previous errors.
##[error]Process completed with exit code 1.
[command]/usr/local/bin/git version
git version 2.31.1
[command]/usr/local/bin/git config --local --name-only --get-regexp core\.sshCommand
[command]/usr/local/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :

[jyn514]

curl: (56) OpenSSL SSL_read: Connection reset by peer, errno 54

@bors retry

[bors]

⌛ Testing commit 8c25e27 with merge 10a51c0...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 10a51c0 to master...

[estebank]

I'm making some changes to the structured suggestion output, and it seems like I'm unable to get my local environment to fail the tests that are failing in CI. I'm not sure if it is because cargo uitest is not actually using my locally built rustc/cargo (maybe due to the toolchain file?). I've tried invoking with ./x.py test src/tools/clippy --stage 2 --bless, which seems like it should do the right thing, but I am currently stuck. Could someone point me in the direction on how to keep clippy's tests in sync with rustc?

[jyn514]

That should work. If it's not, I don't know what's going wrong, sorry.

[estebank]

I just lost two hours to being in the wrong branch 🤦‍♂️

Edit: BTW, thank you @jyn514 for improving the workflow here! Had it not been for my PEBKAC problem, the current behavior is almost perfect.