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

Add error annotations in UI tests #11249

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

GuillaumeGomez
Copy link
Member

As discussed on zulip, this PR adds missing error annotations in UI tests.

I used this script to generate them:

import os


def handle_err(line, stderr, elems, kind, i):
    msg = line.split("{}: ".format(kind), 1)[1]
    i += 1
    try:
        line_nb = int(stderr[i].split(":")[1])
    except Exception:
        return i
    in_macro = False
    note_found = False
    help_found = False
    elem = {"kind": kind, "msg": msg, "line": line_nb, "notes": [], "helps": []}
    while i < len(stderr):
        if len(stderr[i]) == 0:
            break
        elif stderr[i].startswith("note:"):
            note_found = True # error checker doesn't like multi-note apparently.
        elif stderr[i].startswith("   = note:"):
            if not note_found and not help_found and "this error originates in the macro" not in stderr[i]:
                elem["notes"].append(stderr[i].split("   = note:", 1)[1].strip())
            note_found = True # error checker doesn't like multi-note apparently.
        elif stderr[i].startswith("   = help:") or stderr[i].startswith("help:"):
            help_found = True
        # elif stderr[i].startswith("help:"):
        #     if not help_found:
        #         elem["helps"].append(stderr[i].split("help:", 1)[1].strip())
        #         help_found = True # error checker doesn't like multi-help apparently.
        elif "in this macro invocation" in stderr[i]:
            in_macro = True
        i += 1
    if not in_macro:
        elems.append(elem)
    return i


def show_kind(kind):
    if kind == "error":
        return "ERROR"
    elif kind == "warning":
        return "WARNING"
    elif kind == "note":
        return "NOTE"
    return "HELP"


def generate_code_err(indent, elem, up):
    content = "{}//~{} {}: {}".format(indent, up, show_kind(elem["kind"]), elem["msg"])
    if up == "^":
        up = "|"
    for note in elem["notes"]:
        content += "\n{}//~{} {}: {}".format(indent, up, show_kind("note"), note)
    for help_msg in elem["helps"]:
        content += "\n{}//~{} {}: {}".format(indent, up, show_kind("help"), help_msg)
    return content, up


def update_content(p, content):
    TO_IGNORE = [
        "needless_bool/simple.rs", # https://github.com/rust-lang/rust-clippy/issues/11248
        "crashes/ice-7868.rs", # Has errors but in another file.
        "trivially_copy_pass_by_ref.rs", # the `N` in the stderr needs to be replaced by the number
        "tests/ui/large_types_passed_by_value.rs", # the `N` in the stderr needs to be replaced by the number
    ]
    for to_ignore in TO_IGNORE:
        if p.endswith(to_ignore):
            return
    try:
        with open(p.replace(".rs", ".stderr"), "r", encoding="utf8") as f:
            stderr = f.read().split('\n')
    except Exception:
        return
    print("Updating `{}`".format(p))
    i = 0
    elems = []
    while i < len(stderr):
        line = stderr[i]
        if line.startswith("error: ") and not line.startswith("error: aborting due to"):
            i = handle_err(line, stderr, elems, "error", i)
        elif line.startswith("warning: ") and not line.endswith("warning emitted") and line.endswith("warnings emitted"):
            i = handle_err(line, stderr, elems, "warning", i)
        i += 1
    elems.sort(key=lambda e: e["line"], reverse=True)
    i = 0
    while i < len(elems):
        elem = elems[i]
        indent = ""
        c = 0
        line = content[elem["line"] - 1]
        while c < len(line) and line[c] == ' ':
            indent += " "
            c += 1
        new_content, up = generate_code_err(indent, elem, "^")
        i += 1
        while i < len(elems) and elems[i]["line"] == elem["line"]:
            elem = elems[i]
            ret = generate_code_err(indent, elem, up)
            new_content += "\n" + ret[0]
            up = ret[1]
            i += 1
        content.insert(elem["line"], new_content)
    with open(p, "w", encoding="utf8") as f:
        f.write("\n".join(content))


def check_if_contains_ui_test(p):
    if not p.endswith(".rs"):
        return
    with open(p, "r", encoding="utf8") as f:
        x = f.read()
    if "//~" not in x and "@run-rustfix" not in x and "@aux-build" not in x:
        update_content(p, x.split("\n"))


for path, subdirs, files in os.walk("tests/ui"):
    for name in files:
        check_if_contains_ui_test(os.path.join(path, name))

Then ran cargo uibless.

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Jul 28, 2023

r? @blyxyas

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 28, 2023
@GuillaumeGomez GuillaumeGomez force-pushed the ui-tests-annotations branch 2 times, most recently from 4cefbcf to 3723670 Compare July 28, 2023 19:35
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jul 28, 2023

Fixed the very weird differences between rustc's ui tests and clippy's. Also correctly handled comments length. Should be ready now.

@GuillaumeGomez
Copy link
Member Author

r? @llogiq

@rustbot rustbot assigned llogiq and unassigned blyxyas Jul 28, 2023
@bors
Copy link
Contributor

bors commented Jul 28, 2023

☔ The latest upstream changes (presumably #11250) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflict (this PR is going to be very merge conflict prone ><).

@bors
Copy link
Contributor

bors commented Aug 1, 2023

☔ The latest upstream changes (presumably #11269) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -12,27 +12,46 @@ fn main() {
const Z: u32 = 0;
let u: u32 = 42;
u <= 0;
//~^ ERROR: this comparison involving the minimum or maximum element for this type con
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these could use //~ instead for ones that are a single error then that'd be great, would require tons of manual work though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep having the message level. The goal is to ensure both that the message content and the message level don't get updated unnoticed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean having it inline, not omitting the level, like

    u <= 0; //~ ERROR: etc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, misunderstood. So yes, a lot of manual work which I don't have time for, sorry about that... Do you mind if we keep it this way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, it's fine as is. Maybe we can update the script you used at some point to do this instead

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflicts. Would be nice if it didn't take too long to merge if possible. ^^'

@Centri3 Centri3 added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Aug 10, 2023
@Centri3
Copy link
Member

Centri3 commented Aug 10, 2023

Nominating to see if everyone's onboard with this in 2 weeks. If we are, then this can probably be merged fine. (Though it'll result in a ton of conflicts, how fun!)

Alternatively, we could also create a poll in a new thread so it doesn't take 2 weeks. I think everyone's onboard with this already so it may not be worth the time. How does that sound?

@bors
Copy link
Contributor

bors commented Aug 11, 2023

☔ The latest upstream changes (presumably #11316) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

I'll keep it up-to-date as much as possible (for example, gonna fix the current conflict :D ).

@bors
Copy link
Contributor

bors commented Aug 11, 2023

☔ The latest upstream changes (presumably #11239) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez GuillaumeGomez force-pushed the ui-tests-annotations branch 2 times, most recently from 0fcad09 to ab5bfee Compare August 14, 2023 12:05
@bors
Copy link
Contributor

bors commented Aug 15, 2023

☔ The latest upstream changes (presumably #11336) made this pull request unmergeable. Please resolve the merge conflicts.

@llogiq
Copy link
Contributor

llogiq commented Aug 16, 2023

r=me once we have everyone on board next Tuesday shortly after 5pm CEST. Perhaps rebase during the hours before, then we can merge quickly without requiring multiple rebases.

@GuillaumeGomez
Copy link
Member Author

Noted! Please ping me when I can make the final rebase.

@llogiq
Copy link
Contributor

llogiq commented Aug 22, 2023

Hey @GuillaumeGomez ! We've decided to merge this, so r=me with rebase

@bors delegate+

@bors
Copy link
Contributor

bors commented Aug 22, 2023

✌️ @GuillaumeGomez, you can now approve this pull request!

If @llogiq told you to "r=me" after making some further change, please make that change, then do @bors r=@llogiq

@Centri3
Copy link
Member

Centri3 commented Aug 22, 2023

I believe this should also change mode to Fail (by default) with require_patterns set to true. We should document this before doing this though, so that should probably be a separate PR.

@oli-obk, can you confirm this is what we want? Or would we need a new mode?

@Centri3 Centri3 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Aug 22, 2023
@GuillaumeGomez
Copy link
Member Author

Do you wan the new mode to be set in this PR directly or in a follow-up one?

@Centri3
Copy link
Member

Centri3 commented Aug 22, 2023

Follow-up, once the book is updated

@oli-obk
Copy link
Contributor

oli-obk commented Aug 22, 2023

@oli-obk, can you confirm this is what we want? Or would we need a new mode?

Nope, this is the right setting

@GuillaumeGomez
Copy link
Member Author

@bors r=Centri3,llogiq

@bors
Copy link
Contributor

bors commented Aug 22, 2023

📌 Commit f467012 has been approved by Centri3,llogiq

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 22, 2023

⌛ Testing commit f467012 with merge 4be90d0...

@bors
Copy link
Contributor

bors commented Aug 22, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3,llogiq
Pushing 4be90d0 to master...

@bors bors merged commit 4be90d0 into rust-lang:master Aug 22, 2023
@GuillaumeGomez GuillaumeGomez deleted the ui-tests-annotations branch August 22, 2023 16:06
@GuillaumeGomez
Copy link
Member Author

I'll send the follow-up PR to turn the setting on in the next days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants