More robust comment parsing #2332

oli-obk · 2022-07-05T09:17:31Z

I haven't ported the entire test suite yet. Once we've done that, I will remove the old parsing system (or in fact, turn them into errors so that accidental usage of old-style comments will be detected)

bors · 2022-07-05T22:41:38Z

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

miri

ui_test/src/comments.rs

RalfJung · 2022-07-06T00:18:47Z

ui_test/src/comments.rs

+
+    /// Parse comments in `content`.
+    /// `path` is only used to emit diagnostics if parsing fails.
+    pub(crate) fn parse(path: &Path, content: &str) -> Self {


I don't think we want to keep maintaining all this code.

(or in fact, turn them into errors so that accidental usage of old-style comments will be detected)

If you want to do that, please just use a single regex like // *(compiler?-flag|only-|ignore-) and warn/error when that matches. We don't need the full parser for this.

yes, but I want to keep it around until we ported all the tests and only remove it then

What's the issue with just porting them all now?

I don't like having so many dead code paths here.

That was assuming it's just a fairly simple global search-and-replace to add all those @.

But if you prefer I can also live with doing this in steps.

Mostly because it's very conflict prone and I'd have to rebase too often ^^

ui_test/src/comments.rs

RalfJung · 2022-07-06T00:24:53Z

ui_test/src/comments.rs

-        this
+
+        if let Some(s) = command.strip_prefix("only-") {
+            self.only.push(Condition::parse(s));


Note that Condition::parse can panic... now that you have error reporting here it'd be nice to also use that for incorrect conditions.

For proper reliable parsing, I think ignore-windows should become ignore-target-windows, otherwise we have the same problem again of having some special commands (ignore-debug, ignore-32bit) and then silently falling back to another way of interpreting the command.

agreed. For minimal churn I want to change as little as possible until we rip out the old logic and then improve the parser step by step.

oli-obk · 2022-07-06T09:46:58Z

The new attribute parsing errors now look like

tests/fail/memleak.rs ... ok
Error: 
   0: tests/fail/memleak_rc.rs:3: failed to parse annotation
   1: unknown command normalasdfize-stderr-test

Location:
   ui_test/src/comments.rs:248

(plus some terminal coloring for readability)

RalfJung · 2022-07-06T18:34:12Z

The new attribute parsing errors now look like
tests/fail/memleak.rs ... ok
Error: 
   0: tests/fail/memleak_rc.rs:3: failed to parse annotation
   1: unknown command normalasdfize-stderr-test

Location:
   ui_test/src/comments.rs:248
(plus some terminal coloring for readability)

I am confused by that error. There are two locations there, which one is it at? And why does error 0 have a filename and line but error 1 not?

ui_test/src/comments.rs

ui_test/src/tests.rs

oli-obk · 2022-07-07T08:57:26Z

I am confused by that error. There are two locations there, which one is it at? And why does error 0 have a filename and line but error 1 not?

the "Location" is the place in ui testing where the error was raised

the list is a sort of backtrace of information. This helps avoid passing down all the information, as each bubbling up can attach information in the failure case instead of pushing information in

RalfJung · 2022-07-07T11:34:09Z

the "Location" is the place in ui testing where the error was raised

Okay. For user-visible errors I don't think we want to show any of our internal source code locations by default. IMO basically we should either panic ("we don't care, just report the error somehow") or do it properly... going half-way doesn't seem worth the effort?
Also that doesn't explain why only one of the errors points at a test file+line.

ui_test/src/comments.rs

bors · 2022-07-08T00:38:50Z

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

oli-obk · 2022-07-08T15:25:11Z

Okay. For user-visible errors I don't think we want to show any of our internal source code locations by default. IMO basically we should either panic ("we don't care, just report the error somehow") or do it properly... going half-way doesn't seem worth the effort?

done, we show a message about RUST_BACKTRACE, but nothing else by default

Also that doesn't explain why only one of the errors points at a test file+line.

it's not multiple errors. this is the "spantrace" for a single error. So whenever we call a function, we can attach some information to the failure-path on that function call. To showcase this, I only added information to the parse_checked_line call (adding the file + line information). if we replace more ? with

.map_err(|err| err.wrap_err(format!("your info goes here")))?

then we get more entries in that list, telling us exactly how we got to the error.

… comments in the same files

…ndling

... that grabs things from the front instead of splitting at spaces and colons and hoping for the best

oli-obk · 2022-07-08T16:09:02Z

@bors r+

bors · 2022-07-08T16:09:03Z

📌 Commit 6e10661 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-07-08T16:09:09Z

⌛ Testing commit 6e10661 with merge 7a6a812...

bors · 2022-07-08T16:33:28Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 7a6a812 to master...