fix: anonymous/inaccessible structs for commentstart #40
Conversation
| @@ -32,3 +46,13 @@ type CommentStartTestStruct struct { | |||
|
|
|||
| // DoNothing is used to check that the analyser doesn't report on methods. | |||
| func (CommentStartTestStruct) DoNothing() {} | |||
|
|
|||
| type unexpectedStruct struct { | |||
There was a problem hiding this comment.
Sorry.
That's right. Fixed at 3302613 !
tsuzu
force-pushed
the
commentstart-anonymous-structs
branch
from
ffe4dd5 to
3302613
Compare
There was a problem hiding this comment.
I think this is a good start to solving these issues. I am wondering if, in the future, we might need to look at using similar logic in other linters too. I'm thinking perhaps we will need to create a util from what you've done here, but I think we can cross that bridge when we come to it rather than over-optimising now
Could you please review my comments and also rebase onto the latest main branch please
| if decl.Tok != token.TYPE { | ||
| return false |
There was a problem hiding this comment.
We ignore non-type declarations like var const import.
Let me add a comment here.
| tagInfo := jsonTags.FieldTags(field) | ||
| if tagInfo.Ignored { | ||
| return false |
There was a problem hiding this comment.
Can you add a comment here please
| return false | |
| // Returning false here means we won't inspect the children of an ignored field. | |
| return false |
| if field == nil || len(field.Names) == 0 { | ||
| return | ||
| return false |
There was a problem hiding this comment.
Can you explain why this one is false?
There was a problem hiding this comment.
In Go, we cannot declare anonymous structs in structs without field names, so we can safely ignore fields without names.
type Struct struct {
struct { // ./prog.go:9:2: syntax error: unexpected keyword struct, expected field name or embedded type
NoComment string `json:"noComment"`
} `json:",inline"` // ./prog.go:11:4: syntax error: unexpected literal `json:",inline"` after top level declaration
}https://go.dev/play/p/WSMdxhyTsa1
I also add a comment here.
There was a problem hiding this comment.
And I guess if you embed another struct, inlining it, then that struct has a name? Do we have a test case that represents this case?
type Foo struct {
Bar `json:",inline"
}
type Bar struct {
NoComment `json:"noComment"`
}
There was a problem hiding this comment.
Bar has type declarations itself, so it'll be checked separately from Foo.
But I'll add a test of it for future changes.
Update: Added
| @@ -34,3 +46,13 @@ type CommentStartTestStruct struct { | |||
|
|
|||
| // DoNothing is used to check that the analyser doesn't report on methods. | |||
| func (CommentStartTestStruct) DoNothing() {} | |||
|
|
|||
There was a problem hiding this comment.
If you were to include a definition like below, do you think your logic would catch it? I suspect it may need a small tweak in checking the stack right? WDYT?
type {
Foo struct {
...
}
}
There was a problem hiding this comment.
It seems that inspect.WithStack will traverse all structs even if we have multiple declarations in one type, so it'll be checked.
Do you think we need a test for this?
There was a problem hiding this comment.
Yeah, could you anyway please, as in the future we may change the way the linters traverse to look only at objects accessible from the kubebuilder root annotation and I'd like to make sure we don't regress. Better to capture the case now while we are thinking about it than forget about it and regress in the future
There was a problem hiding this comment.
I got it! Thanks!
Sorry. I haven't added this test yet.
Let me prepare a PR to just add it.
tsuzu
force-pushed
the
commentstart-anonymous-structs
branch
from
8272af4 to
941dbf6
Compare
tsuzu
force-pushed
the
commentstart-anonymous-structs
branch
from
941dbf6 to
32c2184
Compare
related: kubernetes-sigs/cluster-api#11870 (comment)
depends on #39
json:"-"