Repository navigation
Conversation
…tions isInsideBroadEffectHandler walks parents until it reaches an arrow or a function expression, then reports whether that function is the handler argument of Effect.catch/catchAll/catchIf. A function declaration was not a stopping point, so a helper declared inside a handler was treated as part of it. The same helper written as an arrow is outside the handler, so the two spellings disagreed: a declaration was excused by no-manual-tag-comparison and reported by no-manual-effect-error-tag, while an arrow or function expression got the reverse. The advice also pointed the wrong way, since catchTag only helps when the discriminated value is the caught error. Stop the walk at a FunctionDeclaration and report "not in a handler". Both Effect tag rules now treat a nested helper the same way however it is spelled, and the plain tag check is reported by the rule about tag comparisons. Coverage: a declaration helper and its arrow counterpart in no-manual-tag-comparison, a declaration helper in the valid set of no-manual-effect-error-tag, plus a switch inside a declaration so both visitors are exercised. Verified with `pnpm check` on Node 22 (lint, 24 RuleTester suites, tsc --noEmit, skill-asset sync check).
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.
Closes #48.
Problem
isInsideBroadEffectHandlerinsrc/effect/shared/tagged-values.tswalks up the parent chain and stops at the firstArrowFunctionExpressionorFunctionExpression, then reports whether that function is the handler argument ofEffect.catch/catchAll/catchIf.A
FunctionDeclarationwas not a stopping point, so a helper declared inside a handler was treated as part of it:That fires
no-manual-effect-error-tag, which recommendsEffect.catchTag/Effect.catchTags.Cause
The same helper written as an arrow gives the opposite result:
This fires
no-manual-tag-comparison, which recommendsMatch/Predicate.isTagged.So the spelling of an unrelated helper decided which of the two rules reported it, and the declaration spelling recommended
catchTag— wrong advice, sincelabelinspectsstate, not the caughterror.catchTagwould not help; the value being branched on is not the onecatchAllhands the handler.Change
Stop the walk at a
FunctionDeclarationand report "not in a handler". A declaration is never the handler argument itself, so once the search reaches one the enclosing handler cannot own the comparison:Both Effect tag rules now treat a nested helper the same way however it is spelled, and a plain tag check is reported by the rule about tag comparisons.
Effect.catchAll(function (error) { … })passed directly is aFunctionExpression, so that path is unchanged — the existing test for it still passes.Coverage
no-manual-tag-comparison: a declaration helper and its arrow counterpart as invalid cases, so the two spellings now agree; aswitchinside a declaration, to exercise theSwitchStatementvisitor too.no-manual-effect-error-tag: a declaration helper added to the valid set.Verification
pnpm check(lint, all 24 RuleTester suites,tsc --noEmit, skill-asset sync check) on Node 22.23.0, oxlint 1.78.0, againstc44ef22.