Consider adding goccy/go-json to whitelist

[ccoVeille]

I looked on GitHub about the packages commonly excluded from musttag

https://github.com/search?q=path%3A%2F.golangci*%20%22musttag%3A%22%20%22functions%22%20-path%3Areference%20&type=code

I found that https://github.com/goccy/go-json is frequently excluded from musttag

musttag:
    functions:
      - name: github.com/goccy/go-json.Marshal
        tag: json
      - name: github.com/goccy/go-json.MarshalContext
        tag: json
      - name: github.com/goccy/go-json.Unmarshal
        tag: json
      - name: github.com/goccy/go-json.UnmarshalContext
        tag: json

Source: https://github.com/heffcodex/goutil/blob/e5bab38739f47b7b9769fb1172543c59f50d331d/.golangci.yml#L140

The lib is very popular:

cc @goccy, hi 👋, great lib BTW

[ccoVeille]

I also found https://github.com/json-iterator/go with that search BTW

musttag:
    functions:
      # The full name of the function, including the package.
      - name: github.com/json-iterator/go.Marshal
        # The struct tag whose presence should be ensured.
        tag: json
        # The position of the argument to check.
        arg-pos: 0

Source: https://github.com/releaseband/go-progress-bar/blob/7f3fe8277abe1c6c411c38350071a08faf672b82/.golangci.yaml#L211

maybe musttag could whitelist it too.

But the project is almost unmaintained, but widely used:

[tmzane]

I actually regret a bit about enabling all the builtin checks by default. It'd probably be better to enable by default only the stdlib ones (encoding/json and encoding/xml) with the option to enable additional checks for 3rd party packages in the config. Then we could add support for any 3rd package, such as the one you suggested.

We could probably make it work this way, though we would need to consult the golangci-lint maintainers on whether this would be considered a breaking change.

[ccoVeille]

It makes sense, yes.

[tmzane]

Hey @ldez, wanted to ask for your opinion as the maintainer of golangci-lint. Would it be ok to change the list of packages musttag checks by default? For example, to make it check only encoding/json and encoding/xml functions by default, and add an option to enable supported 3rd party packages (e.g. by introducing the packages: []string option).

Currently, musttag checks all the supported packages by default (the full list is in the README), which may become a problem as we add support for more 3rd party packages out-of-the-box. Some users may care about only a few of them, or even want to disable some, e.g. being fine with the absence of db tags for sqlx. Disabling checks for 3rd party packages by default would speed up the linting process by skipping unwanted checks, and would also allow us to introduce support for more packages, e.g. the ones suggested by @ccoVeille.

WDYT?

[ldez]

:thinking:

First, I want to warn about the data and the conclusion: the number obtained through SourceGraph considered indirect dependencies and forks.

For example, if I use more precise search queries:

• query1: only 4 projects (by mainly only one user) has defined a configuration for github.com/goccy/go-json
• query2: only 1 projects has defined a configuration for github.com/json-iterator/go.

And if you use those queries (A, B) you will see that most of the references a related to indirect dependencies.

So I don't share @ccoVeille conclusions about the need to add those elements inside musttag "by default".

Now, about the change of the configuration, this is not a problem to create breaking changes inside your linter if inside golangci-lint we can create a compatibility layer. So I need more information about your changes proposal to evaluate the impact.

Also, I think the performance gain will be marginal.

[ccoVeille]

:thinking:

First, I want to warn about the data and the conclusion: the number obtained through SourceGraph considered indirect dependencies and forks.

For example, if I use more precise search queries:

• query1: only 4 projects as defined a configuration for github.com/goccy/go-json
• query2: only 1 projects as defined a configuration for github.com/json-iterator/go.

And if you use those queries (A, B) you will see that most of the references a related to indirect dependencies.

I wasn't aware of this. Thanks.

So I don't share @ccoVeille conclusions about the need to add those elements inside musttag "by default".

Indeed, then

Now, about the change of the configuration, this is not a problem to create breaking changes if inside golangci-lint we can create a compatibility layer. So I need more information about your changes proposal to evaluate the impact.

Also, I think the performance gain will be marginal.

👍

[tmzane]

That's exactly my motivation: there are some (not widely used) packages that would be nice to support, but I don't want to enable them by default. So I'm thinking about making supported 3rd party packages opt-in.

So I need more information about your changes proposal to evaluate the impact.

The change would be to disable the following packages by default:

• gopkg.in/yaml.v3
• github.com/BurntSushi/toml
• github.com/mitchellh/mapstructure
• github.com/jmoiron/sqlx

And to introduce an option to enable them (and more packages we could support in the future).

This would be a change in the current behavior, but I'm not sure it would be "breaking" 🤔

[ldez]

I think the current default is right. Those libraries are widely used.

Maybe I missed something, but I didn't see the benefits of this change (expect a potential marginal gain).

Do you want to add 2 options (std / 3rd) or multiple options (std / yaml / toml / ...)?

If it's 2 options, why do think this is better? Same thing for multiple options, why do think this is better?

[tmzane]

Maybe I missed something, but I didn't see the benefits of this change

I'd like to be able to support more 3rd party packages without enabling them by default. Enabling individual packages would be more flexible approach, e.g. there are many json packages in the Go ecosystem, but usually only a single one is used in a project. So it'd be nice to allow users to enable checks only for the one they use.

I think the current default is right. Those libraries are widely used.

Well, we could just leave the current packages enabled by default and make only new ones opt-in. But I don't think that "widely used" is a good metric because it's too subjective. I think "std / 3rd party" is better.

Do you want to add 2 options (std / 3rd) or multiple options (std / yaml / toml / ...)?

I'm thinking about something like this:

linters-settings:
  musttag:
    packages:
      # (std packages are enabled by default)
      - gopkg.in/yaml.v3
      - github.com/BurntSushi/toml
      - ...

[tmzane]

Anyway, that's just my thoughts for now, I'm not 100% sure we need this. I will sleep on it, and we can revisit in the future. Thanks for the opinions