Migrate `symbol_mangling` module to new diagnostics structs #100831

JhonnyBillM · 2022-08-21T05:25:17Z

No description provided.

rust-highfive · 2022-08-21T05:25:20Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

rustbot · 2022-08-21T05:25:20Z

rustc_error_messages was changed

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

JhonnyBillM · 2022-08-21T05:27:34Z

compiler/rustc_symbol_mangling/src/errors.rs

+#[derive(SessionDiagnostic)]
+#[error(symbol_mangling::invalid_trait_item)]
+pub struct InvalidTraitItem {
+    #[primary_span]
+    pub span: Span,
+    pub demangling_formatted: String,
+}
+
+#[derive(SessionDiagnostic)]
+#[error(symbol_mangling::alt_invalid_trait_item)]
+pub struct AltInvalidTraitItem {
+    #[primary_span]
+    pub span: Span,
+    pub alt_demangling_formatted: String,
+}


Quoting from the commit message:

Thought of doing this by having a struct and an enum with Default and Alt cases, but not sure if we wanted to have the text in code instead of having “demangling()” and “demangling-alt()” in the ftl file.

Don’t like the current way of having structs representing the same-ish and using long names to distinguish their expectations, instead of putting this in an enum and handling the different cases inside the type.

I am fine with whichever option the team prefers; also understand having them as separate structs keeps it simple.

I think this is okay for this crate, it's certainly something that we've run into before in other crates there where it is more of an issue. It's a big job, but if you wanted to extend SessionDiagnostic to support enums (in a similar way to how SessionSubdiagnostic does - which might make it possible to merge the code behind those somewhat, maybe, a little) then I'd like to see that :)

davidtwco · 2022-08-22T12:53:55Z

r? @davidtwco
@bors r+

bors · 2022-08-22T12:53:56Z

📌 Commit d62461a6d40b651733ef29ca6e13abdcf12f33a5 has been approved by davidtwco

It is now in the queue for this repository.

compiler-errors · 2022-08-26T22:50:06Z

@bors rollup

…mpiler-errors Rollup of 9 pull requests Successful merges: - rust-lang#100724 (Migrate ast lowering to session diagnostic) - rust-lang#100735 (Migrate `rustc_ty_utils` to `SessionDiagnostic`) - rust-lang#100738 (Diagnostics migr const eval) - rust-lang#100744 (Migrate rustc_mir_dataflow to diagnostic structs) - rust-lang#100776 (Migrate `rustc_lint` errors to `SessionDiagnostic`) - rust-lang#100817 (sugg: suggest the usage of boolean value when there is a typo in the keyword) - rust-lang#100836 (Migrate `rustc_attr` crate diagnostics) - rust-lang#100890 (Migrate rustc_driver to SessionDiagnostic) - rust-lang#100900 (on `region_errors.rs`) Failed merges: - rust-lang#100831 (Migrate `symbol_mangling` module to new diagnostics structs) r? `@ghost` `@rustbot` modify labels: rollup

JhonnyBillM · 2022-08-27T13:50:49Z

r? @davidtwco

…ostics infraestructure ADD - dependencies needed to port a module to new Diagnostics infra (rustc_macros, rustc_errors, errors file, and fluent file)

Thought of doing this by having a struct and an enum with Default and Alt cases, but not sure if we wanted to have the text in code instead of having “demangling()” and “demangling-alt()” in the ftl file. Don’t like the current way of having structs representing the same-ish and using long names to distinguish their expectations, instead of putting this in an enum and handling the different cases inside the type. I am fine with whichever option the team prefers; also understand having them as separate structs keeps it simple.

JhonnyBillM · 2022-08-30T18:59:08Z

@rustbot label -S-waiting-on-author +S-waiting-on-review

davidtwco · 2022-08-31T08:12:17Z

@bors r+

bors · 2022-08-31T08:12:19Z

📌 Commit 3ee6946 has been approved by davidtwco

It is now in the queue for this repository.

…g-to-diagnostics-structs, r=davidtwco Migrate `symbol_mangling` module to new diagnostics structs

Rollup of 7 pull requests Successful merges: - rust-lang#90946 (Ignore `reference`s in "Type::inner_def_id") - rust-lang#100730 (Migrate rustc_monomorphize to use SessionDiagnostic) - rust-lang#100753 (translations(rustc_session): migrates `rustc_session` to use `SessionDiagnostic` - Pt. 1) - rust-lang#100831 (Migrate `symbol_mangling` module to new diagnostics structs) - rust-lang#101204 (rustdoc: Resugar async fn return type in `clean`, not `html`) - rust-lang#101216 (Use in-page links for sanitizer docs.) - rust-lang#101237 (fix into_iter on ZST) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

eddyb · 2022-08-31T18:47:50Z

compiler/rustc_symbol_mangling/src/test.rs

-            tcx.sess.span_err(attr.span, &format!("symbol-name({})", mangled));
+            tcx.sess.emit_err(InvalidSymbolName {
+                span: attr.span,
+                mangled_formatted: format!("{mangled}"),
+            });
            if let Ok(demangling) = rustc_demangle::try_demangle(mangled.name) {
-                tcx.sess.span_err(attr.span, &format!("demangling({})", demangling));
-                tcx.sess.span_err(attr.span, &format!("demangling-alt({:#})", demangling));
+                tcx.sess.emit_err(InvalidTraitItem {
+                    span: attr.span,
+                    demangling_formatted: format!("{demangling}"),
+                });
+                tcx.sess.emit_err(AltInvalidTraitItem {
+                    span: attr.span,
+                    alt_demangling_formatted: format!("{:#}", demangling),
+                });
            }
        }

        for attr in tcx.get_attrs(def_id.to_def_id(), DEF_PATH) {
-            let path = with_no_trimmed_paths!(tcx.def_path_str(def_id.to_def_id()));
-            tcx.sess.span_err(attr.span, &format!("def-path({})", path));
+            tcx.sess.emit_err(InvalidDefPath {
+                span: attr.span,
+                def_path: with_no_trimmed_paths!(tcx.def_path_str(def_id.to_def_id())),
+            });


I was confused why we had symbol-mangling diagnostics... and we don't.

This ~~probably~~ needs to be ~~reverted, or at the very least~~ fixed to not have very misleading names.

There are no user-facing errors here, these are the #[rustc_symbol_name] and #[rustc_def_path] internal testing attributes (similar to #[rustc_layout] which should also not be considered user-facing).

Even these using emit_err is a bit of a stretch, but we do need to point at the source code so we can't just dump something on stderr (there are more reasons for that as well).

At the very least, I would like to see the name of the the attribute generating this (instead of "invalid").

So e.g. this should be rustc_def_path instead of invalid_def_path.

Ideally it would be tagged as "internal" so translators don't waste their time or get confused - there's basically no natural-language text here, the def-path(...) can just be changed to mentioning #[rustc_def_path] instead, since it only exists to label the test output - or could even just remove the text around the tcx.def_path_str(...) result entirely.

I would compare it to RUSTC_LOG and #[tracing::instrument]: you shouldn't translate it, because it's just reflecting things in the compiler and it's not user-facing.
_{(debug! is a bit different and it would be interesting to see what we can do in terms of trying to remove natural-language messaging from them, and maybe long-term translate the bits that we can't - but that's getting close to "translating the comments in the compiler", or even identifiers, and at that point a better solution might be a way to annotate the source-code as a whole externally, or at least it's a harder problem I don't know of good solutions for)}

eddyb · 2022-08-31T19:10:18Z

compiler/rustc_error_messages/locales/en-US/symbol_mangling.ftl

+symbol_mangling_invalid_symbol_name = symbol-name({$mangled_formatted})
+
+symbol_mangling_invalid_trait_item = demangling({$demangling_formatted})
+
+symbol_mangling_alt_invalid_trait_item = demangling-alt({$alt_demangling_formatted})
+
+symbol_mangling_invalid_def_path = def-path({$def_path})


These can actually be all merged together, since they're {$kind}({$contents}).
Might even make more sense to use {$kind} = {$contents} or some other style, but that's a bit orthogonal.

I think this might be a good way to make this go through the translation infrastructure - so that we can eventually remove the non-translation codepaths - while making translation of the message a no-op.

@JhonnyBillM do you think you could submit a new PR that makes this change? It's just that these "diagnostics" really only exist to be checked our the test suite to test the mangling scheme, and so names like they have (which I should have caught in review), might be misleading.

Thank you for the review! Helped me understand better this crate and file.
Addressed your points in #101782

eddyb · 2022-08-31T19:16:33Z

compiler/rustc_symbol_mangling/src/test.rs

-                tcx.sess.span_err(attr.span, &format!("demangling({})", demangling));
-                tcx.sess.span_err(attr.span, &format!("demangling-alt({:#})", demangling));
+                tcx.sess.emit_err(InvalidTraitItem {
+                    span: attr.span,
+                    demangling_formatted: format!("{demangling}"),
+                });
+                tcx.sess.emit_err(AltInvalidTraitItem {
+                    span: attr.span,
+                    alt_demangling_formatted: format!("{:#}", demangling),
+                });


Not sure where "trait item" came from, this is just the "demangling output" part of #[rustc_symbol_name] testing and can be be applied to pretty much anything for which tcx.generics_of returns true, and which report_symbol_names happens to visit.

This could include e.g. type aliases and consts (even associated ones), even if they don't have anything like a "symbol name" normally. May even work for mod, extern crate, etc.
(we should probably try testing those things, just to have a good compendium of what the compiler is supposed to do when encountering them in other contexts, heh)

…ng-diags-migration, r=davidtwco Update `symbol_mangling` diagnostics migration Addresses comments raised in rust-lang#100831. r? `@eddyb` `@davidtwco`

rust-highfive assigned estebank Aug 21, 2022

rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 21, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2022

JhonnyBillM commented Aug 21, 2022

View reviewed changes

JhonnyBillM mentioned this pull request Aug 21, 2022

Diagnostic Translation #100717

Open

84 tasks

JhonnyBillM force-pushed the migrate-symbol-mangling-to-diagnostics-structs branch from 6d7cb00 to d62461a Compare August 21, 2022 15:10

davidtwco approved these changes Aug 22, 2022

View reviewed changes

rust-highfive assigned davidtwco and unassigned estebank Aug 22, 2022

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 Aug 22, 2022

davidtwco mentioned this pull request Aug 22, 2022

Migrate rustc_attr crate diagnostics #100836

Merged

compiler-errors mentioned this pull request Aug 26, 2022

Rollup of 9 pull requests #101064

Merged

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 Aug 27, 2022

JhonnyBillM force-pushed the migrate-symbol-mangling-to-diagnostics-structs branch from d62461a to dbe9035 Compare August 27, 2022 04:56

JhonnyBillM added 5 commits August 30, 2022 14:27

ADD - InvalidSymbolName to migrate symbol-name({}) error to new diagn…

86f8c4e

…ostics infraestructure ADD - dependencies needed to port a module to new Diagnostics infra (rustc_macros, rustc_errors, errors file, and fluent file)

UPDATE - accept String instead of unused 'str

bd83bbc

ADD - migrate InvalidDefPath to new diagnostics infra

8f5fada

ADD - diagnostics lints to symbol_mangling module

ef2f6ab

UPDATE - to support diag introduced in PR rust-lang#100765

3ee6946

JhonnyBillM force-pushed the migrate-symbol-mangling-to-diagnostics-structs branch from dbe9035 to 3ee6946 Compare August 30, 2022 18:55

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 Aug 30, 2022

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 Aug 31, 2022

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 31, 2022

Rollup merge of rust-lang#100831 - JhonnyBillM:migrate-symbol-manglin…

7e11c11

…g-to-diagnostics-structs, r=davidtwco Migrate `symbol_mangling` module to new diagnostics structs

GuillaumeGomez mentioned this pull request Aug 31, 2022

Rollup of 6 pull requests #101233

Closed

RalfJung mentioned this pull request Aug 31, 2022

Rollup of 7 pull requests #101238

Merged

bors merged commit 59d2c19 into rust-lang:master Aug 31, 2022

rustbot added this to the 1.65.0 milestone Aug 31, 2022

eddyb reviewed Aug 31, 2022

View reviewed changes

JhonnyBillM mentioned this pull request Sep 13, 2022

Update symbol_mangling diagnostics migration #101782

Merged

JhonnyBillM deleted the migrate-symbol-mangling-to-diagnostics-structs branch October 3, 2022 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `symbol_mangling` module to new diagnostics structs #100831

Migrate `symbol_mangling` module to new diagnostics structs #100831

JhonnyBillM commented Aug 21, 2022

rust-highfive commented Aug 21, 2022

rustbot commented Aug 21, 2022

JhonnyBillM Aug 21, 2022

davidtwco Aug 22, 2022

davidtwco commented Aug 22, 2022

bors commented Aug 22, 2022

compiler-errors commented Aug 26, 2022

This comment was marked as resolved.

JhonnyBillM commented Aug 27, 2022

JhonnyBillM commented Aug 30, 2022

davidtwco commented Aug 31, 2022

bors commented Aug 31, 2022

eddyb Aug 31, 2022 •

edited

Loading

eddyb Aug 31, 2022

eddyb Aug 31, 2022

davidtwco Sep 1, 2022

JhonnyBillM Sep 13, 2022

eddyb Aug 31, 2022

Migrate symbol_mangling module to new diagnostics structs #100831

Migrate symbol_mangling module to new diagnostics structs #100831

Conversation

JhonnyBillM commented Aug 21, 2022

rust-highfive commented Aug 21, 2022

rustbot commented Aug 21, 2022

JhonnyBillM Aug 21, 2022

Choose a reason for hiding this comment

davidtwco Aug 22, 2022

Choose a reason for hiding this comment

davidtwco commented Aug 22, 2022

bors commented Aug 22, 2022

compiler-errors commented Aug 26, 2022

This comment was marked as resolved.

JhonnyBillM commented Aug 27, 2022

JhonnyBillM commented Aug 30, 2022

davidtwco commented Aug 31, 2022

bors commented Aug 31, 2022

eddyb Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

eddyb Aug 31, 2022

Choose a reason for hiding this comment

eddyb Aug 31, 2022

Choose a reason for hiding this comment

davidtwco Sep 1, 2022

Choose a reason for hiding this comment

JhonnyBillM Sep 13, 2022

Choose a reason for hiding this comment

eddyb Aug 31, 2022

Choose a reason for hiding this comment

Migrate `symbol_mangling` module to new diagnostics structs #100831

Migrate `symbol_mangling` module to new diagnostics structs #100831

eddyb Aug 31, 2022 •

edited

Loading