search

krdln / shelly

Shelly — very dumb PowerShell script analyzer
MIT License
3 stars 1 forks source link

Initial changes for detecting unused imports. #16

Closed m-kostrzewa closed 6 years ago

m-kostrzewa commented 6 years ago

Ran it on contrail-windows-ci repo, found a bunch of false positives: https://gist.github.com/m-kostrzewa/5bad3112320bf0c63d0012bc137c9567

False positives mainly for files that import files without defined functions (like Init.ps1) or with classes only (like Job.ps1).

Not sure how to handle either of those scenarios with current Shelly implementation, since it only handles functions for now - not classes or files without functions. Asking the user to specify allow unused-imports for all these files is too cumbersome IMO.

Suggested workaround: don't raise unused-imports, if file has no function definitions. This may create false negatives or whatever for unused classes.

@krdln thoughts?

krdln commented 6 years ago

Sorry for not grouping comments in one review, I thought I'd give just two quick comments and move on, but, you know...

Not sure how to handle either of those scenarios with current Shelly implementation, since it only handles functions for now - not classes or files without functions. Asking the user to specify allow unused-imports for all these files is too cumbersome IMO.

Yup, that's why I wanted to finish the parser, so it gathers also classes and top-level variables before implementing this check.

Suggested workaround: don't raise unused-imports, if file has no function definitions. This may create false negatives or whatever for unused classes.

It may even still create false positives – user imports a class+function file, but uses only a class.

I'd gate this lint, just as you described, but also, for now I'd set the default lint level to allow (to use you'd need too shelly analyze --warn unused-lints).

m-kostrzewa commented 6 years ago

I'd gate this lint

what do you mean by gate?

krdln commented 6 years ago

I'd gate this lint, just as you described

what do you mean by gate?

I meant to disable it on functionless files. Sorry for the wording.

krdln commented 6 years ago

So... I tried to get rid of "definitions per import" and I've broken the tests.

So the problem is in combination of unused-imports with with indirect-imports. Your code took into account where the indirect import came from:

        let all_imported = nested.defined.union(&nested.directly_imported).cloned().collect();
        scope.definitions_per_import.insert(imported_file, all_imported);

whereas my reports "unused import" (because it's not used (at least directly)). I think I will re-add something like your definitions_per_import.

Also, why nested.defined ∪ nested.directly_imported? That's not all imported.

krdln commented 6 years ago

Ok, tests fixed. And I've improved indirect import diagnostics as a side effect :)

indirect-import

krdln commented 6 years ago

@m-kostrzewa LGTY?

Also, I'm not sure with warn-by-default. It produces quite a lot of warnings on contrail-windows-ci repo, even after implementing silencing we discussed. But I've looked into few of them and they seem legit.

Also, clap is awesome! clap-is-awesome-allow