new rule: nested-structs

[rdeusser]

Please, describe in details what's your motivation for this PR

Nested structs are awful to look at and I want to disallow them entirely in my projects.

Did you add tests?

Yes.

Does your code follows the coding style of the rest of the repository?

Yes.

Does the Travis build passes?

TBD

Closes #529

[rdeusser]

Tests pass for me locally. CI failed with this error: The command "openssl aes-256-cbc -K $encrypted_ff954c97a679_key -iv $encrypted_ff954c97a679_iv -in .docs-deploy-key.pem.enc -out .docs-deploy-key -d" failed and exited with 1 during .

[chavacava]

Hi (again) @rdeusser and thanks for the PR.

To add a new rule to the revive machinery it must be added to the list of rules in the configuration. Updates of the README.md and the RULES_DESCRIPTIONS.md are also welcome.

[rdeusser]

@chavacava Hi!

Added the rule to the list of rules in the config and updated the README as well as the RULES_DESCRIPTION.

[chavacava]

@chavacava Hi!

Added the rule to the list of rules in the config and updated the README as well as the RULES_DESCRIPTION.

Nice, thanks.
I left some comments on the code

[rdeusser]

Hey @chavacava I think I got everything. Can you re-review?

[chavacava]

I've tested the rule against some codbases (kebernetes, docker), it works well.

The rule spots cases like the following (from github.com/google/go-cmp/cmp/internal/diff/diff.go:62:31)

func (es EditScript) stats() (s struct{ NI, NX, NY, NM int }) {

and like (from kubernetes/pkg/volume/git_repo/git_repo_test.go:283:19)

func doTestPlugin(scenario struct {
	name              string
	vol               *v1.Volume
	expecteds         []expectedCommand
	isExpectedFailure bool
}, t *testing.T) []error {

Beyond that it seems to me very hard to read, it is not a nested struct. Should we warn on cases like that? If yes, the rule is not about nested structs but something else. We can also let as is (the above cases are very rare)

In the test cases we could add a case for multiple nests:

type Two struct {
	Bar struct { // MATCH /no nested structs are allowed/
		Baz struct { // MATCH /no nested structs are allowed/
			b   boolean
			Foo struct { // MATCH /no nested structs are allowed/
				b boolean
			}
		}
	}
}

[rdeusser]

Hey @chavacava apologies for the late reply.

Yeah those cases are a bit odd and I think there should be a rule for that. I'm of the opinion that this rule should cover nested structs and nothing else and a separate rule (potentially) for the examples you found above to maximize flexibility. I'd like to know your thoughts on this rule covering both use-cases (assuming the rule would be renamed) or remain as-is?

[rdeusser]

I went ahead and made the change to ignore structs in func declarations and added some additional test cases for that until I hear back from you.

[chavacava]

Suggested change

	case *ast.FuncDecl:
	return nil
	case *ast.FuncDecl:
	if v.Body != nil {
	ast.Walk(l, v.Body)
	}
	return nil

[rdeusser]

Oof, yeah didn't see that. Thanks.

[chavacava]

Hi @rdeusser thanks for the new commit and sorry for the late response.

The new version skips function declarations completely (function bodies are not analyzed) therefore we will not detect nested structs declared in function bodies (they were a large portion of failures detected by the previous version in code bases like kubernetes and docker) I've left a suggestion in the code to let the rule analyze function bodies while skipping function signatures.

[rdeusser]

@chavacava Thanks for the review! I got that fixed and added an additional test case for what you mentioned.

[chavacava]

Thanks @rdeusser