Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate symbol_mangling module to new diagnostics structs #100831

Merged
merged 6 commits into from
Aug 31, 2022
Merged

Migrate symbol_mangling module to new diagnostics structs #100831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
86f8c4e
ADD - InvalidSymbolName to migrate symbol-name({}) error to new diagn…
JhonnyBillM Aug 27, 2022
359002b
ADD - migrate InvalidTraitItem and AltInvalidTraitItem errors
JhonnyBillM Aug 21, 2022
bd83bbc
UPDATE - accept String instead of unused 'str
JhonnyBillM Aug 21, 2022
8f5fada
ADD - migrate InvalidDefPath to new diagnostics infra
JhonnyBillM Aug 21, 2022
ef2f6ab
ADD - diagnostics lints to symbol_mangling module
JhonnyBillM Aug 21, 2022
3ee6946
UPDATE - to support diag introduced in PR #100765
JhonnyBillM Aug 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4155,7 +4155,9 @@ dependencies = [
"punycode",
"rustc-demangle",
"rustc_data_structures",
"rustc_errors",
"rustc_hir",
"rustc_macros",
"rustc_middle",
"rustc_session",
"rustc_span",
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_error_messages/locales/en-US/symbol_mangling.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
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})
Comment on lines +1 to +7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

1 change: 1 addition & 0 deletions compiler/rustc_error_messages/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ fluent_messages! {
ty_utils => "../locales/en-US/ty_utils.ftl",
typeck => "../locales/en-US/typeck.ftl",
mir_dataflow => "../locales/en-US/mir_dataflow.ftl",
symbol_mangling => "../locales/en-US/symbol_mangling.ftl",
}

pub use fluent_generated::{self as fluent, DEFAULT_LOCALE_RESOURCES};
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_symbol_mangling/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ rustc_hir = { path = "../rustc_hir" }
rustc_target = { path = "../rustc_target" }
rustc_data_structures = { path = "../rustc_data_structures" }
rustc_session = { path = "../rustc_session" }
rustc_macros = { path = "../rustc_macros" }
rustc_errors = { path = "../rustc_errors" }
36 changes: 36 additions & 0 deletions compiler/rustc_symbol_mangling/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
//! Errors emitted by symbol_mangling.

use rustc_macros::SessionDiagnostic;
use rustc_span::Span;

#[derive(SessionDiagnostic)]
#[diag(symbol_mangling::invalid_symbol_name)]
pub struct InvalidSymbolName {
#[primary_span]
pub span: Span,
pub mangled_formatted: String,
}

#[derive(SessionDiagnostic)]
#[diag(symbol_mangling::invalid_trait_item)]
pub struct InvalidTraitItem {
#[primary_span]
pub span: Span,
pub demangling_formatted: String,
}

#[derive(SessionDiagnostic)]
#[diag(symbol_mangling::alt_invalid_trait_item)]
pub struct AltInvalidTraitItem {
#[primary_span]
pub span: Span,
pub alt_demangling_formatted: String,
}

#[derive(SessionDiagnostic)]
#[diag(symbol_mangling::invalid_def_path)]
pub struct InvalidDefPath {
#[primary_span]
pub span: Span,
pub def_path: String,
}
3 changes: 3 additions & 0 deletions compiler/rustc_symbol_mangling/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@
#![feature(never_type)]
#![recursion_limit = "256"]
#![allow(rustc::potential_query_instability)]
#![deny(rustc::untranslatable_diagnostic)]
#![deny(rustc::diagnostic_outside_of_impl)]

#[macro_use]
extern crate rustc_middle;
Expand All @@ -110,6 +112,7 @@ use tracing::debug;
mod legacy;
mod v0;

pub mod errors;
pub mod test;
pub mod typeid;

Expand Down
22 changes: 17 additions & 5 deletions compiler/rustc_symbol_mangling/src/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//! def-path. This is used for unit testing the code that generates
//! paths etc in all kinds of annoying scenarios.

use crate::errors::{AltInvalidTraitItem, InvalidDefPath, InvalidSymbolName, InvalidTraitItem};
use rustc_hir::def_id::LocalDefId;
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::{subst::InternalSubsts, Instance, TyCtxt};
Expand Down Expand Up @@ -59,16 +60,27 @@ impl SymbolNamesTest<'_> {
tcx.erase_regions(InternalSubsts::identity_for_item(tcx, def_id)),
);
let mangled = tcx.symbol_name(instance);
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),
});
Comment on lines -64 to +75
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
}

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())),
});
Comment on lines -62 to +83
Copy link
Member

@eddyb eddyb Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}
}
}