Expose compiletest as a library for use in Clippy #103266

Alexendoo · 2022-10-19T20:29:57Z

I tried out the binary approach from #99968, with some tweaks and a few extra flags it worked well for most of our test suite. The ui-cargo and ui-toml tests need some very clippy specific bits of setup though

Rather than those clippy specific bits living in compiletest this PR adds two hooks to compiletest's Config that clippy will supply. This lets us keep the clippy specifics in the clippy tree to avoid any sync woes

Some other changes:

--root to break the .parent().unwrap().parent().unwrap().parent().unwrap() == rust repo root assumption
--no-expected-comments as clippy doesn't use //~ ERROR
A port of Use the revision filename when compiling the rustfix output Manishearth/compiletest-rs#258

This doesn't publish the library as a component, as I wasn't sure how to go about that

cc @Mark-Simulacrum
r? @jyn514

Mark-Simulacrum · 2022-10-20T02:05:03Z

This doesn't publish the library as a component, as I wasn't sure how to go about that

Let's leave this to a separate PR, since it'll have a fairly distinct set of changes. That said, the broad strokes are going to be to do something similar to the RustcDev component which takes the artifacts built for compile test and copies them into the target directory: https://github.com/rust-lang/rust/blob/master/src/bootstrap/dist.rs#L651-L652

I believe we don't currently produce a stamp file for tools, but it should be possible to make that happen. It'll be a bit tricky to get compiletest shipped as a library in terms of the dependency tree -- we don't currently build it in the compiler directory which means that copying it into the sysroot likely isn't a good idea. But I think the first step is to just try and do that, and we can go from there if it fails; that will mean replicating the stamp file for tools like we do for rustc and replicating those steps.

Alexendoo · 2022-10-24T12:17:08Z

I split the commits up differently so the main.rs -> lib.rs actually gets picked up as a rename, the changes view still shows ±1000 but if you view the two commits individually it does not

bors · 2022-10-24T14:24:49Z

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

jyn514 · 2022-10-27T15:34:09Z

--ignore-exit-status as clippy tests may or may not fail to compile
--no-expected-comments as clippy doesn't use //~ ERROR

Can you talk a bit more about why these changes were necessary? We already support failing to compile by default, or successfully compiling with // check-pass. What does clippy using instead of //~ ERROR ?

Can you confirm for me that the clippy team is planning to use this changes if they're merged? cc @oli-obk @flip1995

Alexendoo · 2022-10-27T21:02:10Z

I removed --ignore-exit-status, I think I initially added that to keep the test suite relatively unchanged, but yeah adding a bunch of // check-pass isn't a big deal, it's mostly only ICE regression tests that need them

We don't have anything inline in place of //~ ERROR, just the usual .stderr files. e.g. copy_iterator.rs + copy_iterator.stderr

I plan to open a PR on the clippy side at least. My impression is that it's likely to be accepted but I'll wait for the others to weigh in there

flip1995 · 2022-10-28T10:01:50Z

I'm in favor of using compiletest from the rust-lang/rust repo, rather than the fork. So I would give my +1 to move forward with this.

bors · 2022-10-29T17:43:59Z

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

bors · 2022-11-05T02:05:37Z

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

jyn514

I have a few comments on the implementation but overall the changes seem reasonable.

I have a larger question for @Alexendoo: are you planning to use compiletest as a library or as a binary? There's a weird mix of both currently: renaming main to lib, making some things public, but also making the main function public, adding more arguments, and you mentioned packaging as a binary on nightly. We should pick one or the other.

jyn514 · 2022-11-05T13:48:58Z

src/tools/compiletest/src/common.rs

+    pub exclude_file: Option<Arc<dyn Fn(&Path) -> bool + Send + Sync>>,
+
+    /// Modify a [Command] just before it's executed
+    pub modify_command: Option<Arc<dyn Fn(&Path, &mut Command) + Send + Sync>>,


This seems weird. Why would you need to run this before any command? I would expect you to only run it for things like the compiler if you need to change the sysroot.

Ah, looking later I see this is only called for the rustc process. Can you mention that in the doc-comment?

Yeah we don't need it for any command just clippy-driver, I had the hook in a place I think gets called by other things as well like debuggers/run tests (that clippy doesn't happen to use), but I've found a place to move it to that I think is only called for compilation

src/tools/compiletest/src/common.rs

src/tools/compiletest/src/header.rs

jyn514 · 2022-11-05T13:51:56Z

src/tools/compiletest/src/lib.rs

