Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate map keys the same way as switch cases #48

Merged
merged 2 commits into from Aug 15, 2022

Conversation

maratori
Copy link
Contributor

Fix #35

In some cases switch by enum is too verbose and can be simplified by using map. WIth this PR exhaustive validates map keys as well.

type Enum int

const (
	A Enum = iota
	B
	C
)

func EnumToString(e Enum) string {
	return map[Enum]string{
		A: "A",
		B: "B",
		C: "C",
	}[e]
}

This is a preparation for map keys validation
@maratori
Copy link
Contributor Author

@nishanths I've decided to not refactor code in this PR.

  1. I'm not sure how would you like to do it
  2. PR is huge even without refactoring (however the most changes are in tests)

@nishanths
Copy link
Owner

nishanths commented Aug 2, 2022

Thank you so much. I will review it during the upcoming weekend.

@nishanths
Copy link
Owner

I looked at it quickly, and I couldn't find parts that would significantly benefit from refactoring right now, so I can think we can do it later if necessary. Did you have any parts in mind?

@maratori
Copy link
Contributor Author

maratori commented Aug 3, 2022

I don't have concrete suggestions for now. But here are some points of improvement:

  • Following entities are declared in switch.go, but used in map.go as well:
    • type nodeVisitor
    • const resultXXX
    • func diagnosticEnumTypeName
    • func diagnosticMissingMembers
  • Skipping generated files is duplicated in both switchStmtChecker and mapChecker
  • mapChecker calls analyzeCaseClauseExpr

@nishanths nishanths merged commit fad1089 into nishanths:master Aug 15, 2022
@nishanths
Copy link
Owner

Thank you, merged! And sorry for the delay.

I will follow up with a commit to add -maps flag for checking maps, so users have a way to opt-in/opt-out of checking maps.

@maratori maratori deleted the map branch August 15, 2022 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate map keys the same way as switch cases
2 participants