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. :)

[GuillaumeGomez]

r? clippy

[Alexendoo]

This would have to be a cargo lint if it's doing this to deduplicate the cargo metadata invocation and so it doesn't happen by default

But I don't think we need to here, cargo publish already copies the project files into target/package/crate-x.y.z before checking it builds, which would catch most ../../outside-the-project includes

The case that it wouldn't catch is an include of an absolute path, but it seems reasonable to lint these unconditionally

[rustbot]

☔ The latest upstream changes (possibly d28d234) made this pull request unmergeable. Please resolve the merge conflicts.

[Jarcho]

Ping @GuillaumeGomez from triage. Do you intend to return to working on this?

I'm going to agree with @Alexendoo. We really don't want to run cargo metadata from clippy.

[GuillaumeGomez]

I do but likely directly inside rustc or cargo. I kept it open to have it inside my TODO list. ^^'