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

Detect typos for compiletest test directives #121561

Merged
merged 2 commits into from
Mar 10, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Feb 24, 2024

Checks directives against a known list of compiletest directives collected during migration from legacy-style compiletest directives. A suggestion for the best matching known directive will be made if an invalid directive is found.

This PR does not attempt to implement checks for Makefile directives because they still have the problem of regular comments and directives sharing the same comment prefix #.

Closes #83551.

@jieyouxu jieyouxu marked this pull request as ready for review February 24, 2024 17:33
@rustbot
Copy link
Collaborator

rustbot commented Feb 24, 2024

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 24, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu force-pushed the compiletest-directive-typo-check branch from 7ecce91 to f285c6e Compare February 24, 2024 18:21
@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu force-pushed the compiletest-directive-typo-check branch from f285c6e to c230627 Compare February 24, 2024 18:56
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu force-pushed the compiletest-directive-typo-check branch from e7e58a0 to 6579112 Compare February 24, 2024 20:32
@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu marked this pull request as draft February 24, 2024 21:21
@jieyouxu jieyouxu force-pushed the compiletest-directive-typo-check branch from 6579112 to 1e86c63 Compare February 25, 2024 11:39
@jieyouxu jieyouxu marked this pull request as ready for review February 25, 2024 12:05
Comment on lines 82 to 84
// Edit distance taken from rustc's `rustc_span::edit_distance`.

/// Finds the [edit distance] between two strings.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Edit distance taken from rustc's `rustc_span::edit_distance`.
/// Finds the [edit distance] between two strings.
/// Edit distance taken from rustc's `rustc_span::edit_distance`.
///
/// Finds the [edit distance] between two strings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to a top-level comment for the edit_distance module.

