Remove `Session.used_attrs` and move logic to `CheckAttrVisitor`#87739

Aaron1011 · 2021-08-03T20:56:32Z

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 · 2021-08-03T20:56:36Z

r? @wesleywiser

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

bors · 2021-08-04T04:56:50Z

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

compiler/rustc_expand/src/expand.rs

bors · 2021-08-12T07:54:35Z

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

compiler/rustc_interface/src/util.rs

compiler/rustc_mir/src/dataflow/mod.rs

src/test/ui/conditional-compilation/cfg-attr-empty-is-unused.rs

wesleywiser · 2021-08-20T16:39:44Z

src/test/ui/feature-gates/issue-43106-gating-of-builtin-attrs.rs

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

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

compiler/rustc_passes/src/check_attr.rs

src/test/ui/const-generics/defaults/default-annotation.rs

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 · 2021-08-21T19:12:39Z

@wesleywiser I've addressed your comments

wesleywiser

This looks great to me!

wesleywiser · 2021-08-23T15:28:44Z

@bors r+

bors · 2021-08-23T15:28:45Z

📌 Commit 62aea8c has been approved by wesleywiser

bors · 2021-08-24T03:58:25Z

⌛ Testing commit 62aea8c with merge f66e825...

bors · 2021-08-24T06:39:07Z

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

eddyb · 2021-08-27T13:02:48Z

compiler/rustc_typeck/src/collect.rs

    }
-    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) {


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?

Posted #92702 to clean this up.

eddyb · 2021-08-27T13:06:58Z

compiler/rustc_session/src/session.rs

    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())
    }


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

eddyb · 2021-08-27T13:14:59Z

compiler/rustc_passes/src/lang_items.rs

        let attrs = self.tcx.hir().attrs(hir_id);
-        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) {


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

eddyb · 2021-08-27T13:22:09Z

compiler/rustc_passes/src/check_attr.rs

+    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();
+                });
+            }
+            _ => {}
+        }
+    }


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

eddyb · 2021-08-27T13:23:44Z

compiler/rustc_passes/src/check_attr.rs

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


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

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.

Noted in rust-lang#87739 (review), lang_items::extract no longer needs to take a closure.

…etrochenkov Clean up lang_items::extract Noted in rust-lang#87739 (review), lang_items::extract no longer needs to take a closure.

rust-highfive assigned wesleywiser Aug 3, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2021

petrochenkov self-assigned this Aug 3, 2021

petrochenkov reviewed Aug 5, 2021

View reviewed changes

compiler/rustc_expand/src/expand.rs Outdated Show resolved Hide resolved

petrochenkov removed their assignment Aug 5, 2021

Aaron1011 force-pushed the remove-used-attrs branch from 0d06da4 to fe702a4 Compare August 5, 2021 22:59

wesleywiser reviewed Aug 20, 2021

View reviewed changes

Aaron1011 added 3 commits August 21, 2021 13:27

Remove NonMacroAttr.mark_used

17aef21

Address review comments

62aea8c

Aaron1011 force-pushed the remove-used-attrs branch from fe702a4 to 62aea8c Compare August 21, 2021 19:12

wesleywiser approved these changes Aug 23, 2021

View reviewed changes

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 added the merged-by-bors This PR was explicitly merged by bors. label Aug 24, 2021

bors merged commit f66e825 into rust-lang:master Aug 24, 2021

rustbot added this to the 1.56.0 milestone Aug 24, 2021

This was referenced Aug 24, 2021

Make AST->HIR lowering incremental #88186

Closed

Refactor named_asm_labels to a HIR lint #88196

Merged

eddyb reviewed Aug 27, 2021

View reviewed changes

ehuss mentioned this pull request Sep 6, 2021

Check for duplicate attributes. #88681

Merged

ehuss mentioned this pull request Nov 12, 2021

Add passes to mark stability and must-use attributes as used #14407

Closed

ehuss added a commit to ehuss/rust that referenced this pull request Jan 9, 2022

Clean up lang_items::extract

2b75720

Noted in rust-lang#87739 (review), lang_items::extract no longer needs to take a closure.

ehuss mentioned this pull request Jan 9, 2022

Clean up lang_items::extract #92702

Merged

Uh oh!

Conversation

Aaron1011 commented Aug 3, 2021

Uh oh!

rust-highfive commented Aug 3, 2021

Uh oh!

bors commented Aug 4, 2021

Uh oh!

Uh oh!

bors commented Aug 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Aaron1011 commented Aug 21, 2021

Uh oh!

wesleywiser left a comment

Choose a reason for hiding this comment

Uh oh!

wesleywiser commented Aug 23, 2021

Uh oh!

bors commented Aug 23, 2021

Uh oh!

bors commented Aug 24, 2021

Uh oh!

bors commented Aug 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants