Skip to content

annotate-snippet emitter: Add UI test where annotated_files count is > 1 #64205

Closed
@phansch

Description

@phansch
Member

Part of #59346

It would be nice to have a UI test for the annotate-snippet emitter, that includes annotations that span multiple files. The goal of this issue is to find and add such a test.

Instructions

A simple way to look for this, would be to add this code:

if annotated_files.len() > 1 {
    let filename = primary_lo.file.name.to_string();
    eprintln!("annotated_files > 1 in {}", filename);
}

to the top of fn slices_for_files and then check the failing UI tests.

If there are failing UI tests, it means that those UI tests include a diagnostic that probably spans multiple files.

The next step, is to copy that file (and possible auxiliary files) over into src/test/ui/annotate-snippet/ and add // compile-flags: --error-format human-annotate-rs to the top of the UI test.

Once that's done, be sure to run the added test and open a PR including the blessed stderr files.

Activity

phansch

phansch commented on Sep 6, 2019

@phansch
MemberAuthor

Under the assumption, that we do have an existing UI test with annotations from multiple files, I'm happy to help out anyone who wants to take on this issue. Otherwise we'll have to write a test case ourselves.

@rustbot modify labels to +A-diagnostics, +E-mentor, +E-help-wanted

added
A-diagnosticsArea: Messages for errors, warnings, and lints
E-help-wantedCall for participation: Help is requested to fix this issue.
E-mentorCall for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
on Sep 6, 2019
added 2 commits that reference this issue on Sep 6, 2019

Rollup merge of rust-lang#64206 - phansch:update_issue_number, r=varkor

f9aa5d6

Rollup merge of rust-lang#64206 - phansch:update_issue_number, r=varkor

8111cca
bIgBV

bIgBV commented on Sep 7, 2019

@bIgBV

Hey @phansch I'd like to pick this issue up.

phansch

phansch commented on Sep 8, 2019

@phansch
MemberAuthor

@bIgBV sure, it's all yours! Let me know if you have any questions. The rustc-guide should help you to get started working on the compiler.

bIgBV

bIgBV commented on Sep 9, 2019

@bIgBV

@phansch I made the change suggested in the issue and ran all the ui tests (over 9000!). None of them failed, meaning a new test needs to be written for this specific usecase, correct?

phansch

phansch commented on Sep 9, 2019

@phansch
MemberAuthor

Thanks! Unfortunately I just realized I made a mistake in the instructions :/
If it's included in annotate_snippet_emitter_writer.rs it will only get executed for UI tests that use the new work-in-progress emitter, which only has one UI test currently

Instead, you'll want to include the snippet below this line in emitter.rs:

let mut annotated_files = FileWithAnnotatedLines::collect_annotations(msp, &self.sm);

If you could give that a try too, that would be great!

bIgBV

bIgBV commented on Sep 9, 2019

@bIgBV

Will do!

EDIT: @phansch re-ran it with the new changes, still no failures. I'm guessing that we need to add a new test.

bIgBV

bIgBV commented on Sep 13, 2019

@bIgBV

@phansch looks like edits on github don't generate notifications, apologies. But I retried the tests with the second change you suggested. Still no failing tests.

phansch

phansch commented on Sep 15, 2019

@phansch
MemberAuthor

@bIgBV Sorry, been out sick the past couple of days. Thanks for testing again!

It's weird that you don't get failing tests 🤔 Could you try to apply this specific patch?

diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs
index 0ce69eecc6..8a3fbb6e09 100644
--- a/src/librustc_errors/emitter.rs
+++ b/src/librustc_errors/emitter.rs
@@ -1185,6 +1185,10 @@ impl EmitterWriter {
             emit_to_destination(&buffer.render(), level, &mut self.dst, self.short_message)?;
             return Ok(());
         };
+        if annotated_files.len() > 1 {
+            let filename = primary_lo.file.name.to_string();
+            panic!("annotated_files > 1 in {}", filename);
+        }
         if let Ok(pos) =
             annotated_files.binary_search_by(|x| x.file.name.cmp(&primary_lo.file.name)) {
             annotated_files.swap(0, pos);

When you run ./x.py test src/test/ui with the patch you should see at least 5 failing UI tests (At least I do, but I didn't have time to take a closer look, yet)

bIgBV

bIgBV commented on Sep 18, 2019

@bIgBV

Okay, something is weird, after pulling in the latest master and making the exact changes in the patch you provided, there are over 8000 failing tests. I want to check if there is something obviously wrong going on, but I might not get back until tomorrow, as each test run takes close to an hour on my laptop.

12 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-diagnosticsArea: Messages for errors, warnings, and lintsC-enhancementCategory: An issue proposing an enhancement or a PR with one.E-help-wantedCall for participation: Help is requested to fix this issue.E-mentorCall for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @hdhoang@Enselic@crlf0710@phansch@bIgBV

      Issue actions

        annotate-snippet emitter: Add UI test where `annotated_files` count is > 1 · Issue #64205 · rust-lang/rust