Use analysis.Analyzer

[bombsimon]

I did a quick update for the linter to use Go's analyzer which has some great features. This linter no longer has to figure out paths to files, tests, filenames, lines etc. It's also super easy to maintain tests since you just add diagnostics to your test package and write a oneliner test using analysistest. I had one issue with testing test files as you can see which resulted in an upstream issue here. The test code generated will have global variables which will make the test not pass.

The main reason however was to address golangci-lint#1393 and make it easier to keep golangci-lint in sync with this linter.

After merging this it will be super easy to just add this analyzer to golangci-lint like this.

Please let me know if you think this is a good idea or if you want any changes to this approach!

[leighmcculloch]

🎉 Woah, very nice. You beat me to it. Thank you. Some questions below. I'm wondering how we can reduce the diff so that the tests stay mostly the same to ensure that this is only a refactor of code, and not a change in behavior and functionality. Thoughts?

[leighmcculloch]

What causes us not to be able to use the existing tests? The behavior of the tool isn't changing, so it should be possible in theory to make minor alterations to the tests but their setup and expectations should still be valid.

[bombsimon]

Since there's no longer any code to traverse directories, open files and do all the work before getting to the ast I didn't keep them. It felt bad to keep the code or write new code for this kind of job when it's handled by the analysis package already and won't be a part of the code.

To address what you mention in the comment - we could keep the testdata structure and move each folder into testdata/src then add 10 different package tests. However, some of them will be irrelevant since their only purpose was to test the file and directory walking.

I'll take a pass and start looking into how I can minimise the diff.

[bombsimon]

Good input on reducing diff and only doing one thing per PR!

I dropped the nolint support since it's non existing today! I converted the way we parse the -t flag as a type cast for clarity. I merged your suggestions for the README.

I also tried to keep all the test files/packages. As mentioned above I can't keep them 1:1. First of all, we don't have any code walking directories finding files anymore. Secondly as mentioned in the test with a comment, I cannot get tests to pass by enabling packages including tests (with or without the flag).

The last bit I can think of now is if we wan't to wrap the analyser in a constructor to follow the linters own principle of having no globals or if we should keep going with the common way of having the analyzer as a global variable.

Please give this a new pass and let me know what you think!

[bombsimon]

Oh crap... The issue with analysis is not limited to tests. This has to do with the analyzer itself!

% gochecknoglobals ./...
/Users/simon.sawert/git/gochecknoglobals/check_no_globals.go:30:5: Analyzer is a global variable
/Users/simon.sawert/Library/Caches/go-build/c2/c22bf8402833e9a68e0989ace01544e619f369592b95532b3a35bf2f5d9fba5d-d:19:5: tests is a global variable
/Users/simon.sawert/Library/Caches/go-build/c2/c22bf8402833e9a68e0989ace01544e619f369592b95532b3a35bf2f5d9fba5d-d:25:5: benchmarks is a global variable
/Users/simon.sawert/Library/Caches/go-build/c2/c22bf8402833e9a68e0989ace01544e619f369592b95532b3a35bf2f5d9fba5d-d:29:5: examples is a global variable

I'll dig into this to see how we can eliminate this! Feel free to give input if you have any suggestions.

[leighmcculloch]

Hmm, I'd need to dig into the other issue you saw, probably later tonight. I'll come back to you.

[bombsimon]

Hmm, I'd need to dig into the other issue you saw, probably later tonight. I'll come back to you.

Thanks a bunch! I'll fix the other things! Currently the only idea I can think of would be to only parse files with a .go prefix. This would make generated code without this prefix not targeted.

To get some more info on this I did a change in go-mnd which prints all files it passes and ran it on this project and as expected, it also passes the build cache. But since there's no go-mnd issues in those, there's no problem.

I'll commit and push a .go suffix requirement for now and we can see if we find a better solution!

[leighmcculloch]

I had another look through the diff and this looks great! 🎉

I didn't know about the analysistest package and I love that it simplifies things.

I need to test it out on a few projects before merging, but I'll need a few days to do that because of other commitments. I'll come back to you soon.

[leighmcculloch]

This looks great.

I've pushed two commits.

The first reduces the diff a little more, but retaining the same logic and functionality. The main change in this is combining the run and checkNoGlobals functions. I find it easier to understand the checker if its logic is in one function, which is possible given it is so short.

The second moves the main back to the root and moves the logic to a sub-package. This keeps the install instructions the same and doesn't break existing installs. I think this also fits the type of program better too. It is primarily a binary, so that should be the first thing people see.

I ran some tests and I noticed that there are four behavior that weren't evident from the tests:

1. The exit code for the command has changed from 1 to 3 in the case that the check fails. This could break any scripts that are explicitly checking for the value 1. Chances are any scripts wrapping this are probably checking for any failing exit code so this will probably effect no one or a small number of people so I think lets ignore this issue. It's also good that the tool will return an exit code consistent with other analysis tools.
2. The usage output of the tool seems to be pretty complicated now because it has a ton of other default flags. I think it might be better to retain the original main function logic so that the flags are simpler, but I'm not attached to that. I can see good reasons to use the default usage.
3. The output now includes the full absolute path for each file instead of a relative path. I think this is fine and clearly it is more consistent with other analysis tools if that's how it works.
4. The old tool would run and check ./... in the case no package was provided.

All in all I think this is good to merge 🎉 except for point 2 and 4 above. I'd love your thoughts on them.

[Edit: Added point 4 above.]

[bombsimon]

Sorry for the late reply, been super busy today. Sounds good to put back as much as possible to checkNoGlobals, there's no real reason to move it to run so that's nice!

I understand the reasoning of having main in the root. I'm personally not a fan of that layout but I guess it's good to do as few changes as possible for this first pass.

1. If you read the code for the checker.Run() method I think it makes sense. Basically the reasoning is that exit code 1 would symbolise the linter not being able to run due to some coding issues such as compilation errors or syntax errors. Exit code 3 is used to symbolise that the application exited successfully but with a non zero exit status indicating errors. It's not to uncommon to use other exit codes than 1 to differentiate between the program failing vs. the program performing tasks failing. Like you said, I would leave this as is.
2. One could probably override and hide these options but they are a part of the analyser so I don't see the need to. By time as people are moving towards using the analysis package I think this will become more clear. I also think this is the right way to do it. If you want to you could override the usage flag and check for it in main before calling singlechecker.Main. I don't like the idea of manipulating this and I guess it might result in weird behaviour if you try to do the parsing of the flags twice.
3. Not sure how this would be solved but like you said I don't think this could cause many issues.
4. You could manipulate the slice from the os before having the analysis package parse the flags. I can push an example of that.

I think you're doing the right thing to minimise diff but I would also argue that if you are worried about breaking changes, just bump to a new version. You should be able to move forward freely with this linter and if it will become the source of golangci-lint instead of copying it it should be easy to maintain.

[leighmcculloch]

I like the work around you added modifying os.Args. That's neat and very simple. It's much simpler than what I was thinking, basically rewriting the main function instead of using the singlechecker. This is better though.

[leighmcculloch]

@bombsimon Thanks for making this change clean, straightforward, and easy to understand! And for working out the issues with running the tests.

[bombsimon]

Thanks for quick turnarounds, good discussions and the merge! :) I think this will help for future changes and make it easy to maintain golangci-lint.

[leighmcculloch]

Done!