Add support for Go standard use of ./...

ericcornelissen commented 3 years ago

This adds support for Go standard use of ./... to recursively analyze a directory.

Note that this breaks current usage in two ways:

Current invocation paths that are recursive will no longer be recursive. Currently all invocations are recursive unless the -n flag is used. After this change such invocations will be non-recursive unless the "/..." suffix is added.
The -n flag is deprecated as it is no longer meaningful with ./... support. I deprecated it instead of removing it because if the flag is used when not registered the program exits with an error before doing anything.

gordonklaus commented 3 years ago

Thanks for the PR.

Ideally, I think this feature would come from using golang.org/x/tools/go/analysis (or at least golang.org/x/tools/go/packages). If you're willing to rework this change to use one of those packages, that would be great. If not, I totally understand – just say so and I'll merge this as-is.

ericcornelissen commented 3 years ago

Ideally, I think this feature would come from using golang.org/x/tools/go/analysis (or at least golang.org/x/tools/go/packages). If you're willing to rework this change to use one of those packages, that would be great. If not, I totally understand – just say so and I'll merge this as-is.

I'm afraid both of those would require more refactoring than I'm comfortable with.

If I'm mistaken, then if you could give me some indication of where/how to get started I'm willing to give it a try :slightly_smiling_face:

gordonklaus commented 3 years ago

I don't know what you're comfortable with so you'll have to tell me :)

I think it should be as simple as:

creating an analysis.Analyzer with Run: checkPath
adapting checkPath to have the correct signature and use analysis.Pass.Files instead of parsing the files ourselves
implementing main using singlechecker

Of course, nothing ends up being quite as simple as one expects!

ericcornelissen commented 3 years ago

Thanks for that, that was enough for me to do it - I think. The changes in the PR are now a bit more substantial, but ultimately the amount of code was reduced quite a bit. I'm not 100% everything works as one would expect, but from my manual tests it works as I would expect. Let me know if anything is missing or anything in the code should be changed.

The only problem I wasn't quite able to figure out myself was the automated tests... I attempted to use analysis/analsysistest but that doesn't play nicely with the testdata in this repo. It seems that it doesn't work because the testdata has compilation problems, but I'm not sure how to resolve this. If you want @gordonklaus, feel free to push changes for that directly to this branch.

Also, the help messages changed significantly due to the usage of the analyzer. I'm not sure what you think about it, it seems that most flags work out the box. But, regarding #53, the -V flag to print the version does not and I don't know how to get that to work :thinking:

ineffassign: detect ineffectual assignments in Go code

Usage: ineffassign [-flag] [package]

Flags:
  -V    print version and exit
  -all
        no effect (deprecated)
  -c int
        display offending line with this many lines of context (default -1)
  -cpuprofile string
        write CPU profile to this file
  -debug string
        debug flags, any subset of "fpstv"
  -fix
        apply all suggested fixes
  -flags
        print analyzer flags in JSON
  -json
        emit JSON output
  -memprofile string
        write memory profile to this file
  -n    don't recursively check paths (deprecated)
  -source
        no effect (deprecated)
  -tags string
        no effect (deprecated)
  -trace string
        write trace log to this file
  -v    no effect (deprecated)

gordonklaus commented 3 years ago

Nice work! I pushed some changes to your branch, including making the automated tests work and restoring the ignoring of generated files (which was lost with your changes).

I think we can worry about the -V flag later. Seems like a bug in the singlechecker package.

It sounds like you're happy with the changes, but anyway let me know if that's still the case and I'll merge it.

ericcornelissen commented 3 years ago

Awesome, thanks for the help! I made a small adjustment to 7c2182dd6ad00e91233228363dbad056f812f78c to make the code a tiny bit easier to read (if you don't like it, feel free to revert that change).

and restoring the ignoring of generated files (which was lost with your changes).

Oops, forgot to test that :sweat_smile: Thanks for adding it back! It's a shame that dmitri.shuralyov.com/go/generated doesn't support ast.File as input... Or do you think this is something that should be supported by golang.org/x/tools/go/analysis? :thinking:

gordonklaus commented 3 years ago

Nice!

It's a shame that dmitri.shuralyov.com/go/generated doesn't support ast.File as input... Or do you think this is something that should be supported by golang.org/x/tools/go/analysis? 🤔

Perhaps. I created https://github.com/golang/go/issues/43481 to find out.

gordonklaus / ineffassign

Add support for Go standard use of ./... #51