Add constructorcheck linter

[reflechant]

constructorcheck reports places where you create values directly from a type ( T{} or new(T) ) ignoring a constructor NewT defined for this type in the same package as the type.

This is useful to avoid skipping initialization/validation for a type.

Types defined in the standard library are excluded from analysis.

The linter doesn't have any parameters at the moment.

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

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

• It must not be a duplicate of another linter or a rule of a linter (the team will help to verify that).
• It must have a valid license (AGPL is not allowed), and the file must contain the required information by the license, ex: author, year, etc.
• It must use Go version <= 1.22
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic().
• It must not contain log.Fatal(), os.Exit(), or similar.
• It must not modify the AST.
• It must not have false positives/negatives (the team will help to verify that).
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must have integration tests without configuration (default).
• They must have integration tests with configuration (if the linter has a configuration).

.golangci.next.reference.yml

• The file .golangci.next.reference.yml must be updated.
• The file .golangci.reference.yml must NOT be edited.
• The linter must be added to the lists 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 lintersdb/builder_linter.go and .golangci.next.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter uses goanalysis.LoadModeSyntax -> no WithLoadForGoAnalysis() in lintersdb/builder_linter.go
  • if the linter uses goanalysis.LoadModeTypesInfo, it requires WithLoadForGoAnalysis() in lintersdb/builder_linter.go
• 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 file jsonschema/golangci.next.jsonschema.json should be updated.
• The file jsonschema/golangci.jsonschema.json must NOT be edited.
• The linter repository should have a readme and linting.
• The linter should be published as a binary (useful to diagnose bug origins).
• The linter repository should have a .gitignore (IDE files, binaries, OS files, etc. should not be committed)
• A tag should never be recreated.
• use main as the default branch name.

---

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).

The reviews should be addressed as commits (no squash).

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.

[ldez]

It feels like the same thing as #4196

Also this code is a problem.

[reflechant]

Yeah, looks similar indeed.
There are 2 differences:

1. constructorcheck ignores types from the standard library (there are too many exceptions to the rule there like bytes.Buffer whose zero value is safe to use while there is also bytes.NewBuffer)
2. constructorcheck tells you the name of the constructor to use

[ldez]

1. constructorcheck ignores types from the standard library (there are too many exceptions to the rule there like bytes.Buffer whose zero value is safe to use while there is also bytes.NewBuffer)

This code is a major problem.

constructorcheck tells you the name of the constructor to use

It's a minor difference and I feel this will lead to wrong suggestions when there are several "constructors".

[reflechant]

BTW, is there a different way to get the list of standard library packages? This hack with calling go tool was the only way I could find.

[ldez]

This should never be called at runtime.

[reflechant]

It uses a very strict definition of what is a constructor - it's a function NewT, where T is the name of a type, existing in the same package, that returns only one value of the same type T. You can not have two of them in the same package because every function name must be unique within a package. I don't see a potential for a confusion here.

As for the standard library package list - I agree that I need to find a different way to do it.

[ldez]

I don't see a potential for a confusion here.

I can easily see it:

type Foo struct{}

func NewFoo(a string) *Foo           { return nil }
func NewFooWithOption(b string) *Foo { return nil }

type Bar struct{}

// Deprecated: use NewBarWithOption
func NewBar(a string) *Bar           { return nil }
func NewBarWithOption(b string) *Bar { return nil }

What will be the constructor suggested by your linter in those 2 cases?
Your linter will suggest NewFoo and NewBar.

In the first case, no constructors must be suggested.
In the second case, the NewBarWithOption must be suggested.

Like I already said this is a minor difference. But this difference will lead to false positives and this is something we want to avoid inside golangci-lint.

We can also talk about basic package-oriented constructors:

type Foo struct{}

func New() *Foo  { return nil }

It will lead to false negatives.

[reflechant]

You're the first person giving me good feedback :)
All valid points. I need to fix them. Should I close this PR until I do it?

[bombsimon]

... that returns only one value of the same type T

Maybe I misunderstood this part but I'd like to add that it's not uncommon for constructors to be fallible (and if so, sometimes such constructors might have a Must alternative)

type Foo struct {}

func NewFoo() (*Foo, error) {
    if err := setup(); err != nil {
        return nil, err
    }

    return &Foo{}, nil
}

func MustNewFoo() *Foo {
    foo, err := NewFoo()
    if err != nil {
        panic(err)
    }

    return foo
}

[reflechant]

Yes, also sometimes constructors return something like this:
(T, error) (*T, error), (T, bool) (*T, bool)

But I'm much more worried about being misleading and giving false error messages than missing something. The latter makes the linter not 100% efficient, the former makes it unusable.

[reflechant]

This code is a major problem.

Go itself does it all over the place.

I will try to dig how the go tool itself does it and see if it makes sense to copy the implementation from there.

UPD: fortunately it's quite simple: just packages.Load(cfg, "std")