modifies-value-receiver: miss modification by setting something in a sub structure

[ccoVeille]

type B struct {
	whatever bool
}

func (b B) Foo() B {
	b.whatever = true
	return b
}

type A struct {
	B B
}

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

modifies-value-receiver reports b.whatever = true as an error

but a.B.whatever = true is not reported

Same for

type B struct {
	whatever bool
}

func (b B) Foo() {
	b.whatever = true
}

type A struct {
	B
}

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

but this leads to detection

type B struct {
	whatever bool
}

func (b B) Foo() {
	b.whatever = true
}

type A struct {
	B
}

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

Somehow related to :

• modifies-value-receiver: miss modification by setting something in a slice #1116
• modifies-value-receiver: miss modification by ++/-- #1109

[chavacava]

The head version of revive correctly returns no failure for the first code example:

type B struct {
	whatever bool
}

func (b B) Foo() B {
	b.whatever = true
	return b
}

type A struct {
	B B
}

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

For the second code sample

type B struct {
	whatever bool
}

func (b B) Foo() {
	b.whatever = true
}

type A struct {
	B
}

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

revive correctly warns on the b.whatever = true assignment but fails to warn on a.B.whatever = true
The difficulty of cases like this is the level of indirection. The analysis needs to be aware of the type of a.B (in the first example field name and type are the same but it is not necessarily the always the case).
For example in the following case, the assignment should be tagged by revive as suspicious

type B struct {
	whatever bool
}

func (b B) Foo() {
	b.whatever = true
}

type A struct {
	B B
}

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

But if we define B differently

type C struct {
	whatever bool
}

func (c C) Foo() {
	c.whatever = true
}

type B *C

type A struct {
	B B
}

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

the assignment is legit
(keep in mind that in real cases, the level of indirection is not bounded to 2, it can be 3, 4, ...)

The definition of other types than the one for which we check the methods (A in the example) are not always available in the same file thus it will require the rule to, at least, use type-checker information.

Currently, the rule is based on syntax-level analysis of the method (it simply checks if the receiver's identifier appears in the LHS of an assignment expression + some patches to not warn in some corner cases)

Extending the rule to cope with the cases shown in the original post is possible.
The question, as usual, is if the added complexity (code+time) is compensated by the gain in new positive cases it detects.

[ccoVeille]

Thanks for your reply and the detail you dropped.

The example of type B *C is indeed a good one, about the difficulties to determine if something should be reported or not.

I agree it can become a nightmare to detect, also I know the rule about linter should avoid false-positive

For records, I opened a PR yesterday with examples of what fails and what does
https://github.com/mgechev/revive/pull/1180/files

[chavacava]

A current false positive that I think it could be fixed is:

func (c RateLimitsConfig) MarshalJSON() ([]byte, error) {
	type alias RateLimitsConfig

	// serialize as empty array
	if c.Rules == nil {
		c.Rules = []RateLimitRule{} // suspicious modification...
	}

	return json.Marshal(alias(c)) //...that might be considered OK because it's used to compute the result
}

or

// Validate makes PasswordAuthConfig validatable by implementing [validation.Validatable] interface.
func (c PasswordAuthConfig) Validate() error {
	// strip duplicated values
	c.IdentityFields = list.ToUniqueStringSlice(c.IdentityFields)

	return validation.ValidateStruct(&c,
		validation.Field(&c.IdentityFields, validation.Required),
	)
}