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

Hir attributes #131808

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open
4 changes: 3 additions & 1 deletion Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3728,6 +3728,7 @@ dependencies = [
"rustc_span",
"rustc_target",
"smallvec",
"thin-vec",
"tracing",
]

Expand Down Expand Up @@ -4348,6 +4349,7 @@ version = "0.0.0"
dependencies = [
"bitflags 2.6.0",
"rustc_abi",
"rustc_attr",
"rustc_data_structures",
"rustc_hir",
"rustc_middle",
Expand Down Expand Up @@ -4401,9 +4403,9 @@ version = "0.0.0"
dependencies = [
"rustc_abi",
"rustc_ast",
"rustc_ast_pretty",
"rustc_data_structures",
"rustc_hir",
"rustc_hir_pretty",
"rustc_middle",
"rustc_session",
"rustc_span",
Expand Down
51 changes: 8 additions & 43 deletions compiler/rustc_ast/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
//! - [`UnOp`], [`BinOp`], and [`BinOpKind`]: Unary and binary operators.

use std::borrow::Cow;
use std::{cmp, fmt, mem};
use std::{cmp, fmt};

pub use GenericArgs::*;
pub use UnsafeSource::*;
Expand Down Expand Up @@ -1690,32 +1690,20 @@ pub enum AttrArgs {
/// Delimited arguments: `#[attr()/[]/{}]`.
Delimited(DelimArgs),
/// Arguments of a key-value attribute: `#[attr = "value"]`.
Eq(
Eq {
Copy link
Contributor

Choose a reason for hiding this comment

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

This change (refactoring of AttrArgs::Eq and removal of AttrArgsEq) isn't strictly necessary for introducing HIR attributes.
I suggest dropping it, this PR is already too large.
It can be done later as a separate cleanup PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see why we wouldn't do it in this PR. It would only get marginally smaller, and this PR moves responsibilities from ast::Attribute to hir::Attribute, and a part of that is removing hir-parts from ast::Attribute. Unless you absolutely insist, I'd argue including it right now isn't so bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen distracting changes across the PR just from refactoring Eq(..) into Eq { .. } alone, I'd prefer to skip the nonessential parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree with this. The whole point of this PR is to remove Eq, the rest of the PR is simply all the infrastructure around that to enable it to be removed. If we don't remove it, it literally becomes a variant that is unused. We never use it because we lower to hir attributes. I think it's important to keep it in.

/// Span of the `=` token.
jdonszelmann marked this conversation as resolved.
Show resolved Hide resolved
Span,
eq_span: Span,
/// The "value".
AttrArgsEq,
jieyouxu marked this conversation as resolved.
Show resolved Hide resolved
),
}

// The RHS of an `AttrArgs::Eq` starts out as an expression. Once macro
// expansion is completed, all cases end up either as a meta item literal,
// which is the form used after lowering to HIR, or as an error.
#[derive(Clone, Encodable, Decodable, Debug)]
pub enum AttrArgsEq {
Ast(P<Expr>),
Hir(MetaItemLit),
value: P<Expr>,
},
}

impl AttrArgs {
pub fn span(&self) -> Option<Span> {
match self {
AttrArgs::Empty => None,
AttrArgs::Delimited(args) => Some(args.dspan.entire()),
AttrArgs::Eq(eq_span, AttrArgsEq::Ast(expr)) => Some(eq_span.to(expr.span)),
AttrArgs::Eq(_, AttrArgsEq::Hir(lit)) => {
unreachable!("in literal form when getting span: {:?}", lit);
}
AttrArgs::Eq { eq_span, value } => Some(eq_span.to(value.span)),
}
}

Expand All @@ -1725,30 +1713,7 @@ impl AttrArgs {
match self {
AttrArgs::Empty => TokenStream::default(),
AttrArgs::Delimited(args) => args.tokens.clone(),
AttrArgs::Eq(_, AttrArgsEq::Ast(expr)) => TokenStream::from_ast(expr),
AttrArgs::Eq(_, AttrArgsEq::Hir(lit)) => {
unreachable!("in literal form when getting inner tokens: {:?}", lit)
}
}
}
}

impl<CTX> HashStable<CTX> for AttrArgs
where
CTX: crate::HashStableContext,
{
fn hash_stable(&self, ctx: &mut CTX, hasher: &mut StableHasher) {
mem::discriminant(self).hash_stable(ctx, hasher);
match self {
AttrArgs::Empty => {}
AttrArgs::Delimited(args) => args.hash_stable(ctx, hasher),
AttrArgs::Eq(_eq_span, AttrArgsEq::Ast(expr)) => {
unreachable!("hash_stable {:?}", expr);
}
AttrArgs::Eq(eq_span, AttrArgsEq::Hir(lit)) => {
eq_span.hash_stable(ctx, hasher);
lit.hash_stable(ctx, hasher);
}
AttrArgs::Eq { eq_span: _, value } => TokenStream::from_ast(value),
}
}
}
Expand Down Expand Up @@ -2945,7 +2910,7 @@ impl NormalAttr {
}
}

#[derive(Clone, Encodable, Decodable, Debug, HashStable_Generic)]
#[derive(Clone, Encodable, Decodable, Debug)]
pub struct AttrItem {
pub unsafety: Safety,
pub path: Path,
Expand Down
Loading
Loading