UItest error annotations for errors in auxiliary crates are marked as superfluous

[jdonszelmann]

After a discussion on the community discord with @jieyouxu

This pr has tests (in tests/ui/eii) that have errors in auxiliary crates. Until recently (before a rebase yesterday. I've not completely figured out when it stopped working) this was fine. In the aux crate there's also a marking (//~ error ...). However, after the rebase it complains that there's errors without a span being emitted while none were expected. This is in some sense true, errors are emitted with spans from a different crate.

as an example:

the stderr of one of my tests:

and yet:

where the aux crate is annotated with:

in other words, this is an expected error.

[jieyouxu]

• There's an edge case where for a diagnostic emitted against this source file (primary test crate) actually has spans pointing to external (auxiliary crate) file(s). The logic for when to consider a diagnostic as "span-less" (so //~? annotation) currently doesn't account for this case.
• Some diagnostics use synthetic file names like "<crate attribute>", e.g. for malformed -Zcrate-attr.
• Some other ui tests fail when I adjust the heuristics, so still investigating.

Diagnostic JSON

{
    "$message_type": "diagnostic",
    "message": "multiple implementations of `#[eii1]`",
    "code": null,
    "level": "error",
    "spans": [
        {
            "file_name": "/home/joe/repos/rust/tests/ui/eii/duplicate/auxiliary/impl1.rs",
            "byte_start": 130,
            "byte_end": 146,
            "line_start": 10,
            "line_end": 10,
            "column_start": 1,
            "column_end": 17,
            "is_primary": true,
            "text": [
                {
                    "text": "fn other(x: u64) {",
                    "highlight_start": 1,
                    "highlight_end": 17
                }
            ],
            "label": "first implemented here in crate `impl1`",
            "suggested_replacement": null,
            "suggestion_applicability": null,
            "expansion": null
        },
        {
            "file_name": "/home/joe/repos/rust/tests/ui/eii/duplicate/auxiliary/impl2.rs",
            "byte_start": 130,
            "byte_end": 146,
            "line_start": 10,
            "line_end": 10,
            "column_start": 1,
            "column_end": 17,
            "is_primary": false,
            "text": [
                {
                    "text": "fn other(x: u64) {",
                    "highlight_start": 1,
                    "highlight_end": 17
                }
            ],
            "label": "also implemented here in crate `impl2`",
            "suggested_replacement": null,
            "suggestion_applicability": null,
            "expansion": null
        }
    ],
    "children": [
        {
            "message": "in addition to these two, more implementations were also found in the following crates: `impl3`, `impl4`",
            "code": null,
            "level": "note",
            "spans": [],
            "children": [],
            "rendered": null
        },
        {
            "message": "an \"externally implementable item\" can only have a single implementation in the final artifact. When multiple implementations are found, also in different crates, they conflict",
            "code": null,
            "level": "help",
            "spans": [],
            "children": [],
            "rendered": null
        }
    ],
    "rendered": "error: multiple implementations of `#[eii1]`\n  --> /home/joe/repos/rust/tests/ui/eii/duplicate/auxiliary/impl1.rs:10:1\n   |\nLL | fn other(x: u64) {\n   | ^^^^^^^^^^^^^^^^ first implemented here in crate `impl1`\n   |\n  ::: /home/joe/repos/rust/tests/ui/eii/duplicate/auxiliary/impl2.rs:10:1\n   |\nLL | fn other(x: u64) {\n   | ---------------- also implemented here in crate `impl2`\n   |\n   = note: in addition to these two, more implementations were also found in the following crates: `impl3`, `impl4`\n   = help: an \"externally implementable item\" can only have a single implementation in the final artifact. When multiple implementations are found, also in different crates, they conflict\n\n"
}

[jieyouxu]

I've done some more investigating, and after some consideration, I came back to think this current behavior is the right one. compiletest currently doesn't support ui error annotations on auxiliaries. I believe this is because diagnostics spans in general can point into arbitrary places e.g. dependencies or standard library sources where you obviously cannot add ui error annotations. So, in the case where the diag span points into external crates from the POV of the main test itself, I believe //? is indeed the correct behavior. //? lets us enforce that we emit desired errors messages against external crates/sources via annotations which can't just be accidentally blessed away. cc @petrochenkov in case you have differing opinions.

That annotation in auxiliary does nothing, you can confirm by adding a random //~ ERROR never gonna give you up in the auxiliary and see nothing fails.¹

Footnotes

1. Arguably, this should be a tidy lint against auxiliary/*.rs files once we have a more enforced ui annotation shape à la https://github.com/rust-lang/rust/pull/139427. ↩

[jieyouxu]

I'm going to close this issue as intended, because I think it's important that we do check diagnostics (even those that do not have line info corresponding to main test file) exhaustively whereever feasible.