Add new include_file_outside_project lint

[GuillaumeGomez]

So there is one issue with this lint: how to actually test it.

I wrote a test locally to check it works:

main.rs:

#![warn(clippy::include_file_outside_project)]
#![allow(dead_code, unused_variables)]

include!("/home/me/bar.rs");

fn main() {
    let x = include_str!("/etc/passwd");
    let x = true;
    let y = include!("/home/me/foo.rs");
}

/home/me/bar.rs:

fn foo() {}

/home/me/foo.rs:

if x { 0 } else { 1 }

Which outputs when I run cargo dev lint main.rs:

warning: attempted to include a file outside of the project
 --> /home/me/bar.rs:1:1
  |
1 | fn foo() {}
  | ^
  |
  = note: file is located at `/home/me/bar.rs` which is outside of project folder (`/home/me/clippy`)
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#include_file_outside_project
note: the lint level is defined here
 --> tests/ui/include_file_outside_project.rs:1:9
  |
1 | #![warn(clippy::include_file_outside_project)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: attempted to include a file outside of the project
 --> tests/ui/include_file_outside_project.rs:7:13
  |
7 |     let x = include_str!("/etc/passwd");
  |             ^
  |
  = note: file is located at `/etc/passwd` which is outside of project folder (`/home/me/clippy`)
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#include_file_outside_project

warning: attempted to include a file outside of the project
 --> /home/me/foo.rs:1:1
  |
1 | if x { 0 } else { 1 }
  | ^
  |
  = note: file is located at `/home/me/foo.rs` which is outside of project folder (`/home/me/clippy`)
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#include_file_outside_project

warning: 3 warnings emitted

changelog: Add new include_file_outside_project lint

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[GuillaumeGomez]

It seems like the error is kinda legit. The file is indeed outside of the (lintcheck) project. Anyway, let's first see if you anyone has an idea on how to add a ui test for this lint.

[kpreid]

Suggestion: Make this lint check #[path = "../../bar.rs"] mod foo; too. #[path] has basically all the same potential issues as include!().

[y21]

It seems like the error is kinda legit. The file is indeed outside of the (lintcheck) project. Anyway, let's first see if you anyone has an idea on how to add a ui test for this lint.

It would be good if this lint respected the publish field in Cargo.toml. Lintcheck sets publish = false as it's not intended to be published so I don't think it makes sense to lint there. clippy_lints/src/cargo does all the manifest parsing already so I imagine it could live- and reuse some of the logic there?

But also, I'm a bit confused as to why the lint thinks that all packages include clippy.toml? That doesn't seem right, afaict it's not include!d anywhere.

Is the problem with ui tests that the cargo manifest dir is set to rust-clippy and there would be no file to use in the test? We have tests/ui-cargo where the tests can have their own Cargo.toml so that would allow you to include some other file outside of it that's still in the clippy repo

[y21]

Looking a bit more into it, seems like assert!'s expansion contains a dummy span with a lo byte position of 0 which happens to be where clippy.toml is in the source map. So this needs to handle the Span::is_dummy() case

[bors]

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

[GuillaumeGomez]

Suggestion: Make this lint check #[path = "../../bar.rs"] mod foo; too. #[path] has basically all the same potential issues as include!().

Good idea!

It would be good if this lint respected the publish field in Cargo.toml. Lintcheck sets publish = false as it's not intended to be published so I don't think it makes sense to lint there. clippy_lints/src/cargo does all the manifest parsing already so I imagine it could live- and reuse some of the logic there?

True. However, if this is pushed on a (git) repository, I suppose the problem is the same. But I think it can be done as a follow-up. For now, let's only warn on publish = true.

Looking a bit more into it, seems like assert!'s expansion contains a dummy span with a lo byte position of 0 which happens to be where clippy.toml is in the source map. So this needs to handle the Span::is_dummy() case

Thanks, gonna add a check for that.

[GuillaumeGomez]

Found a way to add a ui test. PR should be ready now. :)