Missing inspections for unused class, unused public methods and properties

[ghost]

@ph55 commented on Jun 16, 2020, 1:18 PM UTC:

Feature request / improvement.

Classes missing some inspections for

• Unused class itself
• Unused public properties
• Unused public methods

Steps to Reproduce

class Test {
  final unusedPublicProp = 0;
  final _unusedPrivateProp = 0;

  unusedPublic(){
    return 0;
  }

  int _unusedPrivate(){
    return 0;
  }
}

There is no inspection highlight for unused public methods/properties and class:

Message only pop-up when you Ctrl+click on method/property/class to find usages:

This issue was moved by pq from flutter/flutter-intellij#4629.

[pq]

/fyi @bwilkerson

[bwilkerson]

That's intentional. The kind of dead code analysis that's being requested here requires (a) that we make a closed-world assumption that the analyzer does not (and should not) make, and (b) that those elements will never be referenced via dynamic references in such a way that code would break if they were removed. Neither assumption is safe, so we don't try to implement that kind of dead code analysis.

[cubuspl42]

@bwilkerson I do not agree with you, and probably neither do 90% (guessed number) of Dart developers who do not develop Dart libraries, but create applications (be it Flutter apps or web apps). It's a real world problem to find out that some portion of the app hasn't been used for six month and nobody noticed. I'm speaking from my own experience, and I'm not talking about some low-quality spaghetti-code codebase, but about a reasonable project.

The assumption that the analyzer should not make a closed-world analysis is, in my opinion, arbitrary. For apps (leaves of package dependency graph), analyzer obviously can do closed-world analysis. Nobody is saying that the inspections being suggested should be turned on by default. If something helps a great majority of Dart developers (who can and want to turn such inspections on; I think app developers), then not adding them only because some other developers (probably developers of libraries) couldn't or wouldn't want to enable them is not a strong argument.

For me it's difficult to relate to the point about reflection, because you can't use reflection in Flutter (which I depend on). I am also personally very sceptical about using reflection when a problem can be solved using code generation. That being said, it's still not a conceptually hard problem to handle this in analyzer. As classes referenced purely through reflection are a small minority of all code, a meta annotation could be added to mark them as @actuallyUsed, or something like that.

I'm asking (and I believe I really do that in the name of all apps developers) to reopen this issue and reconsider adding support for closed-world analysis. Thank you in advance.

[srawlins]

@cubuspl42 Thanks for the explanation. It's good to hear the perspective of someone who is primarily concerned with a Dart application, a leaf node.

For the analyzer, however, this concept is not a good fit. The analyzer really just analyzes library-by-library, and produces diagnostics library-by-library. You can run analyzer on individual files, and on specific directories (or globs?) which doesn't fit the whole-world requirements this problem requires.

I think the best way to provide what you're looking for is to write a (probably small) wrapper around the analyzer's "used local elements" code, GatherUsedLocalElementsVisitor, UnusedLocalElementsVerifier, and UsedLocalElements. I think this would have a lot of good code-health impact. You could detect:

• Entire libraries in a package which are not imported by other libraries
• As you write above, unused public classes, public methods, public fields, public top-level functions and variables
• Unused exports

An advanced system could build a reference graph and detect which things are not referenced by any of the main entrypoints (like the app main and the test main functions). So if two public methods reference each other, but no other code references those two methods, they would be reported as unused.

This tool would not have to be authored by the Dart team, but it could be.

[srawlins]

Additionally, this request is very similar to dart-lang/linter#1034; follow that issue for potential updates.

[cubuspl42]

@srawlins Thank you for your explanation, it gives some light on how analyzer is implemented. I understand that such request (and every other that assumes closed-world analysis) is impossible with current analyzer architecture, which obviously makes it difficult or nearly impossible to address in a short term.

I still believe that it's arguable, that this concept is not a good fit for the analyzer. Maybe when knowing it current implementation details, sure. But what analyzer is from the Dart developer's perspective? Analyzer is the extension of the compiler. Analyzer defines half of develper's experience when using Dart. It's present in both CLI and in IDEs through the analysis server. It's what makes Dart compete with other modern programming languages.

In Kotlin, there's tons of powerful analysis built-in. I was recently shocked when I was informed by the IDE that one of the arguments of functions I created has always value 0 (because uses of this functions, in other files, actually always provide value 0). Why shouldn't Dart compete?

I do agree with you that such solution could be implemented in an external tool, but I'm worried that it would greatly limit its usability and would add unnecessary complexity. One would expect that such analysis would be present both in command-line and in IDEs. A plugin would need to be implemented for each IDE (Intellij, VS Code, other...). What would the CLI and graphical UI look like? Well, it would look exactly like Analyzer. A list of issues found, a list of entries in form (problem, location in code). What about the configuration? Enabled rules, some marked as error (if they're considered critical by the team).

So we'd end up with a separate tool, which provides benefit for many, many Dart developers, which looks like analyzer, works like analyzer, is configured like analyzer, would be present in the IDE next to analyzer and plugged into CI/CD next to analyzer. It could be named analyzer-bis 😉

I'm not arguing that this is something that should be put into analyzer right here, right now. I do understand that it would require fundamental refactoring of analyzer's internal. I do argue that the only reasonable place for such functionality is the analyzer. Additionally, such functionality cannot be put into analyzer by the community; a PR adding a lint to existing analyzer's structure would be (I assume?) welcomed, while a major refactoring must be (I assume, again) initiated and approved internally by the Dart team.

Please just consider adding a closed-world analysis layer to the analyzer, or restructuring its frame so it can easily reason about things happening both inside a single library and across libraries. I'm talking about perspective as far as next major version of Dart.

Thank you!

[bwilkerson]

I'm not arguing that this is something that should be put into analyzer right here, right now. I do understand that it would require fundamental refactoring of analyzer's internal.

Actually, the analyzer is fully capable of performing whole program analyses, we just don't currently support them because we can't prove that we have the whole program available to us. But I'd be interested in exploring this line of reasoning a bit farther as I think these kinds of analyses could potentially bring value to our users.

In Kotlin, there's tons of powerful analysis built-in. I was recently shocked when I was informed by the IDE that one of the arguments of functions I created has always value 0 (because uses of this functions, in other files, actually always provide value 0).

Does the Kotlin analysis know when whole program analyses are valid in a given code base (that is, does it know that no other pieces of code will invoke code in the code being analyzed), or are such analyses applied to all code bases? If the former, then how does it know? If the latter, do you find this to be more helpful or annoying when writing packages that are intended to be reused by other packages? How often are you writing code for a package that is intended to be used by other packages?

[cubuspl42]

@bwilkerson

Does the Kotlin analysis know when whole program analyses are valid in a given code base (that is, does it know that no other pieces of code will invoke code in the code being analyzed), or are such analyses applied to all code bases? If the former, then how does it know? If the latter, do you find this to be more helpful or annoying when writing packages that are intended to be reused by other packages?

I don't know the answers. I rarely write in Kotlin, only minor bits of the native side of our Flutter app (and some tiny side-projects). But actually that was kind of my point. I was pleased and positively surprised that the language [/analysis server] helped me with deep and useful inspections, without me knowing all implementation details and things that made that possible. I'd love to see such behaviour in Dart/Flutter (because that's what I and many other developers use).

For Dart, the information if package is app [leaf] or library [non-leaf node] could be present in the pubspec explicitly or be handled solely by the fact that the inspections would be opt-in. Rare reflection-only used classes/items could be handled by a special annotation (similarly to other corner cases, like unawaited). Maybe Kotlin takes similar approach? Maybe Intellij projects created with "command line application" or "Android app" (I don't remember if such templates exist, but you get the point) have necessary entries present? I don't think that this matters that much to Dart developers and community; we care about Dart! :)

How often are you writing code for a package that is intended to be used by other packages?

Hardly ever, my team develops a single app.

[bwilkerson]

I don't think that this matters that much to Dart developers and community; we care about Dart! :)

So do I. :-)

My goal was to try to determine whether there is some prior art that should be considered when designing a solution to this problem. I appreciate your comments and suggestions. I don't know when we'll be able to devote resources to working on this problem, but I do hope that we can do so at some point.

[ph55]

I'll add some thoughts. In general - consistency.

IntelliJ provides mentioned inspections built-in for JS and PHP and many other languages.
One that start using Dart/Fluter for Mobile/Web development would expect to have such feature.

Not having such feature might leave impression of immaturity of the dart dev environment.

[devoncarew]

Re-opening to facilitate the general discussion.

[lukepighetti]

I don't have much to add here except that just about anything, even a little CLI tool for detections of unused & not exported classes, would be much better than what we currently do.... go around uncommenting stuff to try to find the leaf classes, then do it again, and again, doing passes until we're sick of doing it.

[IlyaMax]

I don't have much experience with JS and PHP. As I see above these languages has mentioned features. But I don't understand how it can be used in cases when some tasks are paralleled and you have pull request ci pipelines for analyzer check. I need to ignore lints every time when feature is not completed? So maybe it would be better as @lukepighetti suggested make some CLI tools to analyze project periodically to remove unuzed public classes and members.

[srawlins]

I agree.

There are plenty of similar examples of "presubmit checks" that do not occur for every character type in the IDE, but that only happen during CI (or with an explicit command line). And you're right that anything would be better than what is given today. I was just thinking of this project yesterday and have renewed interest in exploring an idea. :)

[lukepighetti]

Has anyone tried coming up with a solution to this? Finding dead code in Dart/Flutter projects is still very difficult. Everyone I know just runs around commenting out code to see if the analyzer pops up an error. Doing this in a 250k+ LOC project is untenable. I wouldn't need this running in realtime, although it would be nice... just a little CI check script would be fine.

[bwilkerson]

Everyone I know just runs around commenting out code to see if the analyzer pops up an error.

Ouch! That's terrible.

Has anyone tried coming up with a solution to this?

Yes. I wrote a prototype a while back, but didn't have time to get it to product quality (though it does understand the difference between an application and a re-usable package). It essentially does the same kind of tree shaking that dart2js uses to minimize the size of an executable.

There are some caveats, such as not being able to trace references through dynamic, etc., but that's probably not as big an issue as it once was, and users can be made aware of the limitations and their implications and account for them.

It isn't a particularly difficult piece of code to write (tree shaking is a well understood problem), but for us to do it would require that the priority of this is higher than the priority of other tasks so that we could allocating resources. I have no idea what the priority of this would be.

[lukepighetti]

A script POC would be powerful enough to be useful, and it could assume that it was working in a leaf project for sure

Beyond that I would say that a pubspec.yaml entry like package: false would be fine, and no need to try to sense it. Not sure how you could

[bwilkerson]

Yeah, I didn't try to tell the difference based on static analysis, I just added a command-line argument that told me which kind of package I was looking at. If we did want to be more automated, one possibility would be to look for the publish_to key.

[lukepighetti]

Any chance you have this visible online somewhere?

[bwilkerson]

Sorry, but no. And unfortunately, it was written long enough ago that there are new language features that haven't been tested, so I'm not sure it works with today's version of Dart.

[lukepighetti]

@incendial any chance you've attempted to solve this?

[incendial]

@lukepighetti actually, I thought about that a while ago and have a naive idea how to make it work, but no code yet. Also, haven't checked how dart2js, Google Closure Compiler, terser or other similar libraries remove dead code, but definitely should.

@bwilkerson maybe you could share the best starting point (a code file or some document) to read dart2js tree shaking implementation?

[incendial]

Dart Code Metrics prerelease version 4.10.0-dev.1 provides new command called check-unused-code which could help you find unused classes, function and other top level declarations. You can find more info here: https://dartcodemetrics.dev/docs/cli/check-unused-code.

Please note that the command is now in dev release and will be released as a stable soon, if we won't receive major blockers. So any feedback is very welcome!

The command doesn't search for particular methods, but it's definitely doable and we probable will add this in the next iteration.

[dJani97]

Dart Code Metrics prerelease version 4.10.0-dev.1 provides new command called check-unused-code which could help you find unused classes, function and other top level declarations. You can find more info here: https://dartcodemetrics.dev/docs/cli/check-unused-code.

Please note that the command is now in dev release and will be released as a stable soon, if we won't receive major blockers. So any feedback is very welcome!

The command doesn't search for particular methods, but it's definitely doable and we probable will add this in the next iteration.

dart_code_metrics has been discontinued and is no longer maintained. They moved to providing a paid service. This brings back the need for an official solution.

[caseycrogers]

Just jumping in to bump this. I'm pretty sure my app is completely littered with unused public members and classes, but I only find them by accident from time to time. It makes the code base a lot slopier and harder to navigate. I've had times during significant refactors where I refactored a method only to realize after that it wasn't being called anywhere.

[srawlins]

I'd surely love to prioritize this item. :)

@caseycrogers, for now, what I do is use my IDE's "Find all references" feature to see if something is unused; or rename the thing to see if anything shows up in the Problems panel. It is surely stumbling around in the dark, but once I am curious to see if an element is used, there is a definitive way to check.

[snowb1shop]

Would be great to have the possibility of turning it on