@@ -23,7 +23,6 @@ mod typeof_1 {
use super::*;
type Opaque = impl Sized;
fn define() -> Opaque {
//[current]~^ ERROR concrete type differs
Copy link
Member

Choose a reason for hiding this comment

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

Q: Why these are removed?

Copy link
Member Author

@jieyouxu jieyouxu Feb 29, 2024

Choose a reason for hiding this comment

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

Tidy enforces that if a revision like current is associated with a known-bug directive, then it cannot have any revisioned //~ERRORs. At least that's what I understood from tidy's error message. This tidy check was not previously triggered because of the extra colon after //@ [current]...

Comment on lines 676 to 683
// tidy-alphabetical-start
const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we put tidy-alphabetical-start inside the list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, yeah that would make more sense...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

What are your thoughts on creating a separate module for this implementation? Putting these functions and explanations in the generic module util seems to complicate things a bit, in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I can move it out to some edit_distance module. This code is like copy-pasta'd (including this) 3 places over rustc/cargo/compiletest, ideally we would share a single crate for this, but alas...

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to new edit_distance module.

Comment on lines 963 to 1011
} else if let Some((header_revision, original_directive_line)) = line_directive(comment, ln)
{
// Let Makefiles and non-rs files just get handled in the regular way.
if testfile.extension().map(|e| e != "rs").unwrap_or(true) {
it(HeaderLine {
line_number,
original_line,
header_revision,
directive: original_directive_line,
});
continue;
}

let directive_ln = original_directive_line.trim();

if let Some((directive_name, _)) = directive_ln.split_once([':', ' ']) {
if KNOWN_DIRECTIVE_NAMES.contains(&directive_name) {
it(HeaderLine {
line_number,
original_line,
header_revision,
directive: original_directive_line,
});
continue;
} else {
*poisoned = true;
eprintln!(
"error: detected unknown compiletest test directive `{}` in {}:{}",
directive_ln,
testfile.display(),
line_number,
);

if let Some(suggestion) =
find_best_match_for_name(KNOWN_DIRECTIVE_NAMES, directive_name, None)
{
eprintln!("help: did you mean `{}` instead?", suggestion);
}
return;
}
} else if KNOWN_DIRECTIVE_NAMES.contains(&directive_ln) {
it(HeaderLine {
line_number,
original_line,
header_revision,
directive: original_directive_line,
});
continue;
} else {
*poisoned = true;
eprintln!(
"error: detected unknown compiletest test directive `{}` in {}:{}",
directive_ln,
testfile.display(),
line_number,
);

if let Some(suggestion) =
find_best_match_for_name(KNOWN_DIRECTIVE_NAMES, directive_ln, None)
{
eprintln!("help: did you mean `{}` instead?", suggestion);
}
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we write some unit tests for this logic (or maybe for the whole feature)?

Copy link
Member Author

Choose a reason for hiding this comment

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

or maybe for the whole compiletest But yeah, I'll think about how to test this / make this more testable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Simplified this logic and added some (integration) tests for iter_header and directives checking.

@onur-ozkan onur-ozkan 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 Feb 29, 2024
@jieyouxu jieyouxu force-pushed the compiletest-directive-typo-check branch from ffde4b6 to 32e713f Compare March 7, 2024 17:19
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu force-pushed the compiletest-directive-typo-check branch from a7ee012 to a816cc5 Compare March 7, 2024 18:38
@jieyouxu
Copy link
Member Author

jieyouxu commented Mar 7, 2024

Summary:

  • Moved edit_distance to its own module
  • Reworked directive detection logic to make it simpler
  • Added integration tests for iter_header directive checking
  • Added missing known directive needs-sanitizer-dataflow
  • Fixed tests:
    • tests/ui/borrowck/two-phase-reservation-sharing-interference.rs: missing @
    • tests/ui/type-alias-impl-trait/normalize-hidden-types.rs: extra : after revision name causes tidy check to not fire

@jieyouxu
Copy link
Member Author

jieyouxu commented Mar 7, 2024

@rustbot ready

@rustbot rustbot 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 Mar 7, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 9, 2024
…o-check, r=onur-ozkan

Detect typos for compiletest test directives

Checks directives against a known list of compiletest directives collected during migration from legacy-style compiletest directives. A suggestion for the best matching known directive will be made if an invalid directive is found.

This PR does not attempt to implement checks for Makefile directives because they still have the problem of regular comments and directives sharing the same comment prefix `#`.

Closes rust-lang#83551.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2024
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#121561 (Detect typos for compiletest test directives)
 - rust-lang#121567 (Avoid some interning in bootstrap)
 - rust-lang#121685 (Fixing shellcheck comments on lvi test script)
 - rust-lang#121833 (Suggest correct path in include_bytes!)
 - rust-lang#121860 (Add a tidy check that checks whether the fluent slugs only appear once)
 - rust-lang#121907 (skip sanity check for non-host targets in `check` builds)
 - rust-lang#122029 (When displaying multispans, ignore empty lines adjacent to `...`)
 - rust-lang#122221 (match lowering: define a convenient struct)
 - rust-lang#122244 (fix: LocalWaker memory leak and some stability attributes)
 - rust-lang#122251 (Add test to check unused_lifetimes don't duplicate "parameter is never used" error)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 10, 2024
…o-check, r=onur-ozkan

Detect typos for compiletest test directives

Checks directives against a known list of compiletest directives collected during migration from legacy-style compiletest directives. A suggestion for the best matching known directive will be made if an invalid directive is found.

This PR does not attempt to implement checks for Makefile directives because they still have the problem of regular comments and directives sharing the same comment prefix `#`.

Closes rust-lang#83551.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#112136 (Add std::ffi::c_str module)
 - rust-lang#113525 (Dynamically size sigaltstk in std)
 - rust-lang#121561 (Detect typos for compiletest test directives)
 - rust-lang#121685 (Fixing shellcheck comments on lvi test script)
 - rust-lang#121833 (Suggest correct path in include_bytes!)
 - rust-lang#121860 (Add a tidy check that checks whether the fluent slugs only appear once)
 - rust-lang#122125 (Revert back to Git-for-Windows for MinGW CI builds)
 - rust-lang#122221 (match lowering: define a convenient struct)
 - rust-lang#122251 (Add test to check unused_lifetimes don't duplicate "parameter is never used" error)
 - rust-lang#122264 (add myself to rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r- rollup=iffy

#122265

2024-03-10T06:25:45.0257378Z test [mir-opt] tests/mir-opt/inline/polymorphic_recursion.rs ... ok
2024-03-10T06:25:45.0257864Z 
2024-03-10T06:25:45.0259633Z test result: ok. 274 passed; 0 failed; 11 ignored; 0 measured; 0 filtered out; finished in 30.17s
2024-03-10T06:25:45.0260248Z 
2024-03-10T06:25:45.0262781Z ##[endgroup]
2024-03-10T06:25:45.0265007Z [TIMING] core::build_steps::test::Compiletest { compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu, mode: "mir-opt", suite: "mir-opt", path: "tests/mir-opt", compare_mode: None } -- 30.313
2024-03-10T06:25:45.0506328Z [TIMING] core::build_steps::test::MirOpt { compiler: Compiler { stage: 1, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu } -- 0.024
2024-03-10T06:25:45.0549273Z ##[group]Testing stage1 compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu)
2024-03-10T06:25:45.1679235Z error: detected unknown compiletest test directive `filecheck-flags: --check-prefix=CUSTOM` in /checkout/tests/codegen/meta-filecheck/filecheck-flags.rs:4
2024-03-10T06:25:45.1680772Z errors encountered during EarlyProps parsing: /checkout/tests/codegen/meta-filecheck/filecheck-flags.rs

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 10, 2024
- Fix invalid directive in `normalize-hidden-types`
- Update legacy directive in `two-phase-reservation-sharing-interference`
@jieyouxu jieyouxu force-pushed the compiletest-directive-typo-check branch from 046c28f to 64dda8c Compare March 10, 2024 13:14
@jieyouxu
Copy link
Member Author

Added missing known filecheck-flags directive.
@rustbot ready

@rustbot rustbot 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 Mar 10, 2024
@onur-ozkan
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 10, 2024

📌 Commit 64dda8c has been approved by onur-ozkan

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2024
@bors
Copy link
Contributor

bors commented Mar 10, 2024

⌛ Testing commit 64dda8c with merge af69f4c...

@bors
Copy link
Contributor

bors commented Mar 10, 2024

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing af69f4c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 10, 2024
@bors bors merged commit af69f4c into rust-lang:master Mar 10, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 10, 2024
@jieyouxu jieyouxu deleted the compiletest-directive-typo-check branch March 10, 2024 18:39
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (af69f4c): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.3% [0.9%, 1.6%] 2
Regressions ❌
(secondary)
3.1% [2.1%, 4.4%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.3% [0.9%, 1.6%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 646.803s -> 647.057s (0.04%)
Artifact size: 309.98 MiB -> 309.92 MiB (-0.02%)

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 12, 2024
…notriddle

rustdoc: fix up old test

`tests/rustdoc/line-breaks.rs` had several issues:

1. It used `//`@count`` instead of `// `@count`` (notice the space!) which gets treated as a `ui_test` directive instead of a `htmldocck` one. `compiletest` didn't flag it as an error because it's allowlisted ([rust-lang#121561](rust-lang#121561)) presumably precisely because of this test. And before the compiletest→ui_test migration, these directives must've been ignored, too, because …
2. … the checks themselves no longer work either: The count of `<br>`s is actually 0 in all 3 cases because – well – we no longer generate any `<br>`s inside `<pre>`s.

Since I don't know how to ``@count`` `\n`s instead of `<br>`s, I've turned them into ``@matches`.` Btw, I don't know if this test is still desirable or if we have other tests that cover this (I haven't checked).

r? rustdoc
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2024
Rollup merge of rust-lang#122355 - fmease:rustdoc-fix-up-old-test, r=notriddle

rustdoc: fix up old test

`tests/rustdoc/line-breaks.rs` had several issues:

1. It used `//`@count`` instead of `// `@count`` (notice the space!) which gets treated as a `ui_test` directive instead of a `htmldocck` one. `compiletest` didn't flag it as an error because it's allowlisted ([rust-lang#121561](rust-lang#121561)) presumably precisely because of this test. And before the compiletest→ui_test migration, these directives must've been ignored, too, because …
2. … the checks themselves no longer work either: The count of `<br>`s is actually 0 in all 3 cases because – well – we no longer generate any `<br>`s inside `<pre>`s.

Since I don't know how to ``@count`` `\n`s instead of `<br>`s, I've turned them into ``@matches`.` Btw, I don't know if this test is still desirable or if we have other tests that cover this (I haven't checked).

r? rustdoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compiletest should check for possible typos in annotated test directives
8 participants