revive icon indicating copy to clipboard operation
revive copied to clipboard

False Positive in rule max-control-nesting

Open sk- opened this issue 1 year ago • 2 comments

Describe the bug The newly added rule max-control-nesting added in PR #967, does not correctly handle else if blocks.

To Reproduce Steps to reproduce the behavior:

  1. Add the following test to the file testdata/max-control-nesting.go:
	a := 1
	if a == 0 {
	} else if a == 1 {
	} else if a == 2 {
	} else {
	}
  1. Run the tests with go test ./...
  2. You will get the following error:
--- FAIL: TestMaxControlNesting (0.00s)
    utils_test.go:129: Unexpected problem at max-control-nesting.go:71: control flow nesting exceeds 2
    utils_test.go:129: Unexpected problem at max-control-nesting.go:71: control flow nesting exceeds 2

The steps above are for a minimum reproducible example. However, this also fails with the latest version of golangci-lint, which flagged the following code (default is 5):

	if e == "a" {
		return 1
	} else if e == "b" {
		return 2
	} else if e == "c" {
		return 3
	} else if e == "d" {
		return 4
	} else if e == "e" {
		return 5
	} else if e == "f" {
		return 6
	}

Expected behavior No nesting errors should happen when there is no nestedness in else if blocks

Desktop (please complete the following information):

  • OS: macOS 12.3
  • Version of Go: 1.22.0 (when running the tests), 1.21.7 when using golangci-lint

sk- avatar Feb 15 '24 11:02 sk-

I think, the reason for that is that

	if e == "a" {
		return 1
	} else if e == "b" {
		return 2
	} else if e == "c" {
		return 3
	} else if e == "d" {
		return 4
	} else if e == "e" {
		return 5
	} else if e == "f" {
		return 6
	}

is interpreted as

	if e == "a" {
		return 1
	} else {
		if e == "b" {
			return 2
		} else {
			if e == "c" {
				return 3
			} else {
				if e == "d" {
					return 4
				} else {
					if e == "e" {
						return 5
					} else {
						if e == "f" {
							return 6
						}
					}
				}
			}
		}
	}

If you want a go idiomatic solution that will pass through the rule you can use the following alternative:

switch {
case e == "a":
	return 1
case e == "b":
	return 2
case e == "c":
	return 3
case e == "d":
	return 4
case e == "e":
	return 5
case e == "f":
	return 6
}

It will do the same, but it won't be nesting and it will be more idiomatic to go (it is also shorter than if/else if/else construct).

denisvmedia avatar Feb 15 '24 12:02 denisvmedia

Hi @sk-, thanks for reporting the issue. I've developed the rule with the else-if interpretation described by @denisvmedia because I consider else-if constructs are indeed nested structures. The rule being not friendly with else-if guides naturally to write idiomatic Go code (as the above example by @denisvmedia)

chavacava avatar Feb 15 '24 13:02 chavacava

I' ll close this issue. Feel free to reopen if you think the current behavior of the rule with else-if needs to be modified.

chavacava avatar May 02 '24 10:05 chavacava