Fix warnings, possible linter failures and typos #223
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -23,7 +23,7 @@ import ( | |||
| ) | |||
|
|
|||
| func init() { | |||
| caddy.RegisterModule(App{}) | |||
| caddy.RegisterModule(&App{}) | |||
There was a problem hiding this comment.
Registering a module doesn't require an allocation, it just needs an example value.
(Same with other module(s) below)
There was a problem hiding this comment.
As for the value/pointer receiver warnings, my approach is as follows. We do need a pointer receiver in Provision functions, so that fields could be updated in a struct. We don’t have functions where a value receiver is required, so that no field would be updated unintentionally. Golang allows structs to have both value and pointer receivers, but it’s recommended to avoid mixing them for a single struct - that’s what the warning basically says. Thus, my suggestion is to use pointer receivers everywhere unless value receivers are strictly required.
There was a problem hiding this comment.
But the module registration is never the same instance being used for provisioning. Those are separate. I'm not sure what linter you're using, but it's definitely overly aggressive on that. I don't think I agree with that rule.
There was a problem hiding this comment.
What I’m talking about is written in the golang official docs. E.g. see https://go.dev/tour/methods/8, paragraph 5. It’s a general rule to have either value or pointer receivers for all struct functions, but not both.
There was a problem hiding this comment.
The module is now registered here with & just because CaddyModule function has a pointer receiver instead of a value receiver it used to have. My tests show this way of module registration works fine.
There was a problem hiding this comment.
That's interesting... and yeah, I mean, it'll work... it's just unusual in terms of Caddy convention.
(I think our JSON docs generator handles pointer values here correctly when it does its static analysis.)
Typically what I do is determine whether a pointer or value receiver is the correct/conservative thing to use, and choose that. If the value isn't used, or if a copy is permissible (no values from the sync package, for example, in the struct), I prefer a value receiver, I think this avoids an allocation? I'm not sure. But it also makes nil pointer derefs impossible which is nice.
Anyway... I dunno. Still feels weird to use a pointer when it's not needed.
There was a problem hiding this comment.
So we have basically 2 options here:
- revert the "mixed value and pointer receivers" fix and keep both types of receivers for various structs of caddy-l4;
- accept this fix and use only pointer receivers because we must have a pointer receiver in Provision functions.
If there is more support for the first option, I will adjust this PR.
There was a problem hiding this comment.
So after asking teh Twitter what they do, the results are really mixed. Many use only pointer receivers (consistency, avoids bugs). A good number only use pointers if that is most appropriate (self-documenting code, mutable structs, etc).
I am fine with the change as-is I guess :) Thanks!
| @@ -40,7 +40,7 @@ type App struct { | |||
| } | |||
|
|
|||
| // CaddyModule returns the Caddy module information. | |||
| func (App) CaddyModule() caddy.ModuleInfo { | |||
| func (*App) CaddyModule() caddy.ModuleInfo { | |||
There was a problem hiding this comment.
Same as above, this doesn't need to be a pointer. Maybe it doesn't matter. I don't think I'm used to using pointers for the Module registration stuff. 🤷♂️
| @@ -33,7 +33,7 @@ type ListenerWrapper struct { | |||
| } | |||
|
|
|||
| // CaddyModule returns the Caddy module information. | |||
| func (ListenerWrapper) CaddyModule() caddy.ModuleInfo { | |||
| func (*ListenerWrapper) CaddyModule() caddy.ModuleInfo { | |||
There was a problem hiding this comment.
I haven't finished my review yet but does this actually solve a problem? I guess I don't care either way in the cases where this isn't even used (there's not even a variable named for it) but I guess I just am curious.
This is a set of changes fixing code warnings that don't prevent compilation. Besides, I've sorted imports to prevent linter failures like those happened to my PRs in Caddy mainline. Finally, a few grammar mistakes and typos have been fixed.
After these changes only 6 go warnings remain: 1 defer called in a for loop in layer4/connection.go, 1 redundant constant in modules/l4tls/parsehello.go, and 4 deprecation warnings in modules/l4proxy/upstream.go.
The most dangerous change I believe is a fix for the mixed value and pointer receivers warning. I've checked that all tests run flawlessly with pointer receivers, but I didn't analyse if there were any places in the code where value receivers were the only possible way to go.