fix: false positive for returns in exhaustive switch#3067
fix: false positive for returns in exhaustive switch#3067EdRW wants to merge 5 commits intovuejs:masterfrom
Conversation
…n to check for exhaustive switches
…er-return and return-in-computed-property rules
…s in computed properties
…e/require-render-return for exhaustive switch statements with TypeScript
🦋 Changeset detectedLatest commit: 421d9f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
EdRW
changed the title
| /** | ||
| * @param {Type} type | ||
| * @param {typeof import('typescript')} ts | ||
| * @returns {boolean} | ||
| */ | ||
| function isTypeLiteralLike(type, ts) { | ||
| return ( | ||
| (type.flags & | ||
| (ts.TypeFlags.Literal | | ||
| ts.TypeFlags.Undefined | | ||
| ts.TypeFlags.Null | | ||
| ts.TypeFlags.UniqueESSymbol)) !== | ||
| 0 | ||
| ) | ||
| } |
There was a problem hiding this comment.
Basically copied from switch-exhaustiveness-check.ts#L415
| /** | ||
| * @param {Type} type | ||
| * @returns {Type[]} | ||
| */ | ||
| function unionConstituents(type) { | ||
| return type.isUnion() ? type.types : [type] | ||
| } | ||
|
|
||
| /** | ||
| * @param {Type} type | ||
| * @returns {Type[]} | ||
| */ | ||
| function intersectionConstituents(type) { | ||
| return type.isIntersection() ? type.types : [type] | ||
| } |
| /** | ||
| * @param {RuleContext} context | ||
| * @param {SwitchStatement} switchNode | ||
| * @returns {boolean} | ||
| */ | ||
| function isExhaustiveSwitch(context, switchNode) { |
There was a problem hiding this comment.
This function is derived from getSwitchMetadata in switch-exhaustiveness-check.ts
There was a problem hiding this comment.
question
Most of the work was done here because it's TS related, but I wasn't sure if this is actually the most appropriate location.
Is there a more suitable place or is it ok keep the exhaustive switch-related logic here?
There was a problem hiding this comment.
Since return-in-computed-property and require-render-return share the same logic for exhaustive switch statements, the thorough tests of the full range of supported types probably only need to happen in one place. So they were added here.
| } | ||
| }) | ||
| </script>`, | ||
| ...getTypeScriptFixtureTestOptions() |
There was a problem hiding this comment.
(edited)
Could you please add the following test case?
<script setup lang="ts">
import { computed, ref } from 'vue'
const flag = ref<boolean>(true)
const result = computed(() => {
if (x !== 0) {
switch (flag.value) {
case true: return 'yes'
case false: return 'no'
}
} else {
return 'x === 0'
}
})
</script>Is it supported?
There was a problem hiding this comment.
Thanks for the review and suggestion.
Is it supported?
Unfortunately, it's not supported yet. I mainly focused on the case where switch is the last statement in the function. In this example, the switch is nested in an if-statement.
Could you please add the following test case?
Sure I can try. It think it'll involve a lot of recursive traversal though😨 . I might also take a look at try/catch as well.
There was a problem hiding this comment.
In my opinion, rather than using recursion, it would probably be better to use ESLint's Code Path Analysis to process the relevant switch statements.
https://eslint.org/docs/latest/extend/code-path-analysis
However, you don't need to add support in this PR 😄
I think even just the changes in this PR are a valuable improvement.
Could you please add test cases and leave comments about anything that isn't yet supported?
2 participants
vue/require-render-returnfalse positive for switch-case on enum without default #2100Description
Currently
return-in-computed-propertyandrequire-render-returnreport an error for switch statements without adefaultcase, when the switch is the final statement. For Typescript users the error is shown even when the final statement is a switch statements that exhaustively callsreturnorthrowfor every case.This PR updates
executeOnFunctionsWithoutReturnto usetypescript-eslint/parserservices (when available) to verify exhaustive switch statements.No behavior changes for Javascript users. Switch statements without
defaultcase will still show an error when typing info is not available.Supported patterns
This implementation borrows much from typescript-eslint's implementation of switch-exhaustiveness-check.ts, and supports:
true | false)T | null,T | undefined)switch (event.type))Most of the lines added in this PR are just test cases to check all of these patterns.
Screenshots
Examples come from #2142return-in-computed-property
Exhaustive
Non-exhaustive
require-render-return
Examples come from #2100
Exhaustive
Non-exhaustive