modifies-value-receiver: add support for immutable values

[powerman]

Is your feature request related to a problem? Please describe.
In some cases using immutable values can be a good style. For example, I often use type Config struct{...} this way, to have both (a variant of) dysfunctional options and ensure there are no references to config (or it values) outside of object using that config:

package main

import "maps"

type Config struct {
	b bool         // required
	i int          // optional
	m map[int]bool // optional
}

func NewConfig(b bool) Config {
	return Config{b: b, i: 42}
}

func (c Config) WithI(i int) Config {
	c.i = i
	return c
}

func (c Config) WithM(m map[int]bool) Config {
	c.m = maps.Clone(m) // clone referental values
	return c
}

type Thing struct {
	cfg Config // or *Config, both are safe choice here
}

func NewThing(cfg Config) *Thing {
	return &Thing{cfg: cfg}
}

This result in clean and safe code, but revive thinks it's bad:

config.go:16:2: modifies-value-receiver: suspicious assignment to a by-value method receiver (revive)
	c.i = i
	^
config.go:21:2: modifies-value-receiver: suspicious assignment to a by-value method receiver (revive)
	c.m = maps.Clone(m) // clone referental values
	^

Describe the solution you'd like
I'd like to make it possible to not trigger modifies-value-receiver rule in case method returns value of receiver type (or a pointer to receiver type). Examples:

func (v T) same(...) T {...}
func (v T) same_multi(...) (T, error) {...}
func (v T) sameref(...) *T {...}
func (v T) sameref_multi(...) (*T, error) {...}

The reason to support references in returned value is: both NewConfig() and WithX() methods may return *Config without actually changing whole pattern. This is because copy will anyway happens when it will be used as non-reference argument (as a receiver for WithX() or as an argument for NewThing(). Using reference may make sense, e.g. if Config is huge, or just to make NewConfig() looks more consistent (New usually means return reference).

I think it's safe to make modifies-value-receiver work this way by default, but if you unsure it can be configurable.

Describe alternatives you've considered
Adding //nolint:revive or disabling modifies-value-receiver rule. First is annoying, second is unsafe.

Additional context
N/A

[denisvmedia]

To me it looks like a design flaw in the given code.
It should either create an explicit copy or be a standalone function (i.e. not a struct member). The way how you do it is really confusing and indeed suspicious.

[powerman]

It should either create an explicit copy

It does that - by using non-pointer receiver.

or be a standalone function

Function will do the same (receive arg of non-pointer Config type and thus gets a copy), but it'll have longer name like ConfigWithI so you just get more stutter for no value.

I agree it may looks a bit unusual, maybe even confusing - but this will gone as soon as you get used to this style.

As for "design flaw" or "indeed suspicious" - I don't see how these apply. Can you explain what exactly the design flaw is and what kind of suspicions this code raises?

[denisvmedia]

Answering to all of your comments at once: this code style lacks readability and causes confusion. I'd avoid having it. It's still your and your teammates' choice, but I don't think we should change revive to support such code style.

[chavacava]

Hi @powerman, thank you for the proposal.
The pattern you mention (leveraging the object copy from the receiver to later returning it) is something I do not use but I've seen a lot. As you mention, revive warns on assignments on these copies and in some code bases it might generate a lot of false positives.
Identifying when the assignment must not be spotted requires verifying that at some point of the method the copy is returned. That means we will need to add some complexity to the rule. I'll study how hard can be to modify the rule. Optionally, we could study the possibility of reducing the confidence of the warning in some cases.

[powerman]

TBH I don't see much use for this pattern except for Config-like structures. But Config structures are wide spread, so IMO it's worth support. For Config structures it's important to have both required and optional args, and ensure no references to config will exists outside after calling a New constructor (to make sure object configuration can't be modified at any moment from outside).

[chavacava]

Here are some examples of the pattern being used to other things than configurations (notice the cases where the receiver copy is not returned but used, after modification, to build a struct of a different type)

kratos/identity/identity.go:334:2: suspicious assignment to a by-value method receiver
kratos/identity/identity.go:335:2: suspicious assignment to a by-value method receiver
kratos/identity/identity.go:379:2: suspicious assignment to a by-value method receiver
kratos/persistence/sql/devices/persister_devices.go:38:2: suspicious assignment to a by-value method receiver
kratos/persistence/sql/identity/persister_identity.go:78:2: suspicious assignment to a by-value method receiver
kratos/persistence/sql/persister.go:118:2: suspicious assignment to a by-value method receiver
kratos/persistence/sql/persister.go:122:3: suspicious assignment to a by-value method receiver
kratos/persistence/sql/persister.go:127:3: suspicious assignment to a by-value method receiver
kratos/session/session.go:283:2: suspicious assignment to a by-value method receiver
loki/pkg/storage/bloom/v1/bounds.go:153:4: suspicious assignment to a by-value method receiver
loki/pkg/storage/bloom/v1/builder.go:171:2: suspicious assignment to a by-value method receiver
kustomize/api/filters/fieldspec/fieldspec.go:52:2: suspicious assignment to a by-value method receiver
kustomize/api/filters/namespace/namespace.go:63:2: suspicious assignment to a by-value method receiver
kustomize/api/filters/namespace/namespace.go:71:3: suspicious assignment to a by-value method receiver
kustomize/api/filters/patchjson6902/patchjson6902.go:28:2: suspicious assignment to a by-value method receiver
kustomize/kyaml/fn/framework/parser/schema.go:70:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/fn/framework/parser/template.go:75:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/kio/pkgio_reader.go:226:2: suspicious assignment to a by-value method receiver
kustomize/kyaml/kio/pkgio_reader.go:228:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/kio/pkgio_reader.go:241:2: suspicious assignment to a by-value method receiver
kustomize/kyaml/kio/pkgio_writer.go:50:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/kio/pkgio_writer.go:68:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/kio/pkgio_writer.go:69:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/runfn/runfn.go:109:2: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/filters.go:105:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/fns.go:268:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/fns.go:271:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/fns.go:372:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/fns.go:513:2: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/fns.go:646:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/fns.go:701:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/fns.go:80:3: suspicious assignment to a by-value method receiver
kustomize/kyaml/yaml/walk/walk.go:57:2: suspicious assignment to a by-value method receiver

[powerman]

the receiver copy is not returned but used, after modification, to build a struct of a different type

While this is not an error by itself, I doubt this case worth supporting:

• It may be really hard to recognize correctly:
  • Modified value might be not used later "as is", it may be used as part of larger struct/slice/map/etc.
  • Methods which actually has a bug by not using pointer receiver also might use modified field after assign.
• It depends, but using some field of non-pointer receiver instead of new local variable may be a sign of bad code and worth a warning. (My use case differs because it uses a whole receiver as a new local variable and creating another one local variable for it will make code worse, not better.)

[chavacava]

Hi @powerman, I've drafted a modification of the rule to do not warn on cases where the intent is to modify the receiver in order to return it.

At the moment, the rule adds a (false positive?) legend at the end of the failure message on those cases where the receiver is modified and returned by the method. The idea is to remove these failures in the final version of the rule.

The branch with the modified version is here, feedback is welcome.

cc: @denisvmedia

[powerman]

The branch with the modified version is here, feedback is welcome.

Works for me.

[denisvmedia]

@chavacava maybe we can have a different confidence value for the "(false positive?)" case. WDYT?

[ccoVeille]

What about adding the struct name to the error reported when false-positive?

This way they could be excluded by using golangci-lint when a well identified struct could be ignored

https://golangci-lint.run/usage/false-positives/#exclude-or-skip

[powerman]

@denisvmedia @ccoVeille It looks like you both missed the "The idea is to remove these failures in the final version of the rule." part of @chavacava message. I read it this way: there will be no (false positive?) text in a message because rule won't trigger in such cases at all, so there is no need for extra filtering by golangci-lint rules, neither by severity nor by message text.

[ccoVeille]

It looks like you both missed the "The idea is to remove these failures in the final version of the rule." message.

Thanks for your reply @powerman

I was also unsure how do read the message. It was a bit ambiguous to me. I chose one version, you chose yours. I might be likely to have chosen the wrong one 😅

Let's assume it's your version:

The linter evolves in the way of no longer reporting what could be considered as false-positive.

Then I'm a bit worried there might be some rare legitimate use cases of someone altering a non-pointer receiver, while it should be a pointer one, so something that might be a bug won't be reported.

Something like, returning a struct from struct, but updating a counter in the struct.

It's uneasy to know if such use cases exists, because if such use cases happened in public code using revive, it would have been already reported, and fixed, I think.

If the changes goes I'm the way you said, these bugs would be never reported in someone code via the linting, so never fixed maybe.

Let's wait for @chavacava feedback.

[powerman]

Something like, returning a struct from struct, but updating a counter in the struct.

Can you show an example of such a code, please? To avoid some more misunderstanding. 😄

[ccoVeille]

Can you show an example of such a code, please? To avoid some more misunderstanding. 😄

Not really, as I said it's hypothetical, but something like this maybe

type Logger struct {
    // whatever
    Counter int
}

func (l Logger) Foo() Logger {
     l.Counter++
     return l
}

[ccoVeille]

Maybe @alingse could help us, by running go-linter-runner by running revive with this exact version 92c3209 and launch the modifies-value-receiver linter only with this dedicated version, we might discover things by reviewing the one reported as false-positive. The ones that are likely to get removed if they are hidden. And among the ones, I think we could find true-positive error reporting.

[powerman]

as I said it's hypothetical, but something like this maybe

Hypothetical is okay, but why do you think this code is wrong?

To me this example looks exactly like ones in the first comment:

• we're creating a new logger here based on existing one
• new logger differs from original logger - that's the point of creating a new logger and looks pretty valid

Any chance you can craft a (hypothetical, of course) example which looks like an obvious bug? At a glance such example probably should:

• implement optional method chaining pattern (to make it obvious we do not intent to create a new objects)
• somehow make it looks reasonable to use non-pointer receiver in most methods (keeping in mind chaining is optional in this design and implemented as an UX feature which might be useful in some use cases)
• then make a mistake by using non-pointer receiver in a method which should modify receiver

I can't say it's impossible to craft such an example, but chances are this code will looks ugly in one way or another and this unliness might be catched by some other linter.

BTW, I just notice current (unmodified) version of modifies-value-receiver rule does NOT catch rcvr.Field++ case. @chavacava Should I open a new issue about ++?

[ccoVeille]

I just notice current (unmodified) version of modifies-value-receiver rule does NOT catch rcvr.Field++ case. chavacava Should I open a new issue about ++?

Good catch, I think so

EDIT it was created

• modifies-value-receiver: miss modification by ++/-- #1109

[ccoVeille]

it's impossible to craft such an example, but chances are this code will looks ugly in one way or another and this unliness might be catched by some other linter.

Yes, maybe

Let's hope that what alingse launched would help to identity if it exists

• Run revive:92c3209 collect some false positive alingse/go-linter-runner-example#1

[chavacava]

Hi, sorry for the late response (I was offline the last 48h)
@powerman @ccoVeille
About failures with (false positive?): the interpretation by @powerman of my ambiguous phrase (my bad) is the correct one; i.e., these failures will not be reported. I've executed the rule against some big code-bases and in all cases (I've analyzed a big number of failures) these failures were actually false positives.

@denisvmedia about using different confidence level. It was something I've considered but a) I suspect almost nobody configures revive's confidence level, b) the failure itself says "Suspicious ..." thus the confidence is already not 100%. As I've said before, I propose to do not report all those failures that today are marked with (false positive?)

I think that the bigger difficulty with this rule is that in some cases we need to understand the intent of the function to decide if it is or not an acceptable modification of a copy of the receiver. For example, in the example given by @ccoVeille

type Logger struct {
    // whatever
    Counter int
}

func (l Logger) Foo() Logger {
     l.Counter++
     return l
}

I will say that it depends on the meaning of Foo: if instead of Foo the method is called IncrementCounter then I'll say that the assignment is suspicious, by the contrary if the method's name is GetIncrementedLogger (yes I know...) I'll say the assignment is okay.

Of course, revive is waaaaaaaaaaay far from being able to distinguish between these two cases. IMO the new version of the rule (that of the PR) is useful enough.

[ccoVeille]

@chavacava Thanks for your reply

You are right about the naming it's what makes the difference for the same code.
Something that would never be detected.

I'm glad you analyzed big codebase.

Let's say it's better to have less false-positive, even there are a very rare case of badly designed code that might contain a bug.

So I'm fine with the PR

[ccoVeille]

One question, what would be the behavior of the linter with a method that return a pointer

Something like

func (l Logger) Foo() *Logger {
    l.Counter += 1
    return &l
}

Same question with a method that return a signature with error, quite common

func (l Logger) Foo() (Logger, err) {
    l.Counter += 1
    return l, nil
}

[ccoVeille]

BTW what do you think about the repository to automate detection like the one you do manually now

• Run revive:92c3209 collect some false positive alingse/go-linter-runner-example#1
• Run revive:latest to compare to #1 alingse/go-linter-runner-example#2
• Run revive:92c3209 with "modifies-value-receiver linter" alingse/go-linter-runner#8

[ccoVeille]

For anyone interested, @powerman @chavacava

Here are the results via @alingse tool

alingse/go-linter-runner-example#1

[ccoVeille]

OK @chavacava @powerman

Here is a result of what is currently detected with revive 1.5.0, but that will no longer be reported with the version "false-positive?" one.

https://github.com/firecracker-microvm/firecracker-go-sdk/blob/main/jailer.go#L176-L198

// WithBin will set the specific bin path to the builder.
func (b JailerCommandBuilder) WithBin(bin string) JailerCommandBuilder {
        b.bin = bin
        return b
}

// WithID will set the specified id to the builder.
func (b JailerCommandBuilder) WithID(id string) JailerCommandBuilder {
        b.id = id
        return b
}

// WithUID will set the specified uid to the builder.
func (b JailerCommandBuilder) WithUID(uid int) JailerCommandBuilder {
        b.uid = uid
        return b
}

// WithGID will set the specified gid to the builder.
func (b JailerCommandBuilder) WithGID(gid int) JailerCommandBuilder {
        b.gid = gid
        return b
}

Here the code is creating a copy via the non-pointer receiver. It's a way to keep the struct immutable

Then we have to discuss if it's a problem or not. Previously, it was reported

Originally posted with @alingse help in alingse/go-linter-runner-example#1 (comment)

Run revive -config /home/runner/work/go-linter-runner-example/go-linter-runner-example/revive.toml on Repo: https://github.com/firecracker-microvm/firecracker-go-sdk

Got total 26 lines output in action: https://github.com/alingse/go-linter-runner-example/actions/runs/11799381973

Expand

1. drives.go#L43 suspicious assignment to a by-value method receiver (false positive?)
2. drives.go#L70 suspicious assignment to a by-value method receiver (false positive?)
3. command_builder.go#L50 suspicious assignment to a by-value method receiver (false positive?)
4. command_builder.go#L56 suspicious assignment to a by-value method receiver (false positive?)
5. command_builder.go#L72 suspicious assignment to a by-value method receiver (false positive?)
6. command_builder.go#L91 suspicious assignment to a by-value method receiver (false positive?)
7. command_builder.go#L104 suspicious assignment to a by-value method receiver (false positive?)
8. command_builder.go#L117 suspicious assignment to a by-value method receiver (false positive?)
9. command_builder.go#L130 suspicious assignment to a by-value method receiver (false positive?)
10. jailer.go#L178 suspicious assignment to a by-value method receiver (false positive?)
11. jailer.go#L184 suspicious assignment to a by-value method receiver (false positive?)
12. jailer.go#L190 suspicious assignment to a by-value method receiver (false positive?)
13. jailer.go#L196 suspicious assignment to a by-value method receiver (false positive?)
14. jailer.go#L203 suspicious assignment to a by-value method receiver (false positive?)
15. jailer.go#L210 suspicious assignment to a by-value method receiver (false positive?)
16. jailer.go#L217 suspicious assignment to a by-value method receiver (false positive?)
17. jailer.go#L225 suspicious assignment to a by-value method receiver (false positive?)
18. jailer.go#L231 suspicious assignment to a by-value method receiver (false positive?)
19. jailer.go#L244 suspicious assignment to a by-value method receiver (false positive?)
20. jailer.go#L257 suspicious assignment to a by-value method receiver (false positive?)
21. jailer.go#L270 suspicious assignment to a by-value method receiver (false positive?)
22. jailer.go#L277 suspicious assignment to a by-value method receiver (false positive?)
23. jailer.go#L283 suspicious assignment to a by-value method receiver (false positive?)
24. handlers.go#L377 suspicious assignment to a by-value method receiver (false positive?)
25. handlers.go#L384 suspicious assignment to a by-value method receiver (false positive?)
26. handlers.go#L461 suspicious assignment to a by-value method receiver (false positive?)

Report issue: https://github.com/firecracker-microvm/firecracker-go-sdk/issues

[powerman]

Then we have to discuss if it's a problem or not.

Can you please explain why do you think we have to discuss this code?

At a glance I don't see how it differs from my use case in the beginning of this issue. Builder pattern is supposed to be used in exactly this way (val := New().WithA().WithB().Build()). It doesn't looks like this code is buggy and supposed to use pointer receiver in all With() methods.

[ccoVeille]

Then we have to discuss if it's a problem or not.

Can you please explain why do you think we have to discuss this code?

The more examples I look in the reporting, the more I'm convinced it's a good pattern. That should indeed not being reported.

You use a copy, so you return a new object, then the function keeps the struct immutable.

It's a good thing.

There might be very rare use cases where something like this won't be doing what it expected.

func (a A) Foo() A {
   a.whatever = true
   return a
}

But let's say we don't care. It's better to have less false-positive

So yes, I agree, we should avoid reporting them

Sorry for the discussion, but talk is cheap, missing bugs is not. That's why I wanted @alingse reporting to find some random code.

Now, there is one use case, a theorical one again

func (a A) Foo() *A {
   a.whatever = true
   return &a
}

Should this one be reported or not?

This one could be the code of a Clone method that reset a few fields

func (a A) Clone() *A {
   a.whatever = true
   return &a
}

So here again, a code that seems legitimate too.

Is there any counter examples you may think about of a code returning a pointer?

[powerman]

Is there any counter examples you may think about of a code returning a pointer?

I don't see any real difference between returning a data or a pointer - this depends on what user expect to get, not on method implementation details. E.g. if user gets a pointer from New() method and then user may wanna apply a few modifications using a chain of WithX() methods, then these WithX() methods also should return a pointer (but their receiver will not be a pointer to ensure cheap-lazy clone before modification) to make sure type of user's variable won't change and will be the same (as returned by New()) both with and without WithX() calls.

[chavacava]

One question, what would be the behavior of the linter with a method that return a pointer

IMO, do not report a failure (considered as false positive)

Same question with a method that return a signature with error, quite common

IMO, do not report a failure (considered as false positive)

[ccoVeille]

One question, what would be the behavior of the linter with a method that return a pointer

IMO, do not report a failure (considered as false positive)

Same question with a method that return a signature with error, quite common

IMO, do not report a failure (considered as false positive)

Is it covered by the current code (the one in a dedicated branch) ?

I mean will this be marked as false positive and the later ignored (no computer with me now, I'm on phone)

func (l Logger) Clone() (Logger, error) {
   l.whatever = true
   return l
}

[ccoVeille]

Thanks for your time and patience @powerman @chavacava