Skip to content

Use AttrItem instead of Attribute in more places #142552

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

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

Conversation

GrigorenkoPV
Copy link
Contributor

@GrigorenkoPV GrigorenkoPV commented Jun 15, 2025

Changes requested in #142498 (comment) led me down a bit of a rabbit hole, from which I've emerged with this PR.

r? @jdonszelmann

I guess this is a part of #131229

@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 Jun 15, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 15, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Attribute::Parsed(AttributeKind::AllowConstFnUnstable { .. }) => {
self.check_rustc_allow_const_fn_unstable(hir_id, attr, span, target)
Attribute::Parsed(AttributeKind::AllowConstFnUnstable(_)) => {
// FIXME: this is incorrect
Copy link
Contributor Author

@GrigorenkoPV GrigorenkoPV Jun 15, 2025

Choose a reason for hiding this comment

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

This actually fixes an ICE.
Repro:

#[rustc_allow_const_fn_unstable(foo)]
const A: u8 = 1;
fn main(){}

Previously it would go down that one function where trying to get a span for a parsed attr would panic.
While I wasn't able to figure out (yet?) how to do a proper span here, I think that a span that is inaccurate (too large) is still better than an ICE.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't matter. Anyway, we don't care that blatant misuse of rustc attributes (and lang items etc) results in ices. It is nice but by no means required to have nice errors for these.

@rust-log-analyzer

This comment has been minimized.

@jdonszelmann
Copy link
Contributor

I'm not sure this is worth it tbh. The names of these apis match the old names in the compiler such that the original migration had the fewest possible changes. However, I intend to completely remove these apis

@mejrs
Copy link
Contributor

mejrs commented Jun 16, 2025

The current uglyness is just temporary hack while attributes are migrated.

@jdonszelmann and I were talking about having access to hir::Target (or something like it) during the attribute parsing, which would also get rid of calls to functions like check_applied_to_fn_or_method and hence the need for this PR.

@GrigorenkoPV
Copy link
Contributor Author

I'm not sure this is worth it tbh. The names of these apis match the old names in the compiler such that the original migration had the fewest possible changes. However, I intend to completely remove these apis

The renaming of methods is probably insignificant and brings no value on its own, but I think (temporary) changing APIs to use more precise types will greatly help with the migration. And it's probably more robust to do this and bring the checks that a proper type is used into compile time, than have a bunch of (temporary, yes, but still less robust) methods that effectively do the same but at runtime and panic on mismatches.

The current uglyness is just temporary hack while attributes are migrated.

Yes, I suppose by the end of it most type names will go back to what they were before, if that even matters that much.

I think that being more certain in the correctness of the code during the migration is far more important that minimizing the surface of (simple renaming) changes.

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