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.

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

Const validation error messages have lots of duplication #116764

Open
RalfJung opened this issue Oct 15, 2023 · 7 comments · May be fixed by #116777
Open

Const validation error messages have lots of duplication #116764

RalfJung opened this issue Oct 15, 2023 · 7 comments · May be fixed by #116777
Labels
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.

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 15, 2023

In the porting to translatable diagnostics, the error messages from const validation where set up in a way that causes tons of code and text duplication:

PtrToUninhabited { ptr_kind: PointerKind::Box, .. } => {
const_eval_validation_box_to_uninhabited
}
PtrToUninhabited { ptr_kind: PointerKind::Ref, .. } => {
const_eval_validation_ref_to_uninhabited
}
PtrToStatic { ptr_kind: PointerKind::Box } => const_eval_validation_box_to_static,
PtrToStatic { ptr_kind: PointerKind::Ref } => const_eval_validation_ref_to_static,
PtrToMut { ptr_kind: PointerKind::Box } => const_eval_validation_box_to_mut,
PtrToMut { ptr_kind: PointerKind::Ref } => const_eval_validation_ref_to_mut,

const_eval_validation_dangling_box_no_provenance = {$front_matter}: encountered a dangling box ({$pointer} has no provenance)
const_eval_validation_dangling_box_out_of_bounds = {$front_matter}: encountered a dangling box (going beyond the bounds of its allocation)
const_eval_validation_dangling_box_use_after_free = {$front_matter}: encountered a dangling box (use-after-free)
const_eval_validation_dangling_ref_no_provenance = {$front_matter}: encountered a dangling reference ({$pointer} has no provenance)
const_eval_validation_dangling_ref_out_of_bounds = {$front_matter}: encountered a dangling reference (going beyond the bounds of its allocation)
const_eval_validation_dangling_ref_use_after_free = {$front_matter}: encountered a dangling reference (use-after-free)

We shouldn't have one variant per pointer kind here, we should just tell the diagnostic about the pointer kind so that all the other text does only have to be written once. For those "dangling" messages we also should just have a single template, that only splits cases for the tail (explaining why this particular pointer is dangling). Currently we get a huge combinatorial explosion of all the things that appear in these messages, making maintenance of this code a pain.

I was about to introduce the information of whether the reference is shared or mutable, but the current system makes that way too complicated, so I'll hold off on that for now.

Cc @fee1-dead @davidtwco

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 15, 2023
@fee1-dead
Copy link
Member

To my understanding, this is intended. I agree that it doesn't look very good to have this duplication, but this was justified in Project Fluent's documentation. Fluent does have some sort of mechanism for matching strings and showing different strings depending on that, but they suggest you to only use it for grammar cases and not for cases like this. The reason for that is translators should get the full sentence with the complete context.

For this specific issue, I think we could do something like first translating what is in the parentheses (one identifier for "use-after-free", one identifier for "has no provenance" and so on) and then feeding that to the message about dangling reference/box

@RalfJung
Copy link
Member Author

RalfJung commented Oct 15, 2023

I don't think it is reasonable to ask rustc maintainers to write out the full combination of "shared reference", "mutable reference", "box" times the 10 kind of errors that we have that talk about those things. (It would be 12 kinds of errors after #116745, except I decided this is too painful and went with less precise and thus less helpful error messages instead.) There's a limit how to much we can make rustc maintenance harder for the sake of supporting translations, beyond which the translations are doing more harm than good.

So either the translation system has good support for letting translators deal with that, or it doesn't, but the rustc code side must present this as orthogonal. And for languages where these things do compose (like English, the language that rustc maintainers have to write the messages in), it must be possible to write this as O(n + m) strings rather than O(n * m). If the translation system cannot achieve that then we shouldn't use it.

@fee1-dead
Copy link
Member

Yep. I'll take a look in a day or two and try to fix this.

@fee1-dead fee1-dead self-assigned this Oct 15, 2023
@fee1-dead fee1-dead added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 15, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Oct 15, 2023

I'm sure Project Fluent's recommendations are well-intentioned, and I can see how they do lead to better translations, but remember that having good translations is but one of many goals of rustc, and has to be weighed against the other goals. fluent is supposed to help us get a translatable compiler, without getting in the way of regular compiler maintenance and development more than necessary. This combinatorial issue is, in my view, definitely crossing that bar.

Yep. I'll take a look in a day or two and try to fix this.

Thanks. :)

@fee1-dead
Copy link
Member

This combinatorial issue is, in my view, definitely crossing that bar.

I agree. This might be a limitation of Fluent, because it would be many times better if there is a system where we can add cases without having to add tens of variants to all the other variants in the message, and other languages can compose with the different variants when possible or, in the case where it is not, override the individual variants in the translation files for that specific language.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 15, 2023

Yeah, handling that well requires support from the translation system. We can probably only do the crude thing and do string concatenation / feed the result of the inner translation to the next one (like we already do for the "front matter" in validation messages).

@fee1-dead fee1-dead removed their assignment Oct 22, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Feb 16, 2024

I have since then learned that fluent supports some amount of sharing:

shared = a lot of complicated text, using variables and conditions
error1 = {shared}: error 1
error2 = {shared}: error 2

That seems like it could be quite helpful here?

See for instance:

const_eval_frame_note = {$times ->
[0] {const_eval_frame_note_inner}
*[other] [... {$times} additional calls {const_eval_frame_note_inner} ...]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants