Selectively replace instances of error-pattern with check-run-results
#143537
+188
−237
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
PG-exploit-mitigations
This comment has been minimized.
This comment has been minimized.
Oneirical
force-pushed
the
uncountable-infinity
branch
from
211c933 to
5abe0ed
Compare
This comment has been minimized.
This comment has been minimized.
Oneirical
force-pushed
the
uncountable-infinity
branch
from
5abe0ed to
168f65c
Compare
This comment has been minimized.
This comment has been minimized.
Oneirical
force-pushed
the
uncountable-infinity
branch
from
168f65c to
4b544a1
Compare
This comment has been minimized.
This comment has been minimized.
Oneirical
force-pushed
the
uncountable-infinity
branch
from
4b544a1 to
ca0bf4b
Compare
Oneirical
changed the title
error-pattern with //~? ERROR in tests/uierror-pattern with check-run-results
|
The job Click to see the possible cause of the failure (guessed by this bot) |
jieyouxu
added
the
S-waiting-on-review
|
I'll still need to look at this, but you might find my comment in the other PR useful for context |
|
Hm yeah, looking at this, I believe my comment on the other PR still holds:
|
jieyouxu
reviewed
Jul 8, 2025
There was a problem hiding this comment.
Thanks for doing this experiment!
I think it is a non-goal to remove //@ error-pattern, but rather the original issue #134889 was proposing to double-check if //@ check-run-results should be added, and even if so, there probably are core strings that should be checked by //@ error-pattern.
check-run-resultswill snapshot and compare the exact stderr. However, the exact stderr is easy to accidentally bless away.error-patternorregex-error-patternis there to additionally ensure the key things we're looking for are still there, i.e. harder to accidentally bless away. This can be useful if you consider that (1) the exact stderr should be snapshotted and (2) there are specific key bits that you want to ensure its presence.If anything, usually only having
check-run-resultscan be suspicious.
I left a few initial remarks, I'll look at this more closely tmrw to see how we can pivot this into making it more clear what the use case of these two directives are.
Comment on lines
1
to
5
| //@ run-fail | ||
| //@ error-pattern:index out of bounds | ||
| //@ check-run-results | ||
| //@ needs-subprocess | ||
|
|
||
| use std::mem::size_of; |
There was a problem hiding this comment.
Remark: More concretely, I think we cannot attempt to capture run-time panic output, because the backtrace mechanism, std line numbers of underlying panic impls / macros, etc. can change between targets and is prone to breaking.
Comment on lines
8
to
-9
| // Verify that the expected source code is shown. | ||
| //@ error-pattern: pub struct SomeStruct {} // This line should be show |
There was a problem hiding this comment.
Remark: for tests like this, I believe it's intentional that we do not snapshot exact run output, because the exact paths themselves are not important but rather it's
The actual error is irrelevant. The important part it that is should show a snippet of the dependency's source.
Comment on lines
-1
to
-3
| //@ error-pattern: r#T: 'static | ||
| //@ error-pattern: r#K: 'static | ||
| //@ error-pattern: T: 'static= |
There was a problem hiding this comment.
Remark: for this test though, I do wonder why this is error-pattern
Comment on lines
-2
to
-4
| //@ error-pattern:assertion `left matches right` failed: 1 + 1 definitely should be 3 | ||
| //@ error-pattern: left: 2 | ||
| //@ error-pattern: right: 3 |
There was a problem hiding this comment.
Remark: for example, I believe this intentionally does not try to capture full run output, because the exact panic does not matter, and only the core substrings do matter.
Comment on lines
-2
to
3
| //@ error-pattern: a call in this macro requires a mutable binding due to mutable borrow of `d` | ||
| //@ check-run-results | ||
| //FIXME: remove error-pattern (see #141896) |
There was a problem hiding this comment.
Remark: I want to say this can be
//~? a call in this macro requires a mutable binding due to mutable borrow of `d`
but I think //~? had limitations in that it only matched on rustc check/build output?
| @@ -1,7 +1,7 @@ | |||
| //@ run-fail | |||
| //@ ignore-i686-pc-windows-msvc: #112480 | |||
| //@ compile-flags: -C debug-assertions | |||
| //@ error-pattern: misaligned pointer dereference: address must be a multiple of 0x4 but is | |||
There was a problem hiding this comment.
Remark: same here, the exact assertion shape or content is not as important as having a core substring
| @@ -1,5 +1,5 @@ | |||
| //@ run-fail | |||
| //@ error-pattern:location-mod-assign-by-zero.rs | |||
There was a problem hiding this comment.
Remark: I believe these intentionally want to check for the file name in panic messages, but only to a file granularity (cf. #114814)
Comment on lines
-4
to
5
| //@ error-pattern: overflow | ||
| //@ check-run-results | ||
| //@ compile-flags: -C overflow-checks=yes |
There was a problem hiding this comment.
Remark: for example, only "overflow" within panic message matters and less about the exact panic message
Comment on lines
-4
to
-5
| //@ error-pattern: panic in a function that cannot unwind | ||
| //@ error-pattern: Noisy Drop |
There was a problem hiding this comment.
Remark: this is an example where //@ check-run-results is not a replacement of //@ error-pattern but rather complementary, because //@ error-pattern makes sure you can't accidentally bless away the run snapshots.
| @@ -1,6 +1,6 @@ | |||
| //@ run-fail | |||
| //@ compile-flags: -Copt-level=3 -Cdebug-assertions=no -Zub-checks=yes | |||
| //@ error-pattern: unsafe precondition(s) violated: ptr::copy requires | |||
There was a problem hiding this comment.
Remark: for example, here this message is critical for test intention yet not the exact panic message
|
Some changes occurred in tests/ui/sanitizer cc @rcvalle |
|
@rustbot author |
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
☔ The latest upstream changes (presumably #143667) made this pull request unmergeable. Please resolve the merge conflicts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I combed through the stderr of every
error-patterntest, looking for patterns that would be troublesome in acheck-run-resultscontext.The following should keep
error-pattern:hashmap-capacity-overflow.rsoverflowing-neg-nonzero.rsoverflowing-pow-signed.rsoverflowing-pow-unsigned.rscopy-nonoverlapping.rscopy.rsstr-get_unchecked.rstest-tasks-invalid-value.rsThe reason these are problematic is because they contain line numbers in their
stderrwhich refers to parts of the Rust compiler. For example,copy-nonoverlapping.rs's error message starts withthread 'main' panicked at /rustc/FAKE_PREFIX/library/core/src/ptr/mod.rs:522:5:. That means that changing this unrelated file at any point in the future would result in this test failing.After this, there are hundreds of tests which contain lines such as
thread 'main' panicked at /checkout/tests/ui/precondition-checks/slice-from-raw-parts-mut.rs:13:30:These would start failing if the test was ever modified to have additional lines. However, this vulnerability is contained to the test itself, and in almost all cases, the line number is actually important for the test's purpose. Therefore, I think this would be unwise to normalize away these numbers.
If we aim to remove
error-patterncompletely, a possible approach would be to normalize the stderr of these flaky tests to remove the paths leading to the Rust compiler files.If you would like to review my methodology, here is the log I used to review the test stderrs: https://triage.rust-lang.org/gha-logs/rust-lang/rust/45509161982
Closes #134889