Add option to write missing error annotations back to the test file.

[Jarcho]

Currently has no tests and it can only write error codes. There's also a problem that this does not take into account other annotations already in the file. e.g.

foo(err1, err2);
//~^ err1

Gets changed to:

foo(err1, err2);
//~^ err2
//~^ err1

This isn't really a big deal to manually fix up, but it would be better if this didn't happen.

---

This was successfully used on clippy, minus one file which had the aforementioned problem and a few times where rustfmt moved the added annotation.

[oli-obk]

I already rejected this in the rustc repo: rust-lang/rust#109080

While it may be convenient, me going through 100+ files individually has usually revealed a lot to me and caused me to do follow-up changes. I guess self-reviewing them could yield similar results, but it just feels wrong ^^.

Is this really a recurring large issue? Or a problem for beginners or sth?

[Jarcho]

This is different than just regenerating the annotations.

1. This isn't a command line option, but has to be set in the test code while as part of enabling yolo mode.
2. Annotations won't be written if the test fails without them. (e.g. one of the current annotations is wrong)
3. Only missing annotations will be added, existing ones aren't touched.

This is more of a way to assist with converting a test suite to use annotations. Not really a thing which would be used afterwards. I do agree that doing it all the time does defeat part of the purpose of having annotations.

[oli-obk]

I wonder if we can make this a standalone emitter instead. Should have all the information available, and can then be combined with an existing emitter, run on its own, or even turned into a runtime switchable mode, all without affecting the core ui_test engine at all

[xFrednet]

I've been trying to understand ui_test and these changes. Based on my current (most likely limited) understanding, I think that the emitter would be the wrong place or at least it would be inconsistent with how the normal "bless" works.

Edit: Never mind the later parts of my comment. I just saw that it was incorrect.

[oli-obk]

This is more of a way to assist with converting a test suite to use annotations. Not really a thing which would be used afterwards. I do agree that doing it all the time does defeat part of the purpose of having annotations.

yea makes sense. If it can be done as a standalone emitter, I'll take it in this crate just to avoid it bitrotting across version bumps, but then it could be written entirely outside this repo, which is good for encapsulation.

[oli-obk]

ping ^^

[Jarcho]

The current api for emitters would require buffering everything until the end since there's currently no way to tell when all the revisions for a test are completed. Adding an extra trait layer like StatusEmitter -> TestStatus -> RevisionStatus would be enough to fix that.

[oli-obk]

We do have

ui_test/src/status_emitter.rs

Line 46 in 680bb08

fn for_revision(&self, revision: &str) -> Box<dyn TestStatus>;

which should allow you to just create an Arc<AtomicU8> that you count up and down when adding a revision and when it finishes. When the last one finishes, do the overwriting action.

[oli-obk]

This can now be written as a custom flag. the entire rustfix logic is a custom flag now, and works similarly, so you should be able to implement it the same way.

[xFrednet]

@Jarcho Are there any updates on this implementation? I would really like to have this! :D

[xFrednet]

I'll look into continuing this implementation, but no promises though... I'm also on summer break, so my time is a bit all over the place.

I also wanted to add to a previous discussion

Is this really a recurring large issue? Or a problem for beginners or sth?

I want this for newcomers and myself. For newcomers it would be super convenient to give them a comment like cargo uitest -- -- --bless --bless-comments that adds them automatically. Lowering the entry requirements and saving on these explanations would be nice.

I would personally also prefer to have the comments automatically added and then review them when doing the git add. If others have a different preference, they can just ignore this option

[Jarcho]

@xFrednet I can get to it this week since I'll be mostly off work for a while.

---

Custom flags won't work with how they're currently exposed. There's currently no access to which messages don't have a corresponding annotation.

A status emitter could work, but the implementation would be awkward. Would basically look like:

• Error annotations must be set to required
• An Arc<Mutex<_>> wrapper around the real emitter implementation
• for_revision adds the annotations and done adds which messages don't have an annotation
• The drop impl would then actually write the annotations back

[xFrednet]

Okay, then I'll leave the implementation to you. I'm happy to help if you need any assistance :D

[Jarcho]

Outline of what using an annotator looks like:

struct AnnotationEmitter;
impl StatusEmitter for AnnotationEmitter {
    fn register_test(&self, path: PathBuf) -> Box<dyn TestStatus> {
        Box::new(Annotator {
            inner: Arc::new(Mutex::new(AnnotatorImpl {
                path,
                revisions: HashMap::new(),
                failed: false,
            })),
            revision: String::new(),
        })
    }

    fn finalize(
        &self,
        _failed: usize,
        _succeeded: usize,
        _ignored: usize,
        _filtered: usize,
    ) -> Box<dyn Summary> {
        Box::new(())
    }
}

struct AnnotatorImpl {
    path: PathBuf,
    revisions: HashMap<String, Vec<Message>>,
    failed: bool,
}
impl Drop for AnnotatorImpl {
    fn drop(&mut self) {
        // same as write_back_annotations
    }
}

struct Annotator {
    inner: Arc<Mutex<AnnotatorImpl>>,
    revision: String,
}

impl TestStatus for Annotator {
    fn for_revision(&self, revision: &str) -> Box<dyn TestStatus> {
        self.inner
            .lock()
            .unwrap()
            .revisions
            .insert(revision.to_owned(), Vec::new());
        Box::new(Annotator {
            inner: self.inner.clone(),
            revision: revision.to_owned(),
        })
    }

    fn for_path(&self, _path: &Path) -> Box<dyn TestStatus> {
        // do something here.
        panic!()
    }

    fn failed_test<'a>(
        &'a self,
        _cmd: &'a Command,
        _stderr: &'a [u8],
        _stdout: &'a [u8],
    ) -> Box<dyn Debug + 'a> {
        struct S;
        impl Debug for S {
            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
                Ok(())
            }
        }
        Box::new(S)
    }

    fn update_status(&self, _msg: String) {}

    fn done(&self, result: &TestResult) {
        if let Err(e) = result {
            match &*e.errors {
                [] => {}
                [Error::ErrorsWithoutPattern { msgs, .. }] => self
                    .inner
                    .lock()
                    .unwrap()
                    .revisions
                    .get_mut(&self.revision)
                    .unwrap()
                    .extend(msgs.iter().cloned()),
                _ => self.inner.lock().unwrap().failed = true,
            }
        }
    }

    fn path(&self) -> &Path {
        // actually deal with this
        panic!()
    }

    fn revision(&self) -> &str {
        &self.revision
    }
}

Still a few extra things to add before it works and it's quite awkward, but it would work.

[oli-obk]

Yea I like that approach. Can you explain why it's awkward for you? Just because I think this is the better approach doesn't make it the better approach

[oli-obk]

maybe do a filter pass on the full data first, and then just print out everything at once instead of mixing the printing and the filtering

[Jarcho]

I don't see how this comment matches the lines you're commenting on.

[oli-obk]

This filtering out of duplicated messages could be a separate pass before the printing. Here it is in the middle of printing logic

[Jarcho]

The printing code follows immediately after this, it's not in the middle.

[Jarcho]

A few problems that it has as a status emitter:

• ui_test needs to be set to error on missing annotations.
• The file contents need to be re-read.
• It changes whether a test is a failure or not.

The first two issues can just be ignored and it would be fine-ish. The last one means printing needs to be partially reimplemented since we can't use this with the normal status emitter.

[oli-obk]

• ui_test needs to be set to error on missing annotations

Isn't that the current behaviour? Or I'm misunderstanding what you're saying

• It changes whether a test is a failure or not.

Why that? It fails, the next time it may pass. Your changes should not affect the run in which the changes are happening

[Jarcho]

Isn't that the current behaviour? Or I'm misunderstanding what you're saying

The behaviour in this PR changes that into three option: yolo, annotate and error. Using a status emitter allows four options, one of which (yolo + annotate) doesn't work.

Why that? It fails, the next time it may pass. Your changes should not affect the run in which the changes are happening

Because not doing so is a terrible interface for the same reason printing error messages when blessing stderr output is a terrible interface.

[oli-obk]

Ah I see.

We could make the annotating emitter a wrapper around any other emitter and not forward the method calls to the nested emitter if we know we're gonna annotate for this test. Even if we are only collecting the annotations to do at once at the end, we know at each error emission whether we'll be able to annotate (even if the annotations may get aggregated).

[oli-obk]

The behaviour in this PR changes that into three option: yolo, annotate and error. Using a status emitter allows four options, one of which (yolo + annotate) doesn't work.

"doesn't work" means "getting no annotations done automatically" right?

Maybe we can refactor yolo mode in general (or just get rid of it, wasn't the plan to make clippy fully annotated?)

[Jarcho]

"doesn't work" means "getting no annotations done automatically" right?

That is correct. The annotator uses the unmatched annotations error.

Maybe we can refactor yolo mode in general (or just get rid of it, wasn't the plan to make clippy fully annotated?)

IIRC last time this was discussed we decided on transitioning clippy to require annotations.

---

We could make the annotating emitter a wrapper around any other emitter and not forward the method calls to the nested emitter if we know we're gonna annotate for this test. Even if we are only collecting the annotations to do at once at the end, we know at each error emission whether we'll be able to annotate (even if the annotations may get aggregated).

The way the PR is currently implemented all revisions must pass the other tests (e.g stderr and rustfix) before the annotations are written. I'm not sure if this part is actually a useful constraint or not. Even if we do remove this constraint it still leaves the emitter not composing at all. I'm not sure if this poses a practical issue or not.

[oli-obk]

I'm not sure if this part is actually a useful constraint or not.

it is, because we want to avoid modifying the tests while other revisions on the same test may be running. I guess we could refactor emitters to handle revisions less independently.

I'm not sure if this poses a practical issue or not.

I don't think it's an issue. This is going to get set up once, and the fact that it needs to wrap other emitters will hopefully make ppl read the documentation 😆

[Jarcho]

it is, because we want to avoid modifying the tests while other revisions on the same test may be running. I guess we could refactor emitters to handle revisions less independently.

We will always have to wait until all revisions are done running to get the full list of annotations (to see if the annotation is per-revision or for all revisions). My comment was about whether we block writing annotations to the file if any revision fails. Which is to say we don't know if the current revision should be considered successful until after all revisions have run. The need to buffer test status here would make the implementation awkward.

---

Also somethings that's not currently handled is insertion around existing comments. e.g. converting

foo();
//~^ some_error

to

foo();
//~^ some_error
//~| some_other_error

Not really the biggest problem, but I don't think status emitters are given any access to the parsed comments, let alone the (never actually constructed) list of all comments independent of their revision.

[oli-obk]

can't you just use a Result<AnnotationData, ()> that gets flipped to Err if any revision fails? I'm not sure what you mean by buffering

[Jarcho]

When wrapping an existing status emitter we can't forward the failed_test or done functions to the inner TestStatus until after all revisions have been completed.