Skip to content

Add lint to detect unused Rust files in crate directory #544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Add lint to detect unused Rust files in crate directory #544

wants to merge 1 commit into from

Conversation

fhahn
Copy link

@fhahn fhahn commented Jan 10, 2016

This PR adds an used files lint.

It works as follows. It collects all Rust source files in the source directory and all subdirectories and also generates a set of all files used in the crate using CodeMap. It then computes the difference between the two sets to find unused files.

It was initially suggest in rust-lang/rust#28711, but I think clippy is a better fit for this lint.

@birkenfeld
Copy link
Contributor

How will this interact with modules included conditionally, depending on #[cfg] values?

@llogiq
Copy link
Contributor

llogiq commented Jan 10, 2016

I see four complications (one of which made the CI build fail):

  1. Build scripts (needs to check Cargo.toml)
  2. Tests (or modules used for testing, behind #[cfg(test)])
  3. Benchmarks
  4. Compiletest files or other source files used for building any of the above

@fhahn
Copy link
Author

fhahn commented Jan 10, 2016

@birkenfeld it seems like rustc populates CodeMap with all files for all modules, regardless of #[cfg]s. I have to check again though, because I think that changed fairly recently.

@llogiq

Build scripts (needs to check Cargo.toml)

Not sure what you meant by your first point, could you give additional information?

Tests (or modules used for testing, behind #[cfg(test)])

Should not be an issue (see above)

Benchmarks

Compiletest files or other source files used for building any of the above

This is the issue that causes th CI to fail, for the following reason. Each file in tests/compile-tests is compiled separately with rustc (and clippy enabled). Those are all standalone files, so all other files in the directory are marked as unused. For such cases we would have to provide a way to ignore those files in the lint. This could be some blacklist or an attribute. I created this PR to get some discussion going what makes the most sense.

@llogiq
Copy link
Contributor

llogiq commented Jan 10, 2016

Regarding cfg: That's certainly news to me. Look at the cargo docs for more information on build scripts.

@llogiq
Copy link
Contributor

llogiq commented Jan 10, 2016

The canonical way is to #[allow] a lint within a module, so the file(s) making up that module would be ignored.

@Manishearth
Copy link
Member

I don't think this lint should be searching outside of src/

@llogiq
Copy link
Contributor

llogiq commented Jan 10, 2016

Also I don't think the lint should warn by default – with so many ways to false positives, I'd make it allow by default.

@birkenfeld
Copy link
Contributor

I don't think this lint should be searching outside of src/

But isn't src/ only convention anyway?

@Manishearth
Copy link
Member

Sure, but there can be lots of things (including example files and benches!) that would reside out of it and it's really hard to tell them apart. Whereas you expect regular lib code under src. If there is no src, we don't know what to expect.

@fhahn
Copy link
Author

fhahn commented Jan 18, 2016

I've updated the PR. It now only checks files if the crate root is in src/. It's also allow as default. And I've added a test case, which shows that even though a module is hidden behind #[cfg] it gets added to the codemap anyways (https://github.com/Manishearth/rust-clippy/pull/544/files#diff-d2caebdde4a6d42d3ff0e0f217ef6615R10).

@oli-obk
Copy link
Contributor

oli-obk commented Dec 21, 2016

So I rebased this in https://github.com/Manishearth/rust-clippy/commits/unused-files3, but there's two big issues we need to fix.

  1. enabling the lint in a test file will state that all other test files are unused
  2. enabling the lint on a main.rs will lint for the lib.rs and vice versa

Not sure how to proceed, but it feels to me like this should rather be something that a cargo subcommand should do.

@oli-obk oli-obk added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Dec 21, 2016
@oli-obk
Copy link
Contributor

oli-obk commented Jan 30, 2017

opinions? This could be something that cargo publish complains about, similiar to --allow-dirty

@llogiq
Copy link
Contributor

llogiq commented Feb 25, 2017

cargo-modules shows unused modules, too.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 24, 2017

continued in cargo: rust-lang/cargo#3754

@oli-obk oli-obk closed this Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants