Skip to content

Analyzer not hinting for unused const's and var's in top-level #23957

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
sethladd opened this issue Aug 4, 2015 · 11 comments
Closed

Analyzer not hinting for unused const's and var's in top-level #23957

sethladd opened this issue Aug 4, 2015 · 11 comments
Labels
devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@sethladd
Copy link
Contributor

sethladd commented Aug 4, 2015

Consider this code:

const foo = 'X';
var bar;

main() {}

Expected: hints that foo and bar are not used.

Actual: no hints.

@sethladd sethladd added legacy-area-analyzer Use area-devexp instead. devexp-warning Issues with the analyzer's Warning codes labels Aug 4, 2015
@harryterkelsen
Copy link
Contributor

I don't think there should be hints in this case. What if I am writing a library that exports a constant? For instance https://github.com/dart-lang/charcode/blob/master/lib/ascii.dart

@bwilkerson
Copy link
Member

What if I am writing a library that exports a constant?

Exactly. There would be too many false positives for a rule like this.

@sethladd
Copy link
Contributor Author

sethladd commented Aug 4, 2015

Bummer. After a refactoring, I had unused constants. I wondered what they were for. I took them out, no hints. I ran the tests, no errors. Maybe I'm safe?

@bwilkerson
Copy link
Member

Maybe. Are they public names in a published package? Do you have access to every piece of code that imports your package? If not, then it's impossible to know whether it's safe to remove them.

That said, if they're private names, then it's safe, but we don't currently provide a hint and we should.

And it's possible that if it's in non-public library (in lib/src or anywhere other than lib) then perhaps we could provide some kind of feedback (although the cost of doing so might not be worth the value).

But I do think we should handle private names, so I'm re-opening the issue.

@sethladd
Copy link
Contributor Author

sethladd commented Aug 4, 2015

If not, then it's impossible to know whether it's safe to remove them.

I'm not asking for a tool to tell me if it's ok to change a public API. :) That's impossible, as you point out.

If I change a public API, I need to bump my package version anyway. That's a different mental and technical exercise, though quite important.

I'm really curious to know if my app uses this code, so I know if, after a refactoring, it's safe to delete code.

In the meantime, I've been manually right-clicking on names and selecting "find references" and if that list is empty, I delete it.

@bwilkerson
Copy link
Member

I'm really curious to know if my app uses this code ...

Which is totally reasonable. My concern is that if we make that information a hint that some users will be confused and think we know with certainty that it isn't being used anywhere (without thinking, for example, about the subtle differences between public and private variable visibility).

I think that using "Find References" is exactly the right way to get the information you're looking for.

@kevmoo kevmoo added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug and removed Priority-Medium labels Mar 1, 2016
@srawlins
Copy link
Member

I wonder if the analyzer could have a mode (or a separate package), like --app-mode, or something, that tells you which classes, methods, etc. are not used within this package, and which are used only in tests (essentially unused). This is starting to sound like a new package that uses the analyzer.

@zoechi
Copy link
Contributor

zoechi commented Mar 27, 2016

@srawlins I think a lint for this would be ideal. Lints are easy to enable/disable.

@bwilkerson
Copy link
Member

This is starting to sound like a new package that uses the analyzer.

Or a new application that finds dead code (in the sense of members that are not referenced by a given application. Of course, this is essentially the tree shaking that dart2js already does.

@ghost
Copy link

ghost commented Mar 27, 2016

Sorry, novice here - hope it's relevant. I would like to see conditional compilation. I'm swapping two versions of a file of constants, depending on whether I'm developing or deploying.

@srawlins
Copy link
Member

This request has been moved to the linter project: #57724.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants