Skip to content

Improve handling of numbers in IntoDiagnosticArg #120398

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

Merged
merged 2 commits into from
Jan 27, 2024
Merged

Improve handling of numbers in IntoDiagnosticArg #120398

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
304361a
Improve handling of numbers in IntoDiagnosticArg
Urgau Jan 26, 2024
93ff4a4
Fix typo
Urgau Jan 26, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 22 additions & 25 deletions compiler/rustc_errors/src/diagnostic_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,29 @@ macro_rules! into_diagnostic_arg_using_display {
}
}

macro_rules! into_diagnostic_arg_for_number {
($( $ty:ty ),+ $(,)?) => {
$(
impl IntoDiagnosticArg for $ty {
fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> {
// HACK: `FluentNumber` the underline backing struct represent
// numbers using a f64 which can't represent all the i128 numbers
// So in order to be able to use fluent selectors and still
// have all the numbers representable we only convert numbers
// below a certain threshold.
if let Ok(n) = TryInto::<i128>::try_into(self) && n >= -100 && n <= 100 {
DiagnosticArgValue::Number(n)
} else {
self.to_string().into_diagnostic_arg()
}
Comment on lines +71 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.

Hm, this seems quite confusing. If someone is matching on different numbers, say 0, 1, 100, and 1000, then the latter one will be different, which is pretty inconsistent.
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 it's a necessary compromise given the usage of f64 in FluentNumber. In reality, you're probably using one of the CLDR numerical selectors, rather than the number directly.
Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I thought of using a more complicated system which would take into account f64 maximum representable base-10 number but I think it's overkill.

The reason I wanted to match on the numbers was to change the sentence in the 1 case, and it will be the case of many other peoples and I think that in many languages phrases with numbers generally only care about 0, 1, >1. So to be safe I choose 100 as a low but consistent and predictable number.
}
}
)+
}
}

into_diagnostic_arg_using_display!(
ast::ParamKindOrd,
i8,
u8,
i16,
u16,
u32,
i64,
i128,
u128,
std::io::Error,
Box<dyn std::error::Error>,
std::num::NonZeroU32,
Expand All @@ -82,17 +95,7 @@ into_diagnostic_arg_using_display!(
ExitStatus,
);

impl IntoDiagnosticArg for i32 {
fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> {
DiagnosticArgValue::Number(self.into())
}
}

impl IntoDiagnosticArg for u64 {
fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> {
DiagnosticArgValue::Number(self.into())
}
}
into_diagnostic_arg_for_number!(i8, u8, i16, u16, i32, u32, i64, u64, i128, u128, isize, usize);

impl IntoDiagnosticArg for bool {
fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> {
Expand Down Expand Up @@ -154,12 +157,6 @@ impl IntoDiagnosticArg for PathBuf {
}
}

impl IntoDiagnosticArg for usize {
fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> {
DiagnosticArgValue::Number(self as i128)
}
}

impl IntoDiagnosticArg for PanicStrategy {
fn into_diagnostic_arg(self) -> DiagnosticArgValue<'static> {
DiagnosticArgValue::Str(Cow::Owned(self.desc().to_string()))
Expand Down