compiletest: add option for automatically adding annotations

[mejrs]

This PR adds the option --try-annotate to have compiletest write annotations like //~ERROR found Foo, expected Bar.

I need this to automatically generate tests for #140948 (comment) but I realized this is quite useful generally so I have split this up into its own PR.

I have tested this on our test suite by removing all targeted annotations (skipping revision tests and //~? and //~v), which results in 10k failing tests. By then running

python x.py test tests/ui -- --try-annotate 
python x.py test tests/ui -- --bless

all but 24 tests will pass again. Most of these tests either have opaque types (see the instability comment in the PR), involve syntax errors spread over multiple lines (again, see instability) or they are weird and should use a //~v annotation.

Failing tests

[ui] tests\ui\attributes\export\crate-type-2.rs
[ui] tests\ui\coroutine\clone-impl.rs
[ui] tests\ui\feature-gates\feature-gate-frontmatter.rs
[ui] tests\ui\fmt\format-string-error-2.rs
[ui] tests\ui\frontmatter\multifrontmatter-2.rs
[ui] tests\ui\frontmatter\frontmatter-whitespace-1.rs
[ui] tests\ui\frontmatter\mismatch-2.rs
[ui] tests\ui\frontmatter\mismatch-1.rs
[ui] tests\ui\frontmatter\unclosed-3.rs
[ui] tests\ui\frontmatter\unclosed-1.rs
[ui] tests\ui\frontmatter\unclosed-2.rs
[ui] tests\ui\frontmatter\unclosed-4.rs
[ui] tests\ui\frontmatter\unclosed-5.rs
[ui] tests\ui\impl-trait\issue-55872-3.rs
[ui] tests\ui\lexer\lexer-crlf-line-endings-string-literal-doc-comment.rs
[ui] tests\ui\macros\issue-83344.rs
[ui] tests\ui\methods\filter-relevant-fn-bounds.rs
[ui] tests\ui\parser\bad-char-literals.rs
[ui] tests\ui\parser\doc-before-identifier.rs
[ui] tests\ui\parser\doc-comment-in-stmt.rs
[ui] tests\ui\rustdoc\check-doc-alias-attr.rs
[ui] tests\ui\str\str-escape.rs
[ui] tests\ui\type-alias-impl-trait\mututally-recursive-overflow.rs
[ui] tests\ui\typeck\typeck_type_placeholder_item.rs

Script for deleting annotations, in case you want to try it out

use std::ffi::OsStr;
use std::fs::OpenOptions;
use std::io;
use std::io::BufRead;
use std::io::BufReader;
use std::io::Seek;
use std::io::Write;
use std::path::Path;
use walkdir::WalkDir;

const ROOT: &str = "C:/Users/bruno/Rust/rust/tests/ui";

fn main() -> io::Result<()> {
    for entry in WalkDir::new(ROOT) {
        let entry = entry?;
        if entry.path().extension() == Some(OsStr::new("rs")) {
            delete_annotations(entry.path())?;
            println!(" cleaned {}", entry.path().display());
        }
    }

    Ok(())
}

fn delete_annotations(path: &Path) -> io::Result<()> {
    let mut file = OpenOptions::new().write(true).read(true).open(path)?;
    let br = BufReader::new(&file);
    let mut lines = br.lines().collect::<Result<Vec<_>, _>>()?;
    for line in &lines {
        if line.contains("//@ revisions") || line.contains("//@revisions") {
            return Ok(());
        }
    }

    file.set_len(0).unwrap();
    file.rewind().unwrap();

    for line in lines {
        if line.contains("//~?") || line.contains("//~v") {
            writeln!(file, "{line}")?;
        } else if let Some(bye_annotation) = line.split_terminator("//~").next() {
            writeln!(file, "{}", bye_annotation.trim_end())?;
        } else {
            writeln!(file, "{line}")?;
        }
    }
    file.flush()?;
    Ok(())
}

Also, this PR stops immediately formatting the error messages. It proved quite painful to extract the original error again, so instead I implemented this type which is just formatted when needed.

pub struct ErrorMessage {
    pub span: Option<Location>,
    pub code: Option<DiagnosticCode>,
    pub text: String,
}

r? @jieyouxu

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[fmease]

Error annotation can't be blessed for a reason. They're not intended to be blessed.

[jieyouxu]

I don't want land this, because I don't want to encourage "auto" error annotations. Agreed w/ error annotations can't be automatically bless is a feature, not a bug.

[jieyouxu]

I have some ideas for #140948, I just haven't gotten around to writing them down¹. However, this is IMO the wrong solution for that purpose.

Footnotes

1. Scheduled to write them down approx. tmrw/Saturday. ↩

[mejrs]

I don't want to encourage "auto" error annotations

I am not seeing the problem. This is not intended as an auto tool. You still have to check that the annotation makes sense. Just like how you're not supposed to bless error output blindly; you still have to check that the error message is what you expect, that nothing disappeared or got mangled or something.

[mejrs]

Also, please don't misunderstand me. I am by no means suggesting that we re-annotate the entire UI suite. It's just an illustration to show that it produces valid output in the vast majority of cases.

[jieyouxu]

I don't want to encourage "auto" error annotations

Why not? This is not intended as an auto tool. You still have to check that the annotation makes sense. Just like how you're not supposed to bless error output blindly; you still have to check that the error message is what you expect, that nothing disappeared or got mangled or something.

Error annotations are themselves a safeguard against accidental stderr changes from reblessing. That you have to manually specify and scrutinize error annotations strongly nudges PR authors towards making sure error annotations make sense and are what they expect. I don't want to make this process automatic at all, even as an opt-in. Because if you're able to automatically add error annotations, PR authors will pay less attention to error annotations.

Furthermore, I believe the property that the test files themselves are not modified by compiletest in place is important.

[jieyouxu]

As an concrete example, I don't want to encourage the use case in #140948:

Just these couple of edits have already been quite annoying and error prone. Doing this for dozens of attributes is going to be untenable in the long run.

This needs to be automatically generated, and I have some ideas about how to do that.

IMHO, this is not the right approach, let me elaborate to share what I mean (give me a moment, I need some coffee).

[jieyouxu]

I wrote down my thoughts on the challenges and caveats on testing attribute positions.

Auto-generating error annotations?

Looking at the current approach in #140948, even 3 attributes is already proving to be problematic:

• It's the "same test", copied 3x. Now imagine if we had $builtin_attribute_count copies and you want to fix a typo in a diagnostic message -- that's going to be super painful.
• If you want to update the "template" to cover previously missing positions, e.g. say if you want to work on stmt_expr_attributes, now you'd have to modify all $builtin_attribute_count copies.

Clearly, this doesn't scale. Auto-generating error annotations does not mitigate this maintenance challenge, because once you have $builtin_attribute_count copies of the same thing, it becomes very hard to have the attention to carefully audit those error annotation differences. And like, I don't want to encourage auto-generating error annotations for this kind of use case -- it suggests to me that we should reconsider the testing approach.

[jieyouxu]

Still thinking about how to better implement test coverage for #140948, going to be at least tmrw.

[jieyouxu]

cc @petrochenkov as you were improving error annotations.

[mejrs]

Error annotation can't be blessed for a reason. They're not intended to be blessed.

I don't want land this, because I don't want to encourage "auto" error annotations. Agreed w/ error annotations can't be automatically bless is a feature, not a bug.

I'm not sure what the definition of "bless" is, but if it means "make the ui test pass" then this is not that. Which is why it is a separate option and not covered under --bless. For example, this does not remove error annotations, it just adds them. So, you are messing around in the compiler and accidentally comment out some code that emits an error message, this option will not remove the annotation for you and your test will still fail.

That you have to manually specify and scrutinize error annotations strongly nudges PR authors towards making sure error annotations make sense and are what they expect. I don't want to make this process automatic at all, even as an opt-in. Because if you're able to automatically add error annotations, PR authors will pay less attention to error annotations.

I think the opposite is true. If I make it easy for others to add annotations that leaves more mental energy for contributors to review their results and check that it is what they expected. Anyway, this is irrelevant for the people who aren't paying attention - if you make the process more tedious for them they're just going to try to pay less attention. At least with this option it'll add the whole error message rather than the bare //~ERROR or //~ERROR two or three words that I can see throughout the test suite.

Clearly, this doesn't scale. Auto-generating error annotations does not mitigate this maintenance challenge, because once you have $builtin_attribute_count copies of the same thing, it becomes very hard to have the attention to carefully audit those error annotation differences.

I disagree that it doesn't scale. I imagine that maintaining those tests can look like:

• create a new branch.
• wipe all the existing position tests and stderrs
• change the template if necessary, to add a new thing you want to test for example.
• from the template, regenerate all tests
• using this option, get compiletest to add error annotations
• bless the tests to get the stdout back
• check it out in in git and review the differences

Only this last step requires active thinking, which is exactly what you want. That makes it easy to audit the differences.

And like, I don't want to encourage auto-generating error annotations for this kind of use case -- it suggests to me that we should reconsider the testing approach.

I'm happy to hear about alternative ideas. However I feel there's a strong consensus we need better test coverage in this area.

[jieyouxu]

Let me ask other team members for a vibe check.

[jieyouxu]

I'll gauge the sentiment for this functionality by this Thursday, and then decide to merge this or not.

[jieyouxu]

Okay. We discussed this in:

• #t-compiler > Are `ERROR` annotations worth it?, and
• #t-compiler > Vibe check on compiletest option to add error annotations.

At least one team member expressed they'd want sth like this, and there are no strong objections, so I'm fine with landing this experimentally.

Note that I might not get to reviewing this today, I scheduled this PR for this Friday.

[petrochenkov]

I'm fine with this as long as the option is not documented as a recommended way to do things (and maybe documented as non-recommended instead).
Actually, I'll look at the code a bit later today as well.

[jieyouxu]

FWIW @RalfJung has a design concern, I'll have to think about this some more. ATM I'm busy on the stage 0 bootstrap redesign work, I'll probably only get to the design of this next week.

cf. a related thread: #t-compiler > Are `ERROR` annotations worth it?

[mejrs]

A similar concern is things like opaque types, error messages often mention them like {async block@src/main.rs:8:13: 8:18}, we'd need to normalize to {async block@src/main.rs:LL:CC} at least.