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

Remove Session.used_attrs and move logic to CheckAttrVisitor #87739

Merged
merged 3 commits into from
Aug 24, 2021

Conversation

Aaron1011
Copy link
Member

Instead of updating global state to mark attributes as used,
we now explicitly emit a warning when an attribute is used in
an unsupported position. As a side effect, we are to emit more
detailed warning messages (instead of just a generic "unused" message).

Session.check_name is removed, since its only purpose was to mark
the attribute as used. All of the callers are modified to use
Attribute.has_name

Additionally, AttributeType::AssumedUsed is removed - an 'assumed
used' attribute is implemented by simply not performing any checks
in CheckAttrVisitor for a particular attribute.

We no longer emit unused attribute warnings for the #[rustc_dummy]
attribute - it's an internal attribute used for tests, so it doesn't
mark sense to treat it as 'unused'.

With this commit, a large source of global untracked state is removed.

@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2021
@petrochenkov petrochenkov self-assigned this Aug 3, 2021
@bors
Copy link
Contributor

bors commented Aug 4, 2021

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

@petrochenkov petrochenkov removed their assignment Aug 5, 2021
@bors
Copy link
Contributor

bors commented Aug 12, 2021

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

compiler/rustc_interface/src/util.rs Show resolved Hide resolved
compiler/rustc_mir/src/dataflow/mod.rs Show resolved Hide resolved
Comment on lines 313 to 315
// Don't warn on `automatically_derived` - a custom derive
// could reasonally annotate anything that it emits with
// this attribute
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a behavior change. Do we need a signoff of some kind to do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This attribute is pretty under-specified - it seems to suppress a handful of lints, which aren't documented anywhere. I think the best approach would be to warn when it's used on a non-impl (since impls are the only places where we currently check for it).

compiler/rustc_feature/src/builtin_attrs.rs Show resolved Hide resolved
compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved
compiler/rustc_feature/src/builtin_attrs.rs Show resolved Hide resolved
compiler/rustc_passes/src/check_attr.rs Outdated Show resolved Hide resolved
Instead of updating global state to mark attributes as used,
we now explicitly emit a warning when an attribute is used in
an unsupported position. As a side effect, we are to emit more
detailed warning messages (instead of just a generic "unused" message).

`Session.check_name` is removed, since its only purpose was to mark
the attribute as used. All of the callers are modified to use
`Attribute.has_name`

Additionally, `AttributeType::AssumedUsed` is removed - an 'assumed
used' attribute is implemented by simply not performing any checks
in `CheckAttrVisitor` for a particular attribute.

We no longer emit unused attribute warnings for the `#[rustc_dummy]`
attribute - it's an internal attribute used for tests, so it doesn't
mark sense to treat it as 'unused'.

With this commit, a large source of global untracked state is removed.
@Aaron1011
Copy link
Member Author

@wesleywiser I've addressed your comments

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

This looks great to me!

@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 23, 2021

📌 Commit 62aea8c has been approved by wesleywiser

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2021
@bors
Copy link
Contributor

bors commented Aug 24, 2021

⌛ Testing commit 62aea8c with merge f66e825...

@bors
Copy link
Contributor

bors commented Aug 24, 2021

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing f66e825 to master...

Comment on lines -3184 to 3181
let check_name = |attr, sym| tcx.sess.check_name(attr, sym);
let check_name = |attr: &Attribute, sym| attr.has_name(sym);
if let Some(name) = weak_lang_items::link_name(check_name, &attrs) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be inlined into weak_lang_items::link_name? I'm guessing it only took a closure because the Session type wasn't available or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Posted #92702 to clean this up.

Comment on lines 1067 to 1099
pub fn is_proc_macro_attr(&self, attr: &Attribute) -> bool {
[sym::proc_macro, sym::proc_macro_attribute, sym::proc_macro_derive]
.iter()
.any(|kind| self.check_name(attr, *kind))
.any(|kind| attr.has_name(*kind))
}

pub fn contains_name(&self, attrs: &[Attribute], name: Symbol) -> bool {
attrs.iter().any(|item| self.check_name(item, name))
attrs.iter().any(|item| item.has_name(name))
}

pub fn find_by_name<'a>(
&'a self,
attrs: &'a [Attribute],
name: Symbol,
) -> Option<&'a Attribute> {
attrs.iter().find(|attr| self.check_name(attr, name))
attrs.iter().find(|attr| attr.has_name(name))
}

pub fn filter_by_name<'a>(
&'a self,
attrs: &'a [Attribute],
name: Symbol,
) -> impl Iterator<Item = &'a Attribute> {
attrs.iter().filter(move |attr| self.check_name(attr, name))
attrs.iter().filter(move |attr| attr.has_name(name))
}

pub fn first_attr_value_str_by_name(
&self,
attrs: &[Attribute],
name: Symbol,
) -> Option<Symbol> {
attrs.iter().find(|at| self.check_name(at, name)).and_then(|at| at.value_str())
attrs.iter().find(|at| at.has_name(name)).and_then(|at| at.value_str())
}
Copy link
Member

Choose a reason for hiding this comment

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

Do any of these methods need to be on Session, or can they be moved back to being on the attributes themselves?

Comment on lines -60 to 62
let check_name = |attr, sym| self.tcx.sess.check_name(attr, sym);
let check_name = |attr: &Attribute, sym| attr.has_name(sym);
if let Some((value, span)) = extract(check_name, &attrs) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here as with the weak lang items, I'm guessing?

Comment on lines +1651 to +1673
fn check_stability_promotable(&self, attr: &Attribute, _span: &Span, target: Target) -> bool {
match target {
Target::Expression => {
self.tcx
.sess
.struct_span_err(attr.span, "attribute cannot be applied to an expression")
.emit();
false
}
_ => true,
}
}

fn check_deprecated(&self, hir_id: HirId, attr: &Attribute, _span: &Span, target: Target) {
match target {
Target::Closure | Target::Expression | Target::Statement | Target::Arm => {
self.tcx.struct_span_lint_hir(UNUSED_ATTRIBUTES, hir_id, attr.span, |lint| {
lint.build("attribute is ignored here").emit();
});
}
_ => {}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these checks have lists of allowed Targets, as opposed to disallowed Targets?

| sym::rustc_const_stable
| sym::unstable
| sym::stable
| sym::rustc_promotable => self.check_stability_promotable(&attr, span, target),
_ => true,
Copy link
Member

Choose a reason for hiding this comment

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

If we're handling all the builtin attributes, shouldn't we disallow unknown attributes?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could be an exhaustive match over an enum generated by rustc_feature::builtin_attrs? So that there's no way it can go out of sync.

ehuss added a commit to ehuss/rust that referenced this pull request Jan 9, 2022
Noted in rust-lang#87739 (review),
lang_items::extract no longer needs to take a closure.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 10, 2022
…etrochenkov

Clean up lang_items::extract

Noted in rust-lang#87739 (review),
lang_items::extract no longer needs to take a closure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants