Warn on empty lines after outer attributes

[phansch]

This adds a new lint to warn on empty lines after outer attributes.

Still fairly new to Rust, so I'm not sure if the empty line detection could be shortened or if the tests cover all edge cases, but it seems to work so far 👍 .

Note that rustc already handles attributes with a missing item: libsyntax/parse/parser.rb#L3938

Closes #1165

[phansch]

Looks like by reusing the is_relevant_item method, it only works for functions currently. I will have another look once I'm back from vacation on the 18th. I plan to spend more time on rust-clippy, it has been really nice so far 👍

[phansch]

Looks like by reusing the is_relevant_item method, it only works for functions currently. I will have another look once I'm back from vacation on the 18th. I plan to spend more time on rust-clippy, it has been really nice so far 👍

[oli-obk]

error: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
  --> tests/cc_seme.rs:1:1
   |
1  | / #![feature(plugin)]
2  | | #![plugin(clippy)]
3  | |
4  | | #[allow(dead_code)]
...  |
14 | |
15 | | fn main() {
   | |_
   |
   = note: `-D empty-line-after-outer-attr` implied by `-D clippy`
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.179/index.html#empty_line_after_outer_attr

[oli-obk]

error: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
  --> tests/cc_seme.rs:1:1
   |
1  | / #![feature(plugin)]
2  | | #![plugin(clippy)]
3  | |
4  | | #[allow(dead_code)]
...  |
14 | |
15 | | fn main() {
   | |_
   |
   = note: `-D empty-line-after-outer-attr` implied by `-D clippy`
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.179/index.html#empty_line_after_outer_attr

[clarcharr]

There should also be a test case for items with a comment between the attribute and item.

[clarcharr]

There should also be a test case for items with a comment between the attribute and item.

[phansch]

The lint now works for all members of syntax::ast::Item_, I added test cases for enum, struct and mod. Should I add test cases for all the other Item_ kinds as well or would that be too much?

[phansch]

The lint now works for all members of syntax::ast::Item_, I added test cases for enum, struct and mod. Should I add test cases for all the other Item_ kinds as well or would that be too much?

[phansch]

I'm not really sure why cc_seme.rs produces the warning, I tried to reproduce it in a UI test, but there it seems to work. It also only seems to happen during compilation: when I run cargo test a second time, the warning is gone. Do you have some ideas how I could debug it? @oli-obk

edit: ok, nevermind I found out how to reproduce it locally:

cargo build --features debugging
cp target/debug/cargo-clippy ~/rust/cargo/bin/cargo-clippy
cp target/debug/clippy-driver ~/rust/cargo/bin/clippy-driver
~/rust/cargo/bin/cargo-clippy clippy --all -- -D clippy

[phansch]

I'm not really sure why cc_seme.rs produces the warning, I tried to reproduce it in a UI test, but there it seems to work. It also only seems to happen during compilation: when I run cargo test a second time, the warning is gone. Do you have some ideas how I could debug it? @oli-obk

edit: ok, nevermind I found out how to reproduce it locally:

cargo build --features debugging
cp target/debug/cargo-clippy ~/rust/cargo/bin/cargo-clippy
cp target/debug/clippy-driver ~/rust/cargo/bin/clippy-driver
~/rust/cargo/bin/cargo-clippy clippy --all -- -D clippy

[oli-obk]

Should I add test cases for all the other Item_ kinds as well or would that be too much?

Nah, the current tests are sufficient

Do you have some ideas how I could debug it?

That is very weird. Something must be converting the inner attribute into an outer attribute. You can try dumping the crate AST and look at that

[oli-obk]

Should I add test cases for all the other Item_ kinds as well or would that be too much?

Nah, the current tests are sufficient

Do you have some ideas how I could debug it?

That is very weird. Something must be converting the inner attribute into an outer attribute. You can try dumping the crate AST and look at that

[phansch]

Ok this seems like a weird problem.

I managed to dump the AST of cc_seme, but both attributes are inner attributes as expected.
Next I included the meta_item_list of the attribute in the error message and this is what I got:

warning: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute? 
Some([Spanned { node: MetaItem(MetaItem { name: dead_code, node: Word, span: tests/cc_seme.rs:1:1: 1:1 }), span: tests/cc_seme.rs:1:1: 1:1 }])

The attribute seems to be an allow(dead_code) with a span from 1 to 1. I managed to rule out feature(plugin) as a cause because it works if there is only feature(plugin) and plugin(clippy) in the file.

I did some more shots in the dark and now I'm sure that this is caused by the main method, in combination with feature(plugin) and plugin(clippy). For example this test will cause the warning to appear:

// ./test/weird_spans.rs
#![feature(plugin)]
#![plugin(clippy)]

fn main() {
    assert!(true);
}

So, somewhere an allow(dead_code) or deny(dead_code) with a span of 1:1 is inserted that does not seem to show up in the AST. That causes the warning. And this only happens if there is a main method. Naming the main method something else will not cause the warning.

I will try to dig deeper in the next days, but it looks like it will take some more time to get this merged

[phansch]

Ok this seems like a weird problem.

I managed to dump the AST of cc_seme, but both attributes are inner attributes as expected.
Next I included the meta_item_list of the attribute in the error message and this is what I got:

warning: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute? 
Some([Spanned { node: MetaItem(MetaItem { name: dead_code, node: Word, span: tests/cc_seme.rs:1:1: 1:1 }), span: tests/cc_seme.rs:1:1: 1:1 }])

The attribute seems to be an allow(dead_code) with a span from 1 to 1. I managed to rule out feature(plugin) as a cause because it works if there is only feature(plugin) and plugin(clippy) in the file.

I did some more shots in the dark and now I'm sure that this is caused by the main method, in combination with feature(plugin) and plugin(clippy). For example this test will cause the warning to appear:

// ./test/weird_spans.rs
#![feature(plugin)]
#![plugin(clippy)]

fn main() {
    assert!(true);
}

So, somewhere an allow(dead_code) or deny(dead_code) with a span of 1:1 is inserted that does not seem to show up in the AST. That causes the warning. And this only happens if there is a main method. Naming the main method something else will not cause the warning.

I will try to dig deeper in the next days, but it looks like it will take some more time to get this merged

[oli-obk]

Oh... just throw in a macro check for now. The attribute should probably have some expansion info.

[oli-obk]

Oh... just throw in a macro check for now. The attribute should probably have some expansion info.

[phansch]

Unfortunately, I was unable to find any macro expansion info in the spans I'm using. The is_macro check didn't catch that attribute, either. So I added a check for spans that have both locations at 0. I'm not sure that's the best way to do it, but it seems to fix the warning.

[phansch]

Unfortunately, I was unable to find any macro expansion info in the spans I'm using. The is_macro check didn't catch that attribute, either. So I added a check for spans that have both locations at 0. I'm not sure that's the best way to do it, but it seems to fix the warning.

[oli-obk]

What will happen if somebody makes a file like

#[allow(foo)]

fn main() {}

[oli-obk]

What will happen if somebody makes a file like

#[allow(foo)]

fn main() {}

[phansch]

duh, you are right, that would not be detected then 👍

I added a debugging commit, because I'm running out of time for today and want to pick it up again on Sunday.
Travis should show all kinds of information about the attribute. One thing that stands out to me is attr.span.end_point(): tests/weird_hidden_attribute.rs:1:1: 9:4287916127, but I'm not sure how to use that information, yet. I will try to figure out what happens during compilation.

I also added a separate test file at tests/weird_hidden_attribute.rs and it can be executed separately with CARGO_INCREMENTAL=1 TESTNAME=weird_hidden_attribute cargo test

[phansch]

duh, you are right, that would not be detected then 👍

I added a debugging commit, because I'm running out of time for today and want to pick it up again on Sunday.
Travis should show all kinds of information about the attribute. One thing that stands out to me is attr.span.end_point(): tests/weird_hidden_attribute.rs:1:1: 9:4287916127, but I'm not sure how to use that information, yet. I will try to figure out what happens during compilation.

I also added a separate test file at tests/weird_hidden_attribute.rs and it can be executed separately with CARGO_INCREMENTAL=1 TESTNAME=weird_hidden_attribute cargo test

[phansch]

Looking at the output and the Attribute docs again, it says:

attr.span.end_point() tests/weird_hidden_attribute.rs:1:1: 9:4287916577
attr.span.next_point() tests/weird_hidden_attribute.rs:1:2: 1:2

So, maybe it makes sense to check if the attribute span end_point is after the next_point, which should not happen with normal attributes, I guess?

[phansch]

Looking at the output and the Attribute docs again, it says:

attr.span.end_point() tests/weird_hidden_attribute.rs:1:1: 9:4287916577
attr.span.next_point() tests/weird_hidden_attribute.rs:1:2: 1:2

So, maybe it makes sense to check if the attribute span end_point is after the next_point, which should not happen with normal attributes, I guess?

[phansch]

I will update this comment for the next few hours.

Starting with this piece of code:

#![feature(plugin)]
#![plugin(clippy)]

fn main() {
    "hmm"
}

I managed to get the expanded macro with rustc -Z unstable-options --pretty=expanded and this contains an #[allow(dead_code)] above the main function

#![feature(prelude_import)]
#![no_std]
#![feature(plugin)]
#![plugin(clippy)]
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std as std;

#[allow(dead_code)]
fn main() { "hmm" }
<snip>

So, when iterating over the items, it picks up the allow for the main function, because it gets included for main somewhere. I added a allow(dead_code) into the original code and the expanded output now contains two allow(dead_code) for the main function.

#![feature(prelude_import)]
#![no_std]
#![feature(plugin)]
#![plugin(clippy)]
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std as std;

#[allow(dead_code)]
#[allow(dead_code)]
fn main() { "hmm" }
<snip>

The most interesting part is, is that one of the allow(dead_code) attributes has a span of 1:1 and the other has the correct span.
I think with snippet_opt I should be able to work around this, as the span of attributes that have been inserted by expansion always seems to be 1:1.
Additionally, I can use snippet_opt with the 1:1 span and check what it contains.

[phansch]

I will update this comment for the next few hours.

Starting with this piece of code:

#![feature(plugin)]
#![plugin(clippy)]

fn main() {
    "hmm"
}

I managed to get the expanded macro with rustc -Z unstable-options --pretty=expanded and this contains an #[allow(dead_code)] above the main function

#![feature(prelude_import)]
#![no_std]
#![feature(plugin)]
#![plugin(clippy)]
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std as std;

#[allow(dead_code)]
fn main() { "hmm" }
<snip>

So, when iterating over the items, it picks up the allow for the main function, because it gets included for main somewhere. I added a allow(dead_code) into the original code and the expanded output now contains two allow(dead_code) for the main function.

#![feature(prelude_import)]
#![no_std]
#![feature(plugin)]
#![plugin(clippy)]
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std as std;

#[allow(dead_code)]
#[allow(dead_code)]
fn main() { "hmm" }
<snip>

The most interesting part is, is that one of the allow(dead_code) attributes has a span of 1:1 and the other has the correct span.
I think with snippet_opt I should be able to work around this, as the span of attributes that have been inserted by expansion always seems to be 1:1.
Additionally, I can use snippet_opt with the 1:1 span and check what it contains.

[oli-obk]

Additionally, I can use snippet_opt with the 1:1 span and check what it contains.

sgtm

[oli-obk]

Additionally, I can use snippet_opt with the 1:1 span and check what it contains.

sgtm

[phansch]

I added an is_empty check for the attribute spans 👍 . From what I can see that should be enough. I'm just not sure how I could add a proper test for that. I guess it could also be fine without a test as tests/cc_seme.rs will show a warning.

[phansch]

I added an is_empty check for the attribute spans 👍 . From what I can see that should be enough. I'm just not sure how I could add a proper test for that. I guess it could also be fine without a test as tests/cc_seme.rs will show a warning.

[oli-obk]

Great! Thanks

[oli-obk]

Great! Thanks

[phansch]

Thank you!

[phansch]

Thank you!

[TheIronBorn]

Seeing what looks like a false positive on this:

/// A mapping from coordinates in the source sequence to coordinates in the sequence after
/// the delta is applied.

// TODO: this doesn't need the new strings, so it should either be based on a new structure
// like Delta but missing the strings, or perhaps the two subsets it's synthesized from.
pub struct Transformer<'a, N: NodeInfo + 'a> {
// warning: src/delta.rs:447: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make // it an inner attribute?
// note: src/delta.rs:447: #[warn(empty_line_after_outer_attr)] on by default
// help: src/delta.rs:447: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.184/index.html#empty_line_after_outer_attr
// warning: src/delta.rs:448: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
// help: src/delta.rs:448: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.184/index.html#empty_line_after_outer_attr

[TheIronBorn]

Seeing what looks like a false positive on this:

/// A mapping from coordinates in the source sequence to coordinates in the sequence after
/// the delta is applied.

// TODO: this doesn't need the new strings, so it should either be based on a new structure
// like Delta but missing the strings, or perhaps the two subsets it's synthesized from.
pub struct Transformer<'a, N: NodeInfo + 'a> {
// warning: src/delta.rs:447: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make // it an inner attribute?
// note: src/delta.rs:447: #[warn(empty_line_after_outer_attr)] on by default
// help: src/delta.rs:447: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.184/index.html#empty_line_after_outer_attr
// warning: src/delta.rs:448: Found an empty line after an outer attribute. Perhaps you forgot to add a '!' to make it an inner attribute?
// help: src/delta.rs:448: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.184/index.html#empty_line_after_outer_attr

[clarcharr]

This should special case doc comments.

[clarcharr]

This should special case doc comments.

[phansch]

I will have a look 👍

[phansch]

I will have a look 👍

[ozkriff]

btw, example on https://rust-lang-nursery.github.io/rust-clippy/v0.0.184/index.html#empty_line_after_outer_attr is broken - there're no empty lines:

[ozkriff]

btw, example on https://rust-lang-nursery.github.io/rust-clippy/v0.0.184/index.html#empty_line_after_outer_attr is broken - there're no empty lines: