Skip to content

compiletest: technically potential false positives for filecheck annotations #125825

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
jieyouxu opened this issue May 31, 2024 · 3 comments
Closed
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jieyouxu
Copy link
Member

jieyouxu commented May 31, 2024

This is a fun one: in #121561 I added checks to error if we detect any legacy-directives-like comments (which are of the form // <known_directive_name>.

In codegen tests, we have filecheck directives that can be revisioned:

//@ revisions: known_directive_name
// known_directive_name: noundef

... but unfortunately, // known_directive_name looks like a legacy directive to compiletest so we might error here even though it is not actually a legacy directive (it just "looks" like one for the heuristic I added).

Technically a bug (false positive), but in practice changing the revision name to something else would sidestep this check.

Note: the legacy directive check would also fire on

// this is a block of comment that has a
// ignore in the middle of the sentence
// that looks like a legacy directive
@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. C-bug Category: This is a bug. A-compiletest Area: The compiletest test runner labels May 31, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 31, 2024
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 31, 2024
@jieyouxu
Copy link
Member Author

Realized this in #125813 (comment), the long term solution is probably to have compiletest directive handling be cooperative between "base" directives and test suite "customized" directives.

@workingjubilee
Copy link
Member

workingjubilee commented Jun 1, 2024

fwiw despite noting the issue, I think the prefix doesn't matter too much: I reviewed FileCheck's code (as much as I understood it, anyways) and it seems FileCheck is very simple. I don't think it is aware of any comment styles itself. I think it literally just scans for the prefix on a line and assumes you won't introduce code that resembles a FileCheck directive.

@jieyouxu
Copy link
Member Author

compiletest triage: I deleted the legacy directive check so this is no longer a problem.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
No open projects
Development

No branches or pull requests

3 participants