#[inline] attributes should be validated before translation.

[michaelwoerister]

At the moment #[inline] attributes are validated by the compiler during translation because that's the first time they are actually used. However, as the compiler gets more lazy, an inline function might be exported without the #[inline] attribute ever being validate. This can lead to the case of getting an error message only when the function is used in a downstream crate.

To solve this issue, modify the compiler so that compile-fail/E0534.rs passes if the inline function is not called from anywhere.

[nikomatsakis]

Here are some mentoring instructions:

Currently, the inline attributes are parsed by this call here, in librustc_trans. Conveniently, the real logic is done by a function find_inline_attr that lives in libsyntax -- one of the base crates of rustc (see the README for details). This is convenient because it means that these attributes are available much earlier.

(Actually, a good first task is to make a failing test. We should be able to create one based on the changes from #45575.)

Probably the best place to handle this would be in the ast_validation pass, which runs earlier in the compiler. It just walks the tree checking for various sanity checks.

I think what I would do is the following. These changes also have the drive-by effect of a fixing a few other shortcomings.

• Refactor the find_inline_attr helper so that it doesn't directly report an error, but rather returns a Result with the span and other information in the event of an error.
• Invoke find_inline_attr from visit_item, visit_trait_item and visit_impl_item, with the list of attributes from each item-like thing.
  • If it returns Err, report an error.
• Change the inline attribute from being listed as Whitelisted to Normal in feature_gate.rs
  • This will cause the unused_attribute lint to fire if #[inline] is used in an inappropriate place, such as on a struct. You should add a compile-fail or ui test showing that we now get a lint in those scenarios (add #![deny(unused_attrs)] to the test to force it to fail).
• In trans, meanwhile, we can continue to invoke find_inline_attr in the same way, but just ignore any Err returns.
  • For bonus points, trans would somehow assert that the attribute found by find_inline_attr had already been marked as used -- the mark_used call is our way of tracking which attributes are used and which are not. The idea behind this assertion would be that we would be ensuring that the "ast validation" pass did indeed visit the attribute and validate its form (since that pass would mark it as used).

OK, that's a bit rough, but hopefully good to get started, please feel free to ask questions -- though asking on gitter will likely yield faster responses.

[jgouly]

I'd like to have a go at fixing this.

[zackmdavis]

(This is also #40792, and will likely surface some error-index doctest failures.)

[nikomatsakis]

@zackmdavis thanks, I closed that bug in favor of this one since this one seems to have more

[jgouly]

Trying to create a test case for the various issues here:

#[inline(boo)]
pub fn something(){}

struct MyStruct<T>(T);
impl<T> MyStruct<T> {
    #[inline(foo)]
    pub fn f(self) {}
}

#[inline]
type Foo = u32;

struct Foo { #[inline] f: u32 }

I've been extending check_inline (

rust/src/librustc/hir/check_attr.rs

Line 61 in 200c4d0

fn check_inline(&self, attr: &ast::Attribute, target: Target) {

) to perform the checks that find_inline_attr (

rust/src/libsyntax/attr.rs

Line 532 in 9f3b091

pub fn find_inline_attr(diagnostic: Option<&Handler>, attrs: &[Attribute]) -> InlineAttr {

) was doing.
This currently diagnoses the first 3 errors, but the last one gives:

warning: unused attribute
  --> y.rs:17:14
   |    
17 | struct Foo { #[inline] f: u32 }
   |              ^^^^^^^^^
   |
   = note: #[warn(unused_attributes)] on by default

@nikomatsakis is that warning ok for the last one, or do you want to actually diagnose it as 'error[E0518]: attribute should be applied to function'.

[jgouly]

I pushed some progress to: jgouly@9a8927b

One of the issues I'm seeing with asserting that the inline attribute is used after hir validation is this:

$ cat hello.rs
fn main() { println!("Hello world") }
$ ASSERT=1 ATTR=1 RUST_BACKTRACE=1 ./build/x86_64-apple-darwin/stage1/bin/rustc hello.rs
find_inline_attr attr = Attribute { id: AttrId(3517), style: Outer, path: path(inline), tokens: TokenStream { kind: Empty }, is_sugared_doc: false, span: src/libstd/sync/mpsc/mod.rs:1838:39: 1838:48 }
error: internal compiler error: unexpected panic
thread 'rustc' panicked at 'Attr should be marked as used already!', src/libsyntax/attr.rs:569:12
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::begin_panic
   6: syntax::attr::find_inline_attr
   7: syntax::attr::requests_inline
   8: rustc_trans_utils::common::requests_inline
   9: rustc_trans_utils::trans_item::TransItemExt::instantiation_mode
  10: rustc_trans_utils::collector::InliningMap::record_accesses

But that line doesn't have an inline attribute: https://github.com/jgouly/rust/blob/gh45738/src/libstd/sync/mpsc/mod.rs#L1838

Any ideas?

[nikomatsakis]

@jgouly

is that warning ok for the last one, or do you want to actually diagnose it as 'error[E0518]: attribute should be applied to function'.

I think that warning is OK. In particular, we can't really make this a hard-error without going through a big warning period and so forth -- it would break existing code! -- which means we would need a warning anyhow. Let's start with just the warning, and decide late if we ever want it to be a hard error. (The main problem I see is that if we ever wanted #[inline] to have some semantics there, it would affect how existing code works, but in any #[inline] happens to be the kind of attribute whose effects the user shouldn't really be able to "observe" anyway.)

[nikomatsakis]

@jgouly hmm, I wonder if we synthesize fake inline attributes for closures or something. (@eddyb on IRC says no.) Seems curious.

[nikomatsakis]

Ah, I suspect that the problem is that the attribute is actually on the send or unwrap function that is being called -- when we create a local, monomorphized copy in this crate, we presumably make an #[attr] that was never visited. You might fix it @jgouly by adding some check for whether the attribute is attached to something in the LOCAL_CRATE or not.

[jgouly]

Been very busy and unable to make progress the last few days. Hoping to get this near finished at the weekend.

[jgouly]

@nikomatsakis I have two problems now.

One is that the signature for check_attribute was changed to take an ast::Item, in my code I added visit_trait_item and visit_impl_item, so I have ast::TraitItem and ast::ImplItem, what's the best way to abstract over those items? jgouly@a3fcbad

The other problem is the LOCAL_CRATE bit, that seems to come from hir::def_id, which I don't have yet as this is at the AST stage. How can I check at this point?

[jgouly]

After talking with @arielb1 and @nikomatsakis on IRC, I'm going to remove the assert, it seems more hassle than it's worth.

For the check_attribute I'm just going to pass the item's span, than the item itself.

[jgouly]

If it wasn't obvious, someone else should take this over! My code is here as a starting point: https://github.com/jgouly/rust/tree/gh45738

[michaelwoerister]

@wesleywiser Is it possible that you fixed this a while ago? I remember you did some work around querifying attributes.

[jackh726]

Closing as it looks like this has been fixed.