Make checkswitch handle regular switches
#4162
Conversation
1. checkswitch.go: Code added to handle switch statements 2. testdata.go: New test cases handled to ensure switch statements are accounted for
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4162 +/- ##
==========================================
- Coverage 75.01% 73.89% -1.13%
==========================================
Files 339 339
Lines 22827 22827
==========================================
- Hits 17124 16868 -256
- Misses 4410 4677 +267
+ Partials 1293 1282 -11 see 28 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
AlekSi
requested review from
a team,
henvic,
rumyantseva,
chilagrow and
noisersup
AlekSi
changed the title
checkswitch handle regular switches
|
|
||
| // Iterate over each case in the switch statement. | ||
| for _, el := range n.Body.List { | ||
| // If a case has no expressions (i.e., it's a default case), skip to the next case. |
There was a problem hiding this comment.
Do we actually have default cases? I think we have linter enabled to check that switches are exhaustive. Maybe that tool should double-check that too.
But it is not a string: Line 19 in e98d536 |
There was a problem hiding this comment.
Those tests rely on "magic" want comments: https://pkg.go.dev/golang.org/x/tools/go/analysis#hdr-Testing_an_Analyzer There are no such comments, so that test doesn't check anything.
AlekSi
removed request for
a team,
henvic,
rumyantseva,
chilagrow and
noisersup
|
Hi thanks for your review. Will look at this again this week. |
Description
PR Description:
This PR introduces changes to the
checkSwitchfunction incheckswitch.goto improve the handling of switch statements in Go code.The changes include:
*ast.SwitchStmt) in Go code.The reasons for these changes are to ensure that the
checkSwitchfunction correctly identifies and reports cases in switch statements that are out of order. This is important for maintaining code readability and consistency.The decision to handle cases with no expressions and cases with more than one expression separately was made because these cases require different logic. For cases with no expressions, there is no need to check the order, so these cases are skipped. For cases with more than one expression, each expression needs to be checked individually, so a separate loop is used for this.
Reviewers should know that these changes improve the robustness of the
checkSwitchfunction and ensure that it can handle a wider variety of switch statements. The changes have been thoroughly tested to ensure that they work as expected. However, reviewers are encouraged to scrutinize the changes closely, especially the handling of cases with more than one expression, as this is a complex part of the code.Closes #4082
Open Questions
Question to @AlekSi and other reviewers: I don't really understand how the test cases work. I have added them as per my understanding, however if I comment my implementation and run the tests they still pass. Maybe I am missing something but your guidance will help here.
Why does token.STRING work here and why that is added? (this is my understanding, please correct if incorrect)
In Go, token.STRING is a constant that represents the string type in Go's token package. It's used when parsing Go source code into an Abstract Syntax Tree (AST).
When you're dealing with a *ast.SwitchStmt in Go, the cases of the switch can be any expression. In your context, you're dealing with a switch statement that switches over bson.tag. The bson.tag is a string that represents the type of the BSON element.
When the Go source code is parsed into an AST, this bson.tag string becomes a *ast.BasicLit with a Kind of token.STRING. That's why the code checks if the firstTypeCase is a *ast.BasicLit with a Kind of token.STRING. If it is, it means that this case of the switch statement is switching over a bson.tag.
Readiness checklist
task all, and it passed.@FerretDB/core), Milestone (Next), Labels, Project and project's Sprint fields.