Add `gofactory` linter #4196

maranqz · 2023-11-12T19:57:55Z

The linter checks that the structures are created by the factory, and not directly.
The checking helps to provide invariants without exclusion and helps avoid creating an invalid object.

related to #2708

boring-cyborg · 2023-11-12T19:57:58Z

Hey, thank you for opening your first Pull Request !

CLAassistant · 2023-11-12T19:58:01Z

All committers have signed the CLA.

ldez · 2023-11-12T20:21:44Z

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

The CLA must be signed

Pull Request Description

It must have a link to the linter repository.
It must provide a short description of the linter.

Linter

The Linter Tests Inside Golangci-lint

They must have at least one std lib import.
They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

`.golangci.reference.yml`

The linter must be added to the list of available linters (alphabetical case-insensitive order).
- enable and disable options
If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
- The values must be different from the default ones.
- The default values must be defined in a comment.
- The option must have a short description.

Others Requirements

The files (tests and linter) inside golangci-lint must have the same name as the linter.
The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
The load mode (WithLoadMode(...)):
- if the linter doesn't use types: goanalysis.LoadModeSyntax
- goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.
WithURL() must contain the URL of the repository.

Recommendations

The linter should not use SSA. (SSA can be a problem with generics)
The linter repository should have a readme and linting.
The linter should be published as a binary. (useful to diagnose bug origins)

The golangci-lint team will edit this comment to check the boxes before and during the review.

The code review will start after the completion of those checkboxes (except for the specific items that the team will help to verify).

If the author of the PR is a member of the golangci-lint team, he should not edit this message.

This checklist does not imply that we will accept the linter.

go.mod

Antonboom · 2023-11-14T20:55:20Z

Maybe to combine with check from #3488?

maranqz · 2023-11-15T06:30:10Z

Maybe to combine with check from #3488?

I don't want to combine constructor and gofactory because the DDD factory can be a function or separate struct method, not only a constructor.
Also, I think that IssueLoan is more descriptive than NewLoan.

2. Added tests 3. Added config 4. Added linter

pkg/lint/lintersdb/manager.go

maranqz · 2023-11-15T07:26:10Z

Is there anything else that I can do to get the PR be approved and merged?

Antonboom

Thanks for the linter!

In addition to review comments:

TestdataDir() -> analysistest.TestData()
Case like

type OtherStruct struct {}

type Struct struct {
	Other OtherStruct 
}

func NewStruct() Struct {
	return Struct{
		Other: OtherStruct{}, // warning?
	}
}

Type assertion, type declaration and type underlying, tests <- Broken link

Struct with type params?

type Struct[T any] struct {  // type params could be many (this is different ast node)
}

_ = Struct[T]{} // Warning

Import alias (and affect on report warning?).
You covered slice, but not map, channel, func call, etc. places where obj could be constructed.

pkg/config/linters_settings.go

test/testdata/configs/gofactory.yml

test/testdata/gofactory/app.go

2. Added test with options

go.mod

# golangci/golangci-lint#4196 1. TestdataDir() -> analysistest.TestData() 2. Described False-Negative cases as testcases and in README.md 3. Fixed link for type_nested.go.skip in README.md 4. Added testcase with generic 5. Added testcase with alias 6. Added testcase with map, chan, array, func # golangci/golangci-lint#4196 7. Added glob syntax for pkgs 8. Renamed options 9. Extended unexpected code message 10. Added unimplemented testcases

maranqz · 2023-11-19T08:40:48Z

Could I resolve the case on Winter Weekends, not now?)
I added unimplemented testcases for the case and also for chan, var declaration, named returned.

Case like

type OtherStruct struct {}

type Struct struct {
	Other OtherStruct 
}

func NewStruct() Struct {
	return Struct{
		Other: OtherStruct{}, // warning?
	}
}

ldez · 2023-11-20T19:27:57Z

Just a reminder for the maintainers: before reviewing the code, the checklist should be handled.

ldez · 2023-11-20T19:37:13Z

The use of the tests dedicated to linters that need dependencies is not the right way.
The tests should handled as simple linters.

I'm not sure about the pertinence of this linter because Go doesn't require constructors, and data structure is a good pattern, I think this will lead to a lot of false positives.

maranqz · 2023-11-21T05:09:48Z

The use of the tests dedicated to linters that need dependencies is not the right way.
The tests should handled as simple linters.

Do you mean tests should be like this and remove subdirectories?

package gofactory

import "net/http"

func local() {
   _ = http.Request{} // want...
  _, _ = http.NewRequest("GET", "http://example.com", nil)
}

maranqz · 2023-11-21T05:19:47Z

I'm not sure about the pertinence of this linter because Go doesn't require constructors, and data structure is a good pattern, I think this will lead to a lot of false positives.

My motivation was to create the linter for the domain layer with business logic in the factories, which cannot be skipped and where any structures should always be valid.

I agree that using the go-factory-lint in any place is not a good idea, so I added options packageGlobs and onlyPackageGlobs to set domain packages to use the linter only for them.

