Known but incorrect attributes over statements/expressions are ignored.

[oyvindln]

I tried to look for this, but I couldn't find an existing issue. Or is this expected behaviour?

Putting attributes that exist (though the arguments can be bogus) seem to be ignored when one would expect an error, or at least a warning. E.g like this:

#[repr(bogus_statement)]
let x = 0;

or

#[inline(bogus_statement)]
let x = 0;

This seems to be the case on stable, beta and nightly.

More examples here: https://play.rust-lang.org/?gist=b48bfc076d29c770d8d2c81f2f99f0bb

[abonander]

Possibly related to #41475

[abonander]

Turns out this is not related to #41475. This is because CheckAttrVisitor only visits items.

This should be a pretty straightforward fix, actually, and wouldn't be a bad introduction to some concepts used in the compiler frontend. I'm willing to mentor this bug @sfackler @Mark-Simulacrum if you want to add E-easy and E-mentor

[sfackler]

Done!

[abonander]

Mentoring Instructions

This bug is caused by the fact that CheckAttrVisitor doesn't visit statements. What does this mean?

Attributes can be applied to a lot of things and the parser will accept all of them. However, most attributes don't make sense in a certain context: #[inline] tells the compiler (which tells LLVM) that a function should be inlined to eliminate call overhead, but that doesn't make sense when applied to a struct, or an enum, or an individual statement in the block. It also doesn't make sense for a function to have a #[repr] attribute because that is meant to tell the compiler what alignment a type should have or what size an enum's discriminator should be.

CheckAttrVisitor is the bit that walks the Abstract Syntax Tree (AST) after parsing and points out all the places a given built-in attribute like #[inline] or #[repr] doesn't belong. But how do you walk over the AST? You implement the Visitor trait.

The Visitor trait has provided methods for inspecting every kind of node in the AST (items, functions, statements, expressions, individual identifiers, etc.), so implementors that are only concerned with certain node kinds only have to override those methods. The default implementation just recurses into the subnodes of every node and calls the appropriate trait method for them. The implementation of the recursing lives in the walk_* functions; I'm not entirely sure why it's not in the trait methods themselves but I'm sure there's a good reason.

The discrepancy in CheckAttrVisitor is that it only overrides visit_item, so that's all it observes. However, it needs to also override visit_stmt and visit_expr, look at the attributes on them and generate an error if any of the attributes aren't supposed to be there. You can see the error generation being done here, where it emits an error for the #[target_feature] attribute being applied to a non-function item. Notice also that it doesn't actually return from there; it keeps walking the tree. This is so we get all the errors out that we can before we stop compiling so the user can fix them all in one fell swoop.

[tejom]

Hey, I should have time to look at this. From the instructions and what I read so far there seems to be a bit of new code that needs to be written to do this? Something similar to check_attributes but for a Stmt and Expr ?

[abonander]

Yeah, all you should have to do is add visit_stmt and visit_expr on impl Visitor for CheckAttrVisitor, look at the attributes on Stmt or Expr, respectively, and emit an error if they're one of the attribute names checked in the other methods (inline, repr, target_feature, and any others).

I made a mistake in my original assessment and assumed that this code used types from libsyntax but that's not correct. They look very similar but they're actually defined in librustc/hir/mod.rs. The methods on Visitor you would be overriding are defined here and here.

For Stmt the attributes are accessible through an inherent method .attrs() and for Expr they're just a field attrs. ThinVec derefs to a slice so you can handle both cases with the same code probably.

Attribute is the same that's defined in libsyntax, I recommend just using .check_name() which marks it used (otherwise the "unused attribute" lint will fire on it later).

And then the final step would be to write a regression test under src/test/compile-fail. These are just single-file tests like you'd find in the tests/ folder of a Cargo project, except they're expected to fail compilation with a certain message. You can copy the MIT license header from another file in that directory and just update the copyright year to 2018. Compile-fail tests have comments telling the test suite what error messages to expect for what lines. They would look something like this:

fn main() {
    #[repr(bogus_statement)]
    //~^ ERROR the `#[repr]` attribute cannot be applied to statements or expressions
    let x = 0;

    // ... make sure to test expressions as well!
}

The file name for regression tests associated with a certain issue is just issue-#.rs, as you might be able to see in that directory, so this would be issue-43988.rs.

[abonander]

For bonus points, you could add a help message that suggests using cfg_feature_enabled!() or cfg!(target_feature) when you encounter #[target_feature] on statements and expressions; bonus bonus points if you copy the argument from #[target_feature = "..."] to the suggested cfg_feature_enabled!("...") / cfg!(target_feature = "...").

[tejom]

Hey, Maybe I misunderstood something but the two extra functions in that trait don't seem to be used.

These are the two extra functions I added in the impl Visitor here here

fn visit_stmt(&mut self, stmt: &'tcx hir::Stmt) {
        debug!("visting statement {:#?}", stmt );
        self.check_stmt_attributes(stmt);
        intravisit::walk_stmt(self, stmt);
    }

    fn visit_expr(&mut self, ex: &'tcx hir::Expr) {
        debug!("visting an expr {:#?}", ex);
        intravisit::walk_expr(self,ex);
    }

check_stmt_attributes isn't to interesting so Ill leave it out. I also added debug lines to the visit_items function. I do see those being printed out. I am not seeing either of the debug lines from the two new functions.

My test program is

fn main() {

    #[inline]
    let _a = 4;

    #[inline(UUUU)]
    let _b = 4;

    #[repr(nothing)]
    let _x = 0;


    #[repr(something_not_real)]
    {
    	1
    };
}

[abonander]

Ah, you have to change the return value of nested_visit_map in that impl block. The Visitor trait won't recurse into function bodies unless you tell it to and give it a way to resolve them.

fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
    // only visit the bodies of functions, not their definitions
    NestedVisitorMap::OnlyBodies(&this.hir)
}

[tejom]

Cool. I'm moving forward again heh. I guess that makes sense then. I was seeing main() as an item I think, but I guess that allows it to actually look inside main() Thanks

[tejom]

Hey so I pushed a commit on my own fork. I wasn't sure if its ready for a pull request but feedback on the code i think could be helpful just in case I did anything incredibly wrong heh.

tejom@f52572e

I wasn't sure what the best way to share a commit would be. If there's a better or more preferred way let me know.

[abonander]

I've left a couple of comments but you can probably go ahead and open a pull request, it'll start running tests on it. It also makes it easier to review. I don't have review permissions so add r? @petrochenkov to get a reviewer who knows the frontend and mention me.

[tejom]

Cool. Thanks a lot. I'll address your comments and open a pull request