rustc_parse: diagnostics migration, v4 #105670

Xiretza · 2022-12-13T21:05:28Z

This is all the rustc_parse migrations I have in store right now; unfortunately life is pretty busy right now, so I won't be able to do much more in the near future.

cc #100717

r? @davidtwco

rustbot · 2022-12-13T21:05:36Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

mejrs · 2022-12-27T14:29:15Z

src/test/ui/pub/pub-ident-fn-3.rs

-// #60115
-
-mod foo {
-    pub bar();
-    //~^ ERROR missing `fn` or `struct` for function or struct definition
-}
-
-fn main() {}


Why were these tests deleted?

They weren't deleted, just changed and renamed. I've split the changing and renaming into different commits to ease reviewing.

compiler/rustc_errors/src/diagnostic.rs

mejrs · 2022-12-27T14:56:31Z

compiler/rustc_macros/src/diagnostics/diagnostic_builder.rs

+                if let FieldInnerTy::Vec(_) = info.ty {
+                    throw_invalid_attr!(attr, &meta, |diag| diag
+                        .help("#[suggestion_*(...)] applied to `Vec` field is ambiguous"));
+                }
+


What do these changes to the macros do? Also, it might be nice to have that message be a bit more helpful, like: "it's not supported at all" or "try x instead".

I've improved the message. The problem is that #[suggestion(code = "foo")] spans: Vec<Span> could either mean:

for span in spans { span_suggestion(span, "foo"); }

or:

multipart_suggestion(spans.into_iter().map(|span| (span, "foo")));

In order to avoid ambiguity, a Subdiagnostic must be used (either a Vec of #[suggestion] subdiagnostics, or a single #[multipart_suggestion] subdiagnostic).

Ah, so the change is not "forbid x" but "give a better error if x happens"? Thanks for this change, these sort of papercuts have been annoying me in the past 👍

Please add this case to one of the files in https://github.com/rust-lang/rust/tree/master/src/test/ui-fulldeps/session-diagnostic

It is "forbid x", but previously "x" was the first behaviour, which, while technically a correct interpretation, is probably not what most people would want. This behaviour wasn't actually used anywhere, it's just a preventative measure.

Thanks for clarifying, that seems like a good idea.

compiler/rustc_parse/src/parser/item.rs

compiler/rustc_errors/src/diagnostic.rs

mejrs

(now to wait for someone with r+ permissions to drive by 😅 )

davidtwco · 2023-01-07T14:59:36Z

Apologies for the delay in reviewing this. Thanks for your work and thanks for @mejrs for the initial review.

@bors r+

bors · 2023-01-07T14:59:37Z

📌 Commit ddfdcbe5e279ee76e618c6f29f50953abb537527 has been approved by davidtwco

It is now in the queue for this repository.

Xiretza · 2023-01-08T11:06:37Z

@rustbot ready

The check previously matched this, and suggested adding a missing `struct`: pub Foo(...): It was probably intended to match this instead (semicolon instead of colon): pub Foo(...);

This is required in order to support translatable diagnostics.

#[derive(Subdiagnostic)] does not allow multiple subdiagnostics on one variant, as in NonItemInItemListSub::Other.

Xiretza · 2023-02-01T21:15:08Z

@rustbot ready

estebank · 2023-02-01T22:39:43Z

@bors r=davidtwco

bors · 2023-02-01T22:39:45Z

📌 Commit 0d0d369 has been approved by davidtwco

It is now in the queue for this repository.

bors · 2023-02-01T22:39:46Z

🌲 The tree is currently closed for pull requests below priority 50. This pull request will be tested once the tree is reopened.

bors · 2023-02-02T02:23:34Z

⌛ Testing commit 0d0d369 with merge 131f0c6...

bors · 2023-02-02T05:25:46Z

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 131f0c6 to master...

rust-timer · 2023-02-02T06:45:20Z

Finished benchmarking commit (131f0c6): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[1.5%, 3.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.1%	[-6.2%, -2.0%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

rustbot assigned davidtwco Dec 13, 2022

This comment was marked as resolved.

Sign in to view

Xiretza force-pushed the rustc_parse-diagnostics-4 branch from 0d23e8e to 7658faa Compare December 14, 2022 18:21

mejrs reviewed Dec 27, 2022

View reviewed changes

Xiretza force-pushed the rustc_parse-diagnostics-4 branch 4 times, most recently from 671c276 to 7843eb0 Compare December 27, 2022 22:32

This comment was marked as resolved.

Sign in to view

mejrs reviewed Dec 28, 2022

View reviewed changes

compiler/rustc_errors/src/diagnostic.rs Outdated Show resolved Hide resolved

Xiretza force-pushed the rustc_parse-diagnostics-4 branch from 7843eb0 to 2a46df2 Compare December 28, 2022 21:33

mejrs approved these changes Dec 29, 2022

View reviewed changes

davidtwco approved these changes Jan 7, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 7, 2023

matthiaskrgr mentioned this pull request Jan 7, 2023

Rollup of 8 pull requests #106565

Closed

This comment was marked as resolved.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 8, 2023

Xiretza force-pushed the rustc_parse-diagnostics-4 branch from ddfdcbe to 02141b6 Compare January 8, 2023 11:05

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 8, 2023

Xiretza added 9 commits February 1, 2023 21:49

Use AddToDiagnostic for "use latest edition" help

fc0ba2c

Implement IntoDiagnosticArg for ast::Visibility

a84adba

Fix condition for "missing struct" diagnostic on tuple structs

0757d5f

The check previously matched this, and suggested adding a missing `struct`: pub Foo(...): It was probably intended to match this instead (semicolon instead of colon): pub Foo(...);

tests: rename test cases to match new behaviour

ac9c66b

rustc_parse: migrate more to diagnostic structs

87ef37d

Convert rustc_parse::parser::pat::Expected to enum

9564a30

This is required in order to support translatable diagnostics.

migrate parser::ty to diagnostic structs

7631b12

rustc_parse: revert conversion of "non-item in item list" diagnostic

a476683

#[derive(Subdiagnostic)] does not allow multiple subdiagnostics on one variant, as in NonItemInItemListSub::Other.

Make "use latest edition" subdiagnostic translatable

0d0d369

Xiretza force-pushed the rustc_parse-diagnostics-4 branch from d623e3f to 0d0d369 Compare February 1, 2023 20:57

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 2, 2023

bors merged commit 131f0c6 into rust-lang:master Feb 2, 2023

rustbot added this to the 1.69.0 milestone Feb 2, 2023

erikdesjardins mentioned this pull request Feb 19, 2023

CI: spurious error building LLVM on mingw: ranlib.exe: could not create temporary file whilst writing archive: no more archived files #108227

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc_parse: diagnostics migration, v4 #105670

rustc_parse: diagnostics migration, v4 #105670

Xiretza commented Dec 13, 2022

rustbot commented Dec 13, 2022

This comment was marked as resolved.

mejrs Dec 27, 2022

Xiretza Dec 27, 2022 •

edited

Loading

mejrs Dec 27, 2022

Xiretza Dec 27, 2022

mejrs Dec 27, 2022

Xiretza Dec 27, 2022

mejrs Dec 27, 2022

This comment was marked as resolved.

mejrs left a comment

davidtwco commented Jan 7, 2023

bors commented Jan 7, 2023

This comment was marked as resolved.

Xiretza commented Jan 8, 2023

Xiretza commented Feb 1, 2023

estebank commented Feb 1, 2023

bors commented Feb 1, 2023

bors commented Feb 1, 2023

bors commented Feb 2, 2023

bors commented Feb 2, 2023

rust-timer commented Feb 2, 2023

rustc_parse: diagnostics migration, v4 #105670

rustc_parse: diagnostics migration, v4 #105670

Conversation

Xiretza commented Dec 13, 2022

rustbot commented Dec 13, 2022

This comment was marked as resolved.

Choose a reason for hiding this comment

Xiretza Dec 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

mejrs left a comment

Choose a reason for hiding this comment

davidtwco commented Jan 7, 2023

bors commented Jan 7, 2023

This comment was marked as resolved.

Xiretza commented Jan 8, 2023

Xiretza commented Feb 1, 2023

estebank commented Feb 1, 2023

bors commented Feb 1, 2023

bors commented Feb 1, 2023

bors commented Feb 2, 2023

bors commented Feb 2, 2023

rust-timer commented Feb 2, 2023

Overall result: no relevant changes - no action needed

Xiretza Dec 27, 2022 •

edited

Loading