Skip to content

modifies-value-receiver: add support for immutable values #1066

@powerman

Description

@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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions