build(deps): bump github.com/junk1tm/musttag from 0.4.5 to 0.5.0

[tmzane]

go-simpler/musttag@v0.4.5...v0.5.0

[ldez]

@junk1tm Windows is not a friend 😄

I recommend adding Windows and macOS to your CI.

[ldez]

It's a bit weird to have a path inside a report message (ex: at test/testdata/musttag.go:15:2) because the path is already inside the report itself.

Can you remove it from your linter?

[tmzane]

It's actually the path of the function call to which the reported type is passed, not the same path as in the report itself, e.g.

10 var user User
11 json.Marshal(user) // <- this one

`User` should be annotated with the `json` tag as it is passed to `json.Marshal` at <location>:11

I found it quite useful because it helps to understand why the type was reported in the first place. And sometimes the call is located far away, may be in a different package too.

WDYT?

[tmzane]

I believe we can fix the Windows tests using build tags 🤔

[ldez]

I believe we can fix the Windows tests using build tags 🤔

I think a regexp can do the job instead of build tags

[ldez]

It's actually the path of the function call to which the reported type is passed, not the same path as in the report itself

I understand, and this can be useful, but the info is not as accurate as one might expect:

package main

import (
	"encoding/json"
)

type User struct {
	Firstname string
	Lastname  string
}

func main() {
	var user User

	foo(user)
	bar(user)

	_, _ = json.Marshal(user)
	_, _ = json.Marshal(user)
}

func foo(user User) {
	_, _ = json.Marshal(user)
}
func bar(user User) {
	_, _ = json.Marshal(user)
}

$ ./golangci-lint run -Emusttag
foo.go:7:6: `User` should be annotated with the `json` tag as it is passed to `json.Marshal` at foo.go:18:9 (musttag)
type User struct {
     ^

The more I think the more I wonder if the position of the reports shouldn't be the calls to Marshall instead of the struct 🤔

[ldez]

I fixed the problem with the vulnerability scanner in the master branch, can you rebase?

[ldez]

WDYT about #3624 (comment) ?

[tmzane]

I think a regexp can do the job instead of build tags

You're right, sometimes I forget that these comments are actually regexp 😅

but the info is not as accurate as one might expect

Could you elaborate on why the info is not accurate? It contains the function name and the line number, not sure if there is anything to add.

The more I think the more I wonder if the position of the reports shouldn't be the calls to Marshall instead of the struct 🤔

I had the same thoughts but in the end I decided that the primary report location should be the error itself: it's the type declaration that should be fixed (by annotating with tags), not the corresponding function call.

[tmzane]

Maybe we should leave it in the current state for now and wait for more feedback?

[ldez]

Could you elaborate on why the info is not accurate?

In my example there 4 calls to Marshall for the same struct and the report contains only the first call.

[ldez]

I had the same thoughts but in the end I decided that the primary report location should be the error itself: it's the type declaration that should be fixed (by annotating with tags), not the corresponding function call.

The issue about reporting struct outside of the module leads me to think that knowing that a struct will be marshaled without the expected tag can also be a piece of important information.

I'm just thinking. There are no blockers here, just some thoughts.

[tmzane]

In my example there 4 calls to Marshall for the same struct and the report contains only the first call.

Currently it's the result of golangci-lint's report deduplication: if you lint the same code with musttag standalone, it will report all the calls. We could collect multiple reports for the same struct and then report it once with all the locations, but it would only work per package, so still not perfect.

I appreciate your thoughts and ideas, helps a lot 👍

[ldez]

if you lint the same code with musttag standalone, it will report all the calls.

The linter reports the struct but it produces multiple reports related to Marshall calls. 🤔
This enforces my throughs about reporting Marshall instead of the struct.

[ldez]

LGTM but we can continue to discuss.