Should I force set onlyPackageGlobs as true by default and ask to set packageGlobs to run linter?

go.mod

2. Removed nested directory in test

maranqz · 2023-11-26T10:05:46Z

The use of the tests dedicated to linters that need dependencies is not the right way.
The tests should handled as simple linters.

I removed subdirectories in tests. I'm not sure what you mean.
If not, then you mean that I should remove the use of the WithLoadForGoAnasis option.

I can't because the linter needs this to determine whether it is a function or a type when we cast types like this.

package casting

import (
	"factory/unimplemented/casting/nested"
)

func ToNestedMyInt() {
	_ = nested.MyInt(1) // want `Use factory for nested.Struct`
}

pkg/lint/lintersdb/manager.go

2. renamed options 3. changed linter version

maranqz · 2023-12-15T17:50:44Z

test/run_test.go

@@ -137,7 +137,7 @@ func TestCgoOk(t *testing.T) {
 			"--timeout=3m",
 			"--enable-all",
 			"-D",
-			"nosnakecase,gci",
+			"nosnakecase,gci,gofactory",


I hope that is correct solution to pass tests.

your tests are not in the right places.

Do you mean I should move test/testdata/gofactory/blocked.go to test/testdata/gofactory/gofactory_package_globs_only.go?

no, your tests must be inside test/testdata/ and not a dedicated folder.
and your linter should be removed from test/linters_test.go.

#4196 (comment)

pkg/lint/lintersdb/manager.go

Antonboom · 2024-01-07T07:05:41Z

test/run_test.go

@@ -355,7 +355,6 @@ func TestLineDirectiveProcessedFiles(t *testing.T) {
 func TestUnsafeOk(t *testing.T) {
 	testshared.NewRunnerBuilder(t).
 		WithNoConfig().
-		WithArgs("--enable-all").


how does this change relate to PR?

If I return, can I add -D gofactory to fix linter error Use factory for unsafe.Pointer.
Or is solution from comment is better?
#4196 (comment)

this should be reverted.

Antonboom · 2024-01-07T07:08:44Z

test/run_test.go

@@ -137,7 +137,7 @@ func TestCgoOk(t *testing.T) {
 			"--timeout=3m",
 			"--enable-all",
 			"-D",
-			"nosnakecase,gci",
+			"nosnakecase,gci,gofactory",


need to rollback

I added because of linter error Use factory for unsafe.Pointer.

Should I add option to exclude checking of std libraries in gofactory?

Should I add option to exclude checking of std libraries in gofactory?

Nice idea, but I think in case of exclude-std=false we will receive false positive anyway, because a lot of std types could be (or must be) used without factory by design. Like unsafe.Pointer or sync.Mutex.

Zero value is useful 🙂

Look at

$ cd $GOROOT/src $ gofactory -json ./... | jq '.[] | .[] | .[] .message' | sort | uniq

Antonboom · 2024-01-07T07:13:52Z

test/testdata/gofactory.go

+func call(_ http.Request) {}
+
+func alias() {
+	_ = alias_blocked.Request{} // want `Use factory for http.Request`


cosmetic question: why not Use factory for alias_blocked.Request?
at fist time I was confused, but cannot decide what option is better 🤔

Idk too)
Could you decide how to better based on your experience?

I checked wrapcheck, go-reassign. They both used different printing way for package names.
I don't find another linter which uses package names.

wrapcheck: error returned from external package is unwrapped: sig: func encoding/json.Marshal(v any) ([]byte, error)
go-reassign: reassigning variable EOF in other package alias

IMHO, the option that is better for grep and common understanding – to keep the source code and warnings for it consistent:

_ = neturl.URL{} // want `Use factory for neturl.URL` _ = &url.URL{} // want `Use factory for url.URL`

maranqz force-pushed the go-factory-lint branch from 7799b5c to 14c0aba Compare November 12, 2023 19:58

maranqz changed the title ~~Addgofactory linter~~ Add gofactory linter Nov 12, 2023

maranqz changed the title ~~Add gofactory linter~~ Addgofactory linter Nov 12, 2023

maranqz changed the title ~~Addgofactory linter~~ Add gofactory linter Nov 12, 2023

ldez added the linter: new Support new linter label Nov 12, 2023

alexandear reviewed Nov 13, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

ldez added the feedback required Requires additional feedback label Nov 14, 2023

maranqz added 2 commits November 15, 2023 10:46

1. Added reference

bbe0eb9

2. Added tests 3. Added config 4. Added linter

1. Update version after type fixes

70cd3e5

maranqz force-pushed the go-factory-lint branch from 14c0aba to 70cd3e5 Compare November 15, 2023 06:53

maranqz commented Nov 15, 2023

View reviewed changes

pkg/lint/lintersdb/manager.go Outdated Show resolved Hide resolved

1. Added tests in subdir

5a86fb8

maranqz force-pushed the go-factory-lint branch from bf7fb06 to 5a86fb8 Compare November 15, 2023 07:26

maranqz requested review from ldez and alexandear November 15, 2023 07:27

1. Bump golangci-lint version

2a31b53

Antonboom requested changes Nov 15, 2023

View reviewed changes

maranqz mentioned this pull request Nov 15, 2023

Prepare linter for golangci-lint maranqz/gofactory#5

Merged

maranqz added 2 commits November 15, 2023 23:13

1. Fixed settings name

22e29b7

2. Added test with options

1. Fixed test with config file

faecc6b

maranqz commented Nov 15, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

1. Update config after linter fixing

2fde9ce

maranqz requested a review from Antonboom November 19, 2023 08:38

maranqz added 2 commits November 20, 2023 20:09

1. Remove slog to pass go1.20 test

bb4065b

Merge branch 'master' into go-factory-lint

419bf43

maranqz commented Nov 21, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

maranqz added 2 commits November 26, 2023 12:29

Merge branch 'master' into go-factory-lint

e40eddf

1. Added casting function

930e2b2

2. Removed nested directory in test

Antonboom reviewed Dec 7, 2023

View reviewed changes

pkg/lint/lintersdb/manager.go Show resolved Hide resolved

Antonboom mentioned this pull request Dec 7, 2023

Cosmetic fixes maranqz/gofactory#7

Merged

maranqz added 3 commits December 15, 2023 07:57

1. renamed linter

c1195cf

2. renamed options 3. changed linter version

Merge branch 'master' into go-factory-lint

ba1ed19

1. fixed tests

2b59b9d

maranqz commented Dec 15, 2023

View reviewed changes

maranqz added 4 commits December 17, 2023 13:40

1. moved gofactory test from dedicated to common directory

eb43dd5

Merge branch 'master' into go-factory-lint

df588bc

Merge branch 'master' into go-factory-lint

a1dbfe1

Merge branch 'master' into go-factory-lint

985e045

maranqz requested a review from Antonboom December 21, 2023 18:21

Merge branch 'master' into go-factory-lint

111f638

Antonboom requested changes Jan 7, 2024

View reviewed changes

golangci deleted a comment from anfaas1618 Mar 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `gofactory` linter #4196

Add `gofactory` linter #4196

maranqz commented Nov 12, 2023 •

edited by ldez

boring-cyborg bot commented Nov 12, 2023

CLAassistant commented Nov 12, 2023 •

edited

ldez commented Nov 12, 2023 •

edited

Antonboom commented Nov 14, 2023

maranqz commented Nov 15, 2023

maranqz commented Nov 15, 2023

Antonboom left a comment •

edited

maranqz commented Nov 19, 2023 •

edited

ldez commented Nov 20, 2023

ldez commented Nov 20, 2023 •

edited

maranqz commented Nov 21, 2023 •

edited

maranqz commented Nov 21, 2023

maranqz commented Nov 26, 2023 •

edited by ldez

maranqz Dec 15, 2023

ldez Dec 15, 2023

maranqz Dec 15, 2023

ldez Dec 15, 2023 •

edited

Antonboom Jan 7, 2024

maranqz Jan 7, 2024

ldez Jan 7, 2024

Antonboom Jan 7, 2024

maranqz Jan 7, 2024

Antonboom Jan 8, 2024

Antonboom Jan 7, 2024

maranqz Jan 7, 2024

Antonboom Jan 8, 2024

Add gofactory linter #4196

Are you sure you want to change the base?

Add gofactory linter #4196

Conversation

maranqz commented Nov 12, 2023 • edited by ldez

boring-cyborg bot commented Nov 12, 2023

CLAassistant commented Nov 12, 2023 • edited

ldez commented Nov 12, 2023 • edited

Pull Request Description

Linter

The Linter Tests Inside Golangci-lint

.golangci.reference.yml

Others Requirements

Recommendations

Antonboom commented Nov 14, 2023

maranqz commented Nov 15, 2023

maranqz commented Nov 15, 2023

Antonboom left a comment • edited

Choose a reason for hiding this comment

maranqz commented Nov 19, 2023 • edited

ldez commented Nov 20, 2023

ldez commented Nov 20, 2023 • edited

maranqz commented Nov 21, 2023 • edited

maranqz commented Nov 21, 2023

maranqz commented Nov 26, 2023 • edited by ldez

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez Dec 15, 2023 • edited

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

Add `gofactory` linter #4196

Add `gofactory` linter #4196

maranqz commented Nov 12, 2023 •

edited by ldez

CLAassistant commented Nov 12, 2023 •

edited

ldez commented Nov 12, 2023 •

edited

`.golangci.reference.yml`

Antonboom left a comment •

edited

maranqz commented Nov 19, 2023 •

edited

ldez commented Nov 20, 2023 •

edited

maranqz commented Nov 21, 2023 •

edited

maranqz commented Nov 26, 2023 •

edited by ldez

ldez Dec 15, 2023 •

edited