Refactor to use go/analysis #25

zalenskivolt · 2023-04-08T00:19:31Z

a first go at using go/analysis as suggested in #24

zalenskivolt · 2023-04-08T11:29:23Z

nakedret.go

+				Pos:     s.Pos(),
+				End:     s.End(),
+				Message: fmt.Sprintf("naked return in func `%s` with %d lines of code", funName, fun.funcLength),
+				SuggestedFixes: []analysis.SuggestedFix{{


SuggestedFixes do not yet get picked up by golangci-lint, but it's planned,
golangci/golangci-lint#1779 (comment):

I think that it's the right thing for a linter to support SuggestedFixes (even if SuggestedFixes is still an experimental API), I also believe that we have to remove our custom implementation.

Update code to use package nakedret, to be possibly to import

zalenskivolt · 2023-04-08T21:50:53Z

cmd/nakedret/main.go

+func main() {
+	analyzer := nakedret.NakedReturnAnalyzer(DefaultLines)
+	singlechecker.Main(analyzer)
+}


moved main() to cmd/nakedret/main.go so nakedret code can use package nakedret and be imported

alexkohler · 2023-04-08T23:57:52Z

I'm seeing a build failure - I think we'd need to bump up the Go version here:

nakedret/.github/workflows/build-test.yml

Line 12 in c7b0313

go-version: 1.13.x

Mind bumping this up? Happy to merge after that

go mod tidy

zalenskivolt · 2023-04-09T13:05:43Z

Bumped to 1.18

zalenskivolt · 2023-04-09T14:04:32Z

Bumped to 1.18

The build had this error:

Error: ../../../go/pkg/mod/golang.org/x/[email protected]/go/gcexportdata/gcexportdata.go:97:9: undefined: io.ReadAll
note: module requires Go 1.18

but bumping to for example go1.16 also seems to work fine. Bumping to go1.17 adds the // indirect lines in go.mod, and bumping to go1.18 cleans up most of go.sum. I guess 1.18 is fine for most projects by now.

alexandear · 2023-04-10T09:53:02Z

nakedret.go

+	MaxLength uint
+}
+
+func (n *NakedReturnRunner) run(pass *analysis.Pass) (interface{}, error) {


We can use any instead of interface{} because of the Go 1.18.

updated to use any

alexandear · 2023-04-10T10:03:29Z

nakedret.go


+	stack []funcInfo


Maybe it's better to usefunctions as the slice's name? Because stack is unclear.

agree, also thought about this.

updated

alexandear · 2023-04-10T10:07:11Z

nakedret_test.go

+	}
+
+	testdata := filepath.Join(wd, "testdata")
+	log.Printf("Running tests on %s", testdata)


Maybe t.Log instead of log.Printf?

Btw., do we really need this log here?

removed, not really needed

Consider functions that end their definition on the same line as they start as one-liners, not zero-liners.

zalenskivolt · 2023-04-10T11:26:35Z

nakedret.go

+		if length == 0 {
+			// consider functions that finish on the same line as they start as single line functions, not zero lines!
+			length = 1


added check to handle one-liners

zalenskivolt · 2023-04-10T11:32:57Z

README.md

@@ -6,7 +6,7 @@ nakedret is a Go static analysis tool to find naked returns in functions greater
 Install Nakedret via go install:

 ```cmd
-go install github.com/alexkohler/nakedret@latest
+go install github.com/alexkohler/nakedret/cmd/nakedret@latest


From some local testing, it seems like the command-line tool now has to be installed with
go install github.com/alexkohler/nakedret/cmd/nakedret (with cmd/nakedret added)
but this is the same as for example goimports
go install golang.org/x/tools/cmd/goimports@latest

I'm not really sure what are best practices for things like this, it looks like many projects use both cmd/x and the root directory. goimports and golangci-lint both use cmd/x as well

From some local testing, it seems like the command-line tool now has to be installed with
go install github.com/alexkohler/nakedret/cmd/nakedret (with cmd/nakedret added)

Hm, would this break folks that are running things like go install github.com/alexkohler/nakedret@latest in CI? (not saying that's the best practice ever 😅)

I'm not really sure what are best practices for things like this, it looks like many projects use both cmd/x and the root directory. goimports and golangci-lint both use cmd/x as well

Seems like cmd/x might be the most idiomatic thing to do, but I don't really have strong feelings - I'd be fine with cmd/ or cmd/x 👍

Hm, would this break folks that are running things like go install github.com/alexkohler/nakedret@latest in CI? (not saying that's the best practice ever 😅)

that's true, but I guess then you would have the option to either use a specific version if you don't want to update anything, or update the reference?

maybe even update the README to refer to golangci-lint as the primary way to run it? it's just so much easier to all kinds of linting in one place… still good to be able to run each individual linter as a tool of course!

Yeah, I think most workflows should be using golangci-lint. I think this is a positive change that'll reduce some tech debt over in golangci-lint, so even if this ends up breaking some go install github.com/alexkohler/nakedret@latest calls, it's probably worth it IMO.

alexkohler · 2023-04-08T17:51:56Z

nakedret.go

+			//v.pass.Reportf(s.Pos(), "%v naked returns on %v line function", funName, fun.funcLength)
+			//v.pass.Reportf(s.Pos(), "naked return in func `%s` with %d lines of code", funName, fun.funcLength)


Were these intentionally left around?

they were the original message and the updated message before suggested fixes.

removed

alexkohler · 2023-04-10T13:25:22Z

README.md

@@ -6,7 +6,7 @@ nakedret is a Go static analysis tool to find naked returns in functions greater
 Install Nakedret via go install:

 ```cmd
-go install github.com/alexkohler/nakedret@latest
+go install github.com/alexkohler/nakedret/cmd/nakedret@latest


From some local testing, it seems like the command-line tool now has to be installed with
go install github.com/alexkohler/nakedret/cmd/nakedret (with cmd/nakedret added)

Hm, would this break folks that are running things like go install github.com/alexkohler/nakedret@latest in CI? (not saying that's the best practice ever 😅)

I'm not really sure what are best practices for things like this, it looks like many projects use both cmd/x and the root directory. goimports and golangci-lint both use cmd/x as well

Seems like cmd/x might be the most idiomatic thing to do, but I don't really have strong feelings - I'd be fine with cmd/ or cmd/x 👍

alexandear · 2023-04-10T14:42:57Z

We could also write in README.md that it's possible to run nakedret as go vet tool:

go vet -vettool=$(which nakedret) ./...

zalenskivolt · 2023-04-10T14:52:53Z

We could also write in README.md that it's possible to run nakedret as go vet tool:

I pushed a short sentence to the README.

zalenskivolt · 2023-04-10T19:38:21Z

Happy to merge after that

@alexkohler Feel feel to push any final updates if you're mostly happy! I enabled the allow edits by maintainers 😄

alexkohler · 2023-04-10T22:19:47Z

Thanks again for the PR 😄

zalenskivolt force-pushed the use-go-analysis branch 2 times, most recently from d017ec7 to df6fe66 Compare April 8, 2023 00:39

Refactor to use go/analysis

6cd5a2f

zalenskivolt force-pushed the use-go-analysis branch from df6fe66 to 1803bcf Compare April 8, 2023 00:41

zalenskivolt mentioned this pull request Apr 8, 2023

Refactor to use go/analysis #24

Closed

zalenskivolt and others added 2 commits April 8, 2023 03:02

Add suggested fixes

51187a9

Add example from alexkohler#8

ca4c318

zalenskivolt force-pushed the use-go-analysis branch from 1803bcf to ca4c318 Compare April 8, 2023 01:02

zalenskivolt commented Apr 8, 2023

View reviewed changes

Move main() to cmd/nakedret/main.go

8872ec2

Update code to use package nakedret, to be possibly to import

zalenskivolt commented Apr 8, 2023

View reviewed changes

zalenskivolt added 2 commits April 9, 2023 12:50

Update go version to 1.18

d13f192

go mod tidy

Build and test all subdirectories

21c6543

alexandear reviewed Apr 10, 2023

View reviewed changes

zalenskivolt added 3 commits April 10, 2023 12:59

Rename nested function stack to functions, use any

81779aa

Add test for top level function literal, less logs

d3741a8

Handle single line functions

39d92f1

Consider functions that end their definition on the same line as they start as one-liners, not zero-liners.

zalenskivolt commented Apr 10, 2023

View reviewed changes

Update go install line in README with cmd/nakedret

cb3a6c7

zalenskivolt commented Apr 10, 2023

View reviewed changes

zalenskivolt requested a review from alexandear April 10, 2023 11:33

alexkohler reviewed Apr 10, 2023

View reviewed changes

Remove old analysis.Pass.Reportf calls

ba2e2a3

Add go vet command to usage in README

9ee1fbc

alexkohler merged commit 27ab208 into alexkohler:master Apr 10, 2023

alexkohler mentioned this pull request Apr 10, 2023

Add a flag as an option to fix naked returns #8

Closed

zalenskivolt mentioned this pull request Apr 11, 2023

build(deps): bump github.com/alexkohler/nakedret to 2.0.1 golangci/golangci-lint#3760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to use go/analysis #25

Refactor to use go/analysis #25

zalenskivolt commented Apr 8, 2023

zalenskivolt Apr 8, 2023

zalenskivolt Apr 8, 2023

alexkohler commented Apr 8, 2023

zalenskivolt commented Apr 9, 2023

zalenskivolt commented Apr 9, 2023

alexandear Apr 10, 2023

zalenskivolt Apr 10, 2023

alexandear Apr 10, 2023

zalenskivolt Apr 10, 2023

alexandear Apr 10, 2023

zalenskivolt Apr 10, 2023

zalenskivolt Apr 10, 2023

zalenskivolt Apr 10, 2023

alexkohler Apr 10, 2023

zalenskivolt Apr 10, 2023

alexkohler Apr 10, 2023

alexkohler Apr 8, 2023

zalenskivolt Apr 10, 2023

alexkohler Apr 10, 2023

alexandear commented Apr 10, 2023

zalenskivolt commented Apr 10, 2023

zalenskivolt commented Apr 10, 2023

alexkohler commented Apr 10, 2023

		//v.pass.Reportf(s.Pos(), "%v naked returns on %v line function", funName, fun.funcLength)
		//v.pass.Reportf(s.Pos(), "naked return in func `%s` with %d lines of code", funName, fun.funcLength)

Refactor to use go/analysis #25

Refactor to use go/analysis #25

Conversation

zalenskivolt commented Apr 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkohler commented Apr 8, 2023

zalenskivolt commented Apr 9, 2023

zalenskivolt commented Apr 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandear commented Apr 10, 2023

zalenskivolt commented Apr 10, 2023

zalenskivolt commented Apr 10, 2023

alexkohler commented Apr 10, 2023