rule: add no-increment-decrement rule

[ChrisJr404]

This adds no-increment-decrement, a new rule that is the mirror image of the existing increment-decrement rule, for people who prefer the explicit i += 1 / i -= 1 forms over i++ / i--.

As raised in #910, increment-decrement is opinionated in the direction of ++/--, but plenty of styles (and languages) go the other way, and in practice ++/-- are rarely used in Go outside of loop counters. The new rule spots i++ and i-- statements and proposes replacing them with i += 1 and i -= 1. Increment/decrement statements that appear as the post statement of a for loop are intentionally left alone, since that is the idiomatic place for a loop counter (see the for i := 0; i < n; i++ example in the docs).

Like every other revive rule it is opt-in, so the default behaviour is unchanged. It works purely on the AST and requires no type information, so it is registered in untyped.toml.

Naming: I went with no-increment-decrement to make it easy to find as the counterpart of increment-decrement, but I'm happy to rename it if you'd prefer something else.

Changes:

• rule/no_increment_decrement.go — the rule.
• test/no_increment_decrement_test.go + testdata/no_increment_decrement.go — fixtures covering standalone ++/--, a selector operand, a counter incremented inside a loop body (flagged), and for-loop counters (not flagged).
• Registered the rule in config/config.go and untyped.toml, bumped the rule count in config/config_test.go, and documented it in RULES_DESCRIPTIONS.md.

Testing: go test ./... passes, and both revive --config revive.toml ./... and golangci-lint run are clean on the new files.

Closes #910

[chavacava]

Hi @ChrisJr404, thanks for the PR.
I've left some comments.

[chavacava]

I think it's possible to handle the special case of increments in the loop post without keeping track of loopPosts in a list: when visiting a forStmt launch the visitor on the for body and then return nil; this will make the visitor to skip the post part of the for.

Something like:

Suggested change

	if stmt.Post != nil {
	// The post statement of a for loop is the idiomatic place for a loop
	// counter, so it is exempted from the rule.
	w.loopPosts[stmt.Post] = struct{}{}
	}
	w.Visit(stmt.Body) //maybe a check stmt.Body != nil is necessary before calling Visit
	return nil

[chavacava]

confidence could be 1, here we are sure it's an increment/decrement that should be replaced

[chavacava]

It could be interesting to test with an increment in a for body

Suggested change

	for j := 10; j > 0; j-- {
	_ = j
	}
	for j := 10; j > 0; j-- {
	_ = j
	}
	x := 0
	for j := 10; j > 0; j-- {
	_ = j
	x++ // MATCH /should replace x++ with x += 1/
	x-- // MATCH /should replace x-- with x -= 1/
	}

[chavacava]

@ChrisJr404 please also include the rule in the rule list table on the README.md

[ChrisJr404]

Good catch - added the rule to the README rule list table (between nested-structs and optimize-operands-order).

[ccoVeille]

I have a problem here.

First, this rule is purely stylistic. Some may like it, some may not. I feel like 99% of people will be in the second team.

But the real problem is the fact once merged this rule will be part of revive enable all rules settings. And then it would be enabled for anyone.

For me, this should be a linter independent from revive.

The only possible way according to me would be that this feature becomes a setting of an existing revive rule. So here, I feel a setting of increment-decrement should be the way.

Also, if we now consider there is a rule for using i++ and one for using i += 1, using enable-all-rules would lead to infinite back and forth fixes

This is the feedback I do, and I'm looking for maintainers feedback.

Do not rush into doing changes.