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

compiletest: compare-mode false negatives w.r.t. long type file path #136510

Open
jieyouxu opened this issue Feb 3, 2025 · 4 comments · May be fixed by #136865
Open

compiletest: compare-mode false negatives w.r.t. long type file path #136510

jieyouxu opened this issue Feb 3, 2025 · 4 comments · May be fixed by #136865
Assignees
Labels
A-compiletest Area: The compiletest test runner A-compiletest-compare-modes Area: compiletest compare-modes A-compiletest-normalizations Area: compiletest normalizations A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. 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.

Comments

@jieyouxu
Copy link
Member

jieyouxu commented Feb 3, 2025

Random comment while I was in the neighborhood.

When using -Zwrite-long-types-to-disk=yes, we usually normalize away the hash in the long type file name, but that's not enough. Using a compare-mode will embed the compare mode in the long type file path as well.

The path note: the full name for the type has been written to '$TEST_BUILD_DIR/diagnostic-width/E0271.unicode/E0271.long-type-hash.txt' will become note: the full name for the type has been written to '$TEST_BUILD_DIR/diagnostic-width.$compare-mode/E0271.unicode/E0271.long-type-hash.txt' and the test will fail.

I encountered this while looking over the test failures under the new solver, i.e. ./x test bla --compare-mode=next-solver. While this specific test will currently fail for different reasons, it should pass in general without any changes required to diagnostics. (you can check that this test will fail under e.g. --compare-mode polonius)

For that, we'd need either:

  • compiletest to take care of all normalizations needing to be done due to compare-modes
  • expand what we manually normalize in the tests, taking care of the compare-mode suffix, as well as the possible hash

Since we already do part of the latter, I personally went with option 2 in #136310, and expanded the regex to look for the complete file path to remove the subdirectories.

For cases that have a hash suffix to normalize away, like in this E0271 test, it looks like:

// The regex below normalizes the long type file name to make it suitable for compare-modes.
//@ normalize-stderr: "'\$TEST_BUILD_DIR/.*\.long-type-\d+.txt'" -> "'$$TEST_BUILD_DIR/$$FILE.long-type-hash.txt'"

One could remove the comment to avoid reblessing the new line numbers. (Or simplify the regex, I went with making sure only this path output would be changed. The hash suffix normalization doesn't do that, nor did the .nll/ -> "" compare-mode normalization)

I already took care of long-E0308 and so on in that other PR (so they will conflict/require reblessing), but the same issue will also appear in the new long-e0277.rs test here.

Thanks for coming to my TED talk.

Originally posted by @lqd in #136328 (comment)

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 3, 2025
@jieyouxu jieyouxu added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-bug Category: This is a bug. A-compiletest Area: The compiletest test runner and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 3, 2025
@jieyouxu jieyouxu changed the title compare-mode false negatives w.r.t. long type file path compiletest: compare-mode false negatives w.r.t. long type file path Feb 3, 2025
@lqd
Copy link
Member

lqd commented Feb 3, 2025

btw it happens in more cases than only long type file paths: for paths in general, since they encode the test path and so on, the compare-mode changes these.

@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 3, 2025

scared.jpg

@jieyouxu jieyouxu added A-compiletest-normalizations Area: compiletest normalizations A-compiletest-compare-modes Area: compiletest compare-modes labels Feb 3, 2025
@jieyouxu jieyouxu self-assigned this Feb 3, 2025
@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 3, 2025

@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 4, 2025

If only the specific test output directory changes when --compare-mode is specified, we can add another expansion variable like {{TEST_SPECIFIC_BASE}} or something which normalizes even more than {{TEST_BSAE}} or whatever the existing one is called. Blocked on #136542 to avoid making the build directory logic / normalization logic even more convoluted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner A-compiletest-compare-modes Area: compiletest compare-modes A-compiletest-normalizations Area: compiletest normalizations A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. 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
3 participants