build(engine): catch switch fallthrough - #680
Merged
Merged
Conversation
Zero errors on the existing tree, so this costs nothing to adopt and blocks no idiom. It catches a real class for an engine with this many switch-on-code paths, where a missing `return` silently runs the next case's body. Verified rather than assumed: a deliberate fallthrough fails typecheck, and removing it passes. This is the whole of what a four-flag suggestion reduced to, after measuring each rather than taking strict-by-default as obviously right: - `exactOptionalPropertyTypes` — NOT adopted. 45 errors, and the fixes are conditional spreads and piecemeal object construction, which make code worse to read in exchange for a distinction the reader already understood. It also only catches the mechanical half: the three bugs that actually cost us today were semantic — `changedFiles: []` beside an error, `discarded: []` beside `discardedError`, `conflictsWithBase: null` meaning undetermined. No flag catches those. A comment on the field saying which absence means what does, and is already the practice here. - `noUnusedLocals` / `noUnusedParameters` — NOT adopted, because biome already reports both (`noUnusedVariables`, `noUnusedFunctionParameters`) and does it at the right layer: fixable, and suppressible per-line with a reason. A compiler flag would turn an unused callback parameter into a build failure whose only workaround is renaming to `_`. Confirmed by probe, not by reading the config: biome flags both today.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Zero errors on the existing tree. Costs nothing to adopt, blocks no idiom, and catches a real class for an engine with this many switch-on-code paths — where a missing
returnsilently runs the next case's body.Verified rather than assumed: a deliberate fallthrough fails typecheck, and removing it passes.
What this replaced
A four-flag suggestion, measured one at a time rather than adopted as a set:
noFallthroughCasesInSwitchexactOptionalPropertyTypesnoUnusedLocalsnoUnusedParametersOn
exactOptionalPropertyTypesThe argument for it was that today produced three bugs about present-but-meaningless being indistinguishable from absent. True — but the flag only catches the mechanical half. The ones that actually cost time were semantic:
changedFiles: []beside anerror,discarded: []besidediscardedError,conflictsWithBase: nullmeaning undetermined. No compiler flag catches those.What does catch them is a comment on the field stating which absence means what, which is already the practice in this codebase. Meanwhile the flag's cost is certain: 45 errors whose fixes are conditional spreads and piecemeal object construction.
On the unused-code flags
Confirmed by probe rather than by reading config — biome reports both today:
That's the right layer: fixable, and suppressible per-line with a reason. A compiler flag turns an unused callback parameter into a build failure whose only workaround is renaming to
_.Gates
457 tests, typecheck, lint, e2e 70/70.