@@ -67,6 +67,7 @@ pub fn parse_config(args: Vec<String>) -> Config {
        .optflag("", "force-valgrind", "fail if Valgrind tests cannot be run under Valgrind")
        .optopt("", "run-clang-based-tests-with", "path to Clang executable", "PATH")
        .optopt("", "llvm-filecheck", "path to LLVM's FileCheck binary", "DIR")
+        .reqopt("", "root", "root directory of the project", "PATH")


maybe this should be named "source root" or "source directory"? I guess "src-base" below makes that confusing ... that should be named "suite-path" IMO, but it doesn't have to block this PR. would be nice to fix in a follow up though.

👍

Changed it to source root, I'm happy to follow up with another PR that changes src-base also

src/tools/compiletest/src/lib.rs

jyn514 · 2022-11-05T13:53:33Z

src/tools/compiletest/src/lib.rs

@@ -36,7 +36,7 @@ mod read2;
 pub mod runtest;
 pub mod util;

-fn main() {
+pub fn main() {


this seems weird to make public? is clippy planning to use exactly this entrypoint? that means it can't use the new hooks.

We are not, it's only public as main.rs consists of

fn main() { compiletest::main(); }

This was to keep the diff smaller, I can move fn main & others only used by the binary back into main.rs if that's preferable

Here's the current WIP I have for using it in clippy: https://github.com/Alexendoo/rust-clippy/blob/compiletest/tests/compile-test.rs

(using compiletest = { path = "../rust/src/tools/compiletest" })

Ok. Can we keep the parts clippy isn't using in main.rs so it's easier to understand what parts have to work both in-tree and out of tree?

Moved main and log_config there, parse_config is used by some tests though so remains in lib.rs

jyn514 · 2022-11-05T13:57:19Z

src/tools/compiletest/src/runtest.rs

+            for expected in &expected_errors {
+                self.error(&format!(
+                    "{file_name}:{}: found expectation comment `{}`",
+                    expected.line_num, expected.msg,
+                ))
+            }


This seems weird; @flip1995 do you want to actively prevent people from adding //~ ERROR annotations? I mean I guess that's fine, but it would be nice if clippy and rust converged on this eventually, forcefully preventing it seems unfortunate.

Here I was copying how known-bug works, but I could change it to check them if they happen to be present + ignore missing ones

src/tools/compiletest/src/runtest.rs

jyn514 · 2022-11-05T13:59:10Z

src/tools/compiletest/src/runtest.rs

                TargetLocation::ThisFile(self.make_exe_name()),
                emit_metadata,
                AllowUnused::No,
                LinkToAux::Yes,
            );
+            // Set the crate name to avoid `file.revision.fixed` inferring the
+            // invalid name `file.revision`
+            rustc.arg("--crate-name=fixed");


what does "invalid name" mean? Why did this only break now when using clippy?

Currently files with both revisions and // run-rustfix check test.fixed instead of test.revision.fixed, that gets fixed just above here

However rustc seems to use file_stem of the path to determine a default crate name if you don't provide your own, so it infers test.revision here, but considers that invalid:

$ rustc test.revision.fixed error: invalid character `'.'` in crate name: `test.revision` error: aborting due to previous error

There don't seem to be any revision + // run-rustfix tests in the rust suite, but there are some leftovers from a previous one that's no longer used so I'll rm those

Alexendoo · 2022-11-05T18:35:28Z

are you planning to use compiletest as a library or as a binary? There's a weird mix of both currently: renaming main to lib, making some things public, but also making the main function public, adding more arguments, and you mentioned packaging as a binary on nightly. We should pick one or the other.

As a library

The mention of testing the binary is that I tried the approach from #99968 and believe that a library will be better suited

I've removed the flag --no-expected-comments leaving it just as a Config option

jyn514 · 2022-11-17T02:47:44Z

Let's avoid merging this until we work out with @oli-obk whether clippy is going to use this or Oli's new codebase.

Return multiple resolutions from `def_path_res` Changes `def_path_res` to return all the resolutions matching the path rather than the first one (with a namespace hint that covered some cases). This would fix any issues that come up with multiple versions of the same crate being present as they all have the same crate name It also adds resolution of `impl _ {}` items for local items, and removes struct field resolution as it didn't seem to be used anywhere I tested it on a local crate and it worked for the multiple crate issue, but I couldn't come up with a test that worked well with `// aux-build`, maybe `// aux-crate` after rust-lang/rust#103266 could work but I'm not sure on that either changelog: [`disallowed_methods`], [`disallowed_types`], [`disallowed_macros`]: fix path resolution with multiple versions of the same crate changelog: [`disallowed_methods`]: Resolve methods in `impl`s in the current crate

bors · 2022-11-19T19:12:09Z

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

jyn514 · 2023-04-26T06:38:51Z

@Alexendoo it looks like clippy is planning to go with ui_test: rust-lang/rust-clippy#10426
do you mind if I close this PR?

Alexendoo · 2023-04-26T10:46:17Z

Yeah that's the plan, didn't realise this was still open. Thank you for your time on it!

Alexendoo added the A-clippy Area: Clippy label Oct 19, 2022

rust-highfive assigned jyn514 Oct 19, 2022

rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-testsuite Area: The testsuite used to check the correctness of rustc labels Oct 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2022

Alexendoo force-pushed the clippy-compiletest branch from 30249c2 to 75be411 Compare October 24, 2022 12:14

Alexendoo force-pushed the clippy-compiletest branch from 75be411 to 3f11dbe Compare October 24, 2022 17:20

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2022

Alexendoo force-pushed the clippy-compiletest branch from 3f11dbe to b3c473e Compare October 27, 2022 20:15

Alexendoo 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 (such as code changes or more information) from the author. labels Oct 29, 2022

Alexendoo force-pushed the clippy-compiletest branch from b3c473e to 0219f07 Compare October 29, 2022 17:54

Alexendoo mentioned this pull request Nov 4, 2022

Return multiple resolutions from def_path_res rust-lang/rust-clippy#9800

Merged

Alexendoo force-pushed the clippy-compiletest branch from 0219f07 to 3333422 Compare November 5, 2022 13:26

jyn514 suggested changes Nov 5, 2022

View reviewed changes

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2022

Alexendoo added 2 commits November 5, 2022 17:55

Remove some unused .fixed files

2ed8618

Rename compiletest's main.rs to lib.rs

c4c7b30

Alexendoo force-pushed the clippy-compiletest branch from 3333422 to b40547e Compare November 5, 2022 17:56

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 5, 2022

Alexendoo force-pushed the clippy-compiletest branch 2 times, most recently from 8625f36 to 3aae7cc Compare November 5, 2022 18:26

Expose compiletest as a library for use in Clippy

ff54663

Alexendoo force-pushed the clippy-compiletest branch from 3aae7cc to ff54663 Compare November 5, 2022 19:51

Alexendoo 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 (such as code changes or more information) from the author. labels Nov 5, 2022

This was referenced Nov 9, 2022

Recover from common if let syntax mistakes/typos #103636

Merged

Support individual files for clippy tests #104200

Open

who tests the tester? #47606

Open

jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2022

Alexendoo mentioned this pull request Nov 19, 2022

cargo uitest fails when project path has whitespaces rust-lang/rust-clippy#9874

Open

flip1995 mentioned this pull request Feb 28, 2023

Port clippy away from compiletest to ui_test rust-lang/rust-clippy#10426

Merged

1 task

Alexendoo closed this Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose compiletest as a library for use in Clippy #103266

Expose compiletest as a library for use in Clippy #103266

Alexendoo commented Oct 19, 2022 •

edited

Loading

Mark-Simulacrum commented Oct 20, 2022

Alexendoo commented Oct 24, 2022

bors commented Oct 24, 2022

jyn514 commented Oct 27, 2022

Alexendoo commented Oct 27, 2022 •

edited

Loading

flip1995 commented Oct 28, 2022 •

edited

Loading

bors commented Oct 29, 2022

bors commented Nov 5, 2022

jyn514 left a comment

jyn514 Nov 5, 2022

Alexendoo Nov 5, 2022

jyn514 Nov 5, 2022

Alexendoo Nov 5, 2022

jyn514 Nov 5, 2022

Alexendoo Nov 5, 2022 •

edited

Loading

jyn514 Nov 5, 2022

Alexendoo Nov 5, 2022

jyn514 Nov 5, 2022

Alexendoo Nov 5, 2022

jyn514 Nov 5, 2022

Alexendoo Nov 5, 2022

Alexendoo Nov 5, 2022

Alexendoo commented Nov 5, 2022

jyn514 commented Nov 17, 2022

bors commented Nov 19, 2022

jyn514 commented Apr 26, 2023

Alexendoo commented Apr 26, 2023

Expose compiletest as a library for use in Clippy #103266

Expose compiletest as a library for use in Clippy #103266

Conversation

Alexendoo commented Oct 19, 2022 • edited Loading

Mark-Simulacrum commented Oct 20, 2022

Alexendoo commented Oct 24, 2022

bors commented Oct 24, 2022

jyn514 commented Oct 27, 2022

Alexendoo commented Oct 27, 2022 • edited Loading

flip1995 commented Oct 28, 2022 • edited Loading

bors commented Oct 29, 2022

bors commented Nov 5, 2022

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexendoo Nov 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexendoo commented Nov 5, 2022

jyn514 commented Nov 17, 2022

bors commented Nov 19, 2022

jyn514 commented Apr 26, 2023

Alexendoo commented Apr 26, 2023

Alexendoo commented Oct 19, 2022 •

edited

Loading

Alexendoo commented Oct 27, 2022 •

edited

Loading

flip1995 commented Oct 28, 2022 •

edited

Loading

Alexendoo Nov 5, 2022 •

edited

Loading