Visit attributes in more places. #96745

ehuss · 2022-05-05T21:01:50Z

This adds 3 loosely related changes (I can split PRs if desired):

Attribute checking on pattern struct fields.
Attribute checking on struct expression fields.
Lint level visiting on pattern struct fields, struct expression fields, and generic parameters.

There are still some lints which ignore lint levels in various positions. This is a consequence of how the lints themselves are implemented. For example, lint levels on associated consts don't work with unused_braces.

rust-highfive · 2022-05-05T21:01:53Z

r? @jackh726

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

ehuss · 2022-05-05T21:02:15Z

I don't really know if the HIR changes make sense, as I don't fully understand it. I'm likely doing things wrong.

I'm confused that HirId's seem to be generated for things that aren't nodes. Why does it do that? That seems like it should be invalid to me.
It seems awkward to add these HIR nodes. I don't know if it makes sense to make it a Node, but I don't see any other way.
Would it make sense to add dedicated visit_field methods to intravisit to visit struct expression and pattern fields, so that it is easier to override? I think that could make it cleaner, but I'm uncertain about the consequences of extending the visitor.
The pretty-printer seems to be missing stuff. What is the state of it? Should I add tests for that?

petrochenkov · 2022-05-05T21:45:42Z

This does not try to resolve #81282.

You say that it does not, but GitHub thinks that it does.

ehuss · 2022-05-05T22:11:15Z

You say that it does not, but GitHub thinks that it does.

Oops, tricky keywords. Thanks! I removed that comment.

bors · 2022-05-20T18:59:25Z

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

cjgillot

I'm confused that HirId's seem to be generated for things that aren't nodes. Why does it do that? That seems like it should be invalid to me.

I agree that HirIds should eventually map 1-to-1 to Nodes. I guess nobody ever cared enough to enforce it.

It seems awkward to add these HIR nodes. I don't know if it makes sense to make it a Node, but I don't see any other way.

From the code, I don't understand why you need them.

Would it make sense to add dedicated visit_field methods to intravisit to visit struct expression and pattern fields, so that it is easier to override? I think that could make it cleaner, but I'm uncertain about the consequences of extending the visitor.

That would actually be better, yes.

The pretty-printer seems to be missing stuff. What is the state of it? Should I add tests for that?

I've been considering dropping the HIR pretty printer entirely. It's only use is for debugging, and I'm not sure it's actually used by anyone.

cjgillot · 2022-05-22T09:00:40Z

compiler/rustc_ast_lowering/src/index.rs

@@ -193,6 +193,11 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
        let node =
            if let PatKind::Binding(..) = pat.kind { Node::Binding(pat) } else { Node::Pat(pat) };
        self.insert(pat.span, pat.hir_id, node);
+        if let PatKind::Struct(_, fields, _) = pat.kind {


This should be inside the call to with_parent.

compiler/rustc_hir/src/hir.rs

cjgillot · 2022-05-22T09:03:32Z

compiler/rustc_ast_lowering/src/index.rs

@@ -224,6 +224,11 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {

    fn visit_expr(&mut self, expr: &'hir Expr<'hir>) {
        self.insert(expr.span, expr.hir_id, Node::Expr(expr));
+        if let ExprKind::Struct(_, fields, _) = expr.kind {


This should be inside with_parent.

cjgillot · 2022-05-22T09:05:25Z

compiler/rustc_passes/src/check_attr.rs

@@ -2286,6 +2286,11 @@ impl<'tcx> Visitor<'tcx> for CheckAttrVisitor<'tcx> {
        };

        self.check_attributes(expr.hir_id, expr.span, target, None);
+        if let hir::ExprKind::Struct(_, fields, _) = expr.kind {
+            for field in fields {
+                self.check_attributes(field.hir_id, field.span, Target::PatField, None);


Do expression fields behave exactly like PatField? Should the PatField's name reflect that?

That was a bad copy/paste. It should have been Target::ExprField.

compiler/rustc_hir/src/hir.rs

cjgillot · 2022-05-22T09:07:43Z