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

Make some invalid codegen attr errors structured/translatable #137279

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

estebank
Copy link
Contributor

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2025

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2025

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

@compiler-errors
Copy link
Member

I thought we were in limbo w.r.t. translatable/structured diagnostics. https://rust-lang.zulipchat.com/#narrow/channel/147480-t-compiler.2Fwg-diagnostics/topic/.E2.9C.94.20translation.20effort

I kinda don't want to land more PRs to make things into translatable diagnostics for no reason if it's not clear that this effort has a primary owner in the compiler and it's possible that we may just want to rip it all out?

cc @davidtwco @jieyouxu

@jieyouxu
Copy link
Member

jieyouxu commented Feb 19, 2025

(idk what's the plan for the translation infra, I recorded what I found out about the current status over at #132181)

AFAICT, the current status is that:

  • You don't need to use the translatable diagnostics infra if it's overly restrictive/annoying (e.g. in const-eval usually)
  • ... but if you want (or if it makes the code clear or sth), then by all means use the infra.

But yeah, probably doesn't really make sense to migrate existing diagnostics over to the translatable one unless it makes the code nicer or if we have a path forward for the translatable infra, I guess.

FWIW, ripping the translatable infra out is equally churny/disruptive so...

@estebank
Copy link
Contributor Author

I wasn't aware of the policy change discussions, only vaguely remembered a different thread from davidtwco about some of the issues he saw with the current approach.

I'm ok with not landing these.

But I think there's still value in the current infrastructure even independently of the (demise of the?) translation effort. One case is #137201, where I made it so that instead of having to manually shorten types and set the message for where the full type name was written on every error, the translation machinery handles that on every appropriate type without manual intervention.

@nnethercote
Copy link
Contributor

I will say r=me because it looks like conversion is valid, code-wise, and it gets rid of a few calls to struct_span_code_err! which is a macro I don't like, and because it marginally improves the error output in one test. I will leave it up to @estebank whether or not to merge this.

@bors
Copy link
Contributor

bors commented Feb 24, 2025

☔ The latest upstream changes (presumably #137497) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants