Skip to content
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

Designing a dead code detection rule that needs to analyze across files #2431

Closed
2 tasks done
elliottwilliams opened this issue Oct 2, 2018 · 9 comments
Closed
2 tasks done
Labels
rule-request Requests for a new rules.

Comments

@elliottwilliams
Copy link
Contributor

I'm looking into the whether it would be possible to create an analyzer rule to detect dead code in a project. It seems like this is totally doable with CursorInfo requests, but I'm at a loss for how a rule like this would integrate into SwiftLint.

For dead code detection to be useful, it has to work between files: it should build up a set of referenced declarations across a project, and then look at the declarations in a file to determine whether that declaration is ever referenced. This means that to lint any single file, the whole project (i.e. the other files given to SwiftLint) needs to be analyzed first.

Since SwiftLint rules are designed to run per-file, in isolation, I'm not sure how I would build this rule. Could it make a pass over every file to build a references set, and then run again on each file to perform actual linting? Should I write a separate script to build a references set, and pass it to SwiftLint via a command line argument? Are multi-file rules something SwiftLint wants to support in the future?

New Issue Checklist

@realm-probot realm-probot bot added the O:User label Oct 2, 2018
@kimdv
Copy link
Contributor

kimdv commented Oct 2, 2018

There is one for private declarations.
Maybe that can be extended ?

Sent with GitHawk

@jpsim
Copy link
Collaborator

jpsim commented Nov 28, 2018

I'm looking into the whether it would be possible to create an analyzer rule to detect dead code in a project.

Great! This has been next up on my list of analyzer rules I wanted to build. I'd love to support you in doing this in whatever way I can. But it's predicated on this next part:

Are multi-file rules something SwiftLint wants to support in the future?

Yes, but this will need significant changes. I was thinking of having a "analyzer pass" model where the first part of a rule would collect information for later processing, then once all files had been processed, a final phase would generate violations from all the collected info.

Rough idea:

protocol AnalyzerPass {
    associatedtype FileInfo
    func collect(file: File) -> FileInfo
    func validate(fileInfo: [FileInfo]) -> [StyleViolation]
    func correct(fileInfo: [FileInfo]) -> [Correction]
}

For a dead code detection rule, its FileInfo could look like this:

struct USR {
    let identifier: String
    let location: Location
}

struct DeadCodeFileInfo {
    let declaredUSRs: [USR]
    let referencedUSRs: [String]
}

@mikemee
Copy link

mikemee commented Dec 5, 2018

Side note, https://peripheryapp.com/, seems to provide this.

I have no connection with periphery, but found them when I realised that SwiftLint is currently scoped to review in a local context, not project wide and went hunting. Of course adding project wide context would add opportunity for lots of new features – including things like localization file linting as described here: https://medium.com/stash-engineering/how-to-keep-your-ios-localizable-files-clean-swift-script-edition-a01bc649ef1d.

@jpsim
Copy link
Collaborator

jpsim commented Dec 5, 2018

Periphery is pretty great! But SwiftLint is free, built in the open and contributor-extensible 😄.

@elliottwilliams
Copy link
Contributor Author

elliottwilliams commented Dec 6, 2018

Thanks for the feedback!

I was thinking of having a "analyzer pass" model where the first part of a rule would collect information for later processing, then once all files had been processed, a final phase would generate violations from all the collected info.

Here's how I'm imagining something like AnalyzerPass would be implemented. Let me know if this seems like a reasonable approach! I'm definitely still getting my feet wet in the codebase:

  • Rules can be AnalyzerPassing by adopting a protocol like the one you suggested.
  • Each Linter takes a list of analyzer-passing rules, but only runs one stage of the rule on the file.
  • LintableFilesVisitor is aware of the analyzer state of each of its files, maintains shared [FileInfo] that has been collected, and can provide (File, Linter) pairs such that all the collection stages get run first, followed by validations and corrections for all files given to the visitor.
  • Code calling the visitor is passed each file multiple times, first with the linter ready to collect file info and later with the linter ready to validate.

This means that Linter would end up looking roughly like

struct Linter {
  var stage: LinterStage
  
  enum LinterStage {
    case collectable(CollectableLinter)
    case analyzable(AnalyzableLinter)
  }
  
  struct CollectableLinter {
    func collect() -> FileInfo { ... }
  }

  struct AnalyzableLinter {
    func validate(fileInfo: [FileInfo]) -> [StyleViolation]) { ... }
    func correct(fileInfo: [FileInfo]) -> [StyleViolation]) { ... }
  }
}

@jpsim
Copy link
Collaborator

jpsim commented Dec 23, 2018

Hi @elliottwilliams, yes this sounds more or less in line with what I had in mind.

@elliottwilliams
Copy link
Contributor Author

Hey! It's been a little while, but I've been working on this in my free time over the last month and have something to share. #2714 is a working implementation of the "analyzer pass" model for rules that can collect data before processing. @jpsim, i'd be happy to get design feedback and any guidance on how to make this shippable 🙂

@marcelofabri
Copy link
Collaborator

Can we close this now that #2814 and #2714 have been merged?

@elliottwilliams
Copy link
Contributor Author

Sounds good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule-request Requests for a new rules.
Projects
None yet
Development

No branches or pull requests

5 participants