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.)