Refine existing type in Predicate.isTagged#6497
Conversation
🦋 Changeset detectedLatest commit: 7825337 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesisTagged refinement
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/effect/typetest/Predicate.tst.ts (1)
61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the uncurried overload too.
These assertions only exercise
Predicate.isTagged("a"). The changed two-argument overload on Line 1168, used bypackages/effect/src/Filter.ts, can regress independently. Add a direct type-level assertion usingPredicate.isTagged(value, "a")for both matching and non-matching inputs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/effect/typetest/Predicate.tst.ts` around lines 61 - 65, Extend the type-level tests around the existing Predicate.isTagged assertions to cover the uncurried two-argument overload, using Predicate.isTagged(value, "a") with both matching and non-matching inputs. Assert the resulting narrowed types for the matching case and the never/non-match behavior, reusing the existing test values where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/funny-drinks-scream.md:
- Line 5: Update the changeset text to reference the exported API as
Predicate.isTagged, replacing the incorrect Predicated.isTagged name while
preserving the rest of the release note.
In `@packages/effect/src/Predicate.ts`:
- Around line 1167-1168: Update both overloads of the tag predicate to replace
the T extends {} conditional with T extends { _tag: K } ? T : T & { _tag: K }.
Preserve precise narrowing for already-tagged unions while allowing broad object
inputs to narrow to the required _tag shape.
---
Nitpick comments:
In `@packages/effect/typetest/Predicate.tst.ts`:
- Around line 61-65: Extend the type-level tests around the existing
Predicate.isTagged assertions to cover the uncurried two-argument overload,
using Predicate.isTagged(value, "a") with both matching and non-matching inputs.
Assert the resulting narrowed types for the matching case and the
never/non-match behavior, reusing the existing test values where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 77cc775c-09e1-4a77-b49c-52aa0cd1dc74
📒 Files selected for processing (3)
.changeset/funny-drinks-scream.mdpackages/effect/src/Predicate.tspackages/effect/typetest/Predicate.tst.ts
| <K extends string>(tag: K): <T>(self: T) => self is T extends {} ? Extract<T, { _tag: K }> : T & { _tag: K } | ||
| <K extends string, T>(self: T, tag: K): self is T extends {} ? Extract<T, { _tag: K }> : T & { _tag: K } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
pnpm test-types packages/effect/typetest/Predicate.tst.ts
rg -n 'ReadonlyArray<object>|Record<string, unknown>|isTagged' packages/effect/typetest packages/effect/src/Predicate.tsRepository: Effect-TS/effect
Length of output: 535
Preserve narrowing for broad object inputs.
T extends {} routes object and Record<string, unknown> through Extract, which collapses to never for values that can still pass the runtime _tag check. Use T extends { _tag: K } ? T : T & { _tag: K } in both overloads so tagged unions stay precise and broad inputs still narrow correctly.
Suggested refinement
- <K extends string>(tag: K): <T>(self: T) => self is T extends {} ? Extract<T, { _tag: K }> : T & { _tag: K }
- <K extends string, T>(self: T, tag: K): self is T extends {} ? Extract<T, { _tag: K }> : T & { _tag: K }
+ <K extends string>(tag: K): <T>(self: T) => self is T extends { _tag: K } ? T : T & { _tag: K }
+ <K extends string, T>(self: T, tag: K): self is T extends { _tag: K } ? T : T & { _tag: K }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <K extends string>(tag: K): <T>(self: T) => self is T extends {} ? Extract<T, { _tag: K }> : T & { _tag: K } | |
| <K extends string, T>(self: T, tag: K): self is T extends {} ? Extract<T, { _tag: K }> : T & { _tag: K } | |
| <K extends string>(tag: K): <T>(self: T) => self is T extends { _tag: K } ? T : T & { _tag: K } | |
| <K extends string, T>(self: T, tag: K): self is T extends { _tag: K } ? T : T & { _tag: K } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/effect/src/Predicate.ts` around lines 1167 - 1168, Update both
overloads of the tag predicate to replace the T extends {} conditional with T
extends { _tag: K } ? T : T & { _tag: K }. Preserve precise narrowing for
already-tagged unions while allowing broad object inputs to narrow to the
required _tag shape.
Source: MCP tools
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
thewilkybarkid
changed the title
2 participants
Type
Description
This change allows Predicate.isTagged to narrow the type of known tagged structures, rather than turning everything into { _tag: K }. This change does mean that any now remains any, but I can't think of a way around that.
Related
Summary by CodeRabbit
Predicate.isTaggedtype narrowing to preserve more of the original input shape for tagged unions.isTaggedto validate narrowing outcomes across multiple input scenarios.