fix(literal): use Object.is so literal(NaN) accepts NaN - #1529
Conversation
The literal schema compares with === at the value check. Since NaN === NaN is false, literal(NaN) was an unsatisfiable schema: it rejected every input including the NaN it was built from. NaN is a valid number and Literal includes number, so literal(NaN) is a type-checked construction whose runtime could never succeed. The sibling picklist schema accepts NaN (it uses Array.includes, which is SameValueZero), so the two disagreed on NaN. Switch the comparison to Object.is, matching the equality migration in open-circle#1517 (_merge) and open-circle#1477 (value/notValue/values/notValues). Object.is(NaN, NaN) is true, so literal(NaN) now accepts NaN. As a side effect literal(0) no longer accepts -0 (=== treated them as equal), consistent with the Object.is direction of open-circle#1517/open-circle#1477.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@library/src/schemas/literal/literal.ts`:
- Around line 97-99: Update the `~run` implementation in `literal.ts` so it no
longer uses `Object.is` for all comparisons, since that changes zero-literal
behavior. Special-case `NaN` to use a NaN-safe check, and keep strict equality
(`===`) for every other `literal(...)` value so `literal(0)` still accepts `-0`
and `literal(-0)` still accepts `0`. Use the existing `literal` class and its
`~run(dataset, config)` method as the place to make the comparison logic change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ae6e97b-36d9-48e7-85fe-fd693e8d34f1
📒 Files selected for processing (2)
library/src/schemas/literal/literal.test.tslibrary/src/schemas/literal/literal.ts
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="library/src/schemas/literal/literal.ts">
<violation number="1" location="library/src/schemas/literal/literal.ts:98">
P2: Switching literal equality to Object.is introduces a signed-zero behavior change and inconsistency with picklist.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
🧹 Nitpick comments (2)
library/src/schemas/literal/literal.ts (2)
102-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStray blank line inside the condition.
Line 102 appears to be a whitespace-only line left in the middle of the boolean expression; remove it for readability.
🤖 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 `@library/src/schemas/literal/literal.ts` at line 102, There is a stray whitespace-only blank line inside the boolean condition in the literal schema logic, which breaks readability. Remove the empty line from the condition in the relevant expression in literal.ts, keeping the boolean check contiguous and compact so the surrounding logic remains easy to scan.
100-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaN and signed-zero fix looks correct.
Using
dataset.value === this.literal || (dataset.value !== dataset.value && this.literal !== this.literal)correctly matchesNaNtoNaNvia the self-inequality idiom while keeping===for everything else, so-0/+0still compare equal as before. This addresses the prior review comment requesting a NaN-specific special case instead of blanketObject.is.The Biome
noSelfComparewarning on line 103 is a false positive here since the self-comparison is the intentional NaN-detection idiom; consider adding a// biome-ignore lint/suspicious/noSelfCompare: NaN checkcomment to suppress it and document intent for future readers.🤖 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 `@library/src/schemas/literal/literal.ts` around lines 100 - 104, The NaN/signed-zero comparison in literal matching is correct, but the intentional self-comparison triggers Biome’s noSelfCompare warning. In the `Literal` schema’s equality check, add a targeted Biome ignore comment directly above the `dataset.value !== dataset.value && this.literal !== this.literal` NaN guard to document that the self-comparison is deliberate. Keep the existing `Literal` logic unchanged and place the suppression close to the `dataset.value`/`this.literal` comparison so future readers understand the intent.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@library/src/schemas/literal/literal.ts`:
- Line 102: There is a stray whitespace-only blank line inside the boolean
condition in the literal schema logic, which breaks readability. Remove the
empty line from the condition in the relevant expression in literal.ts, keeping
the boolean check contiguous and compact so the surrounding logic remains easy
to scan.
- Around line 100-104: The NaN/signed-zero comparison in literal matching is
correct, but the intentional self-comparison triggers Biome’s noSelfCompare
warning. In the `Literal` schema’s equality check, add a targeted Biome ignore
comment directly above the `dataset.value !== dataset.value && this.literal !==
this.literal` NaN guard to document that the self-comparison is deliberate. Keep
the existing `Literal` logic unchanged and place the suppression close to the
`dataset.value`/`this.literal` comparison so future readers understand the
intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 547d4157-3704-4529-9275-b4440b803b9f
📒 Files selected for processing (2)
library/src/schemas/literal/literal.test.tslibrary/src/schemas/literal/literal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- library/src/schemas/literal/literal.test.ts
commit: |
|
Quick clarification on the automated note above: this uses SameValueZero, not dataset.value === this.literal ||
(dataset.value !== dataset.value && this.literal !== this.literal)So |
|
@spokodev Thanks for your input. Please use Prettier for your changes I believe the fix is correct; we'll discuss it soon wit Fabian |
|
Ran Prettier — only a stray blank line in the condition changed. No rush on the review. |
Problem
literal(NaN)is an unsatisfiable schema: it rejects every input, including theNaNit was constructed from.NaNis a validnumber, andLiteral = bigint | boolean | number | string | symbol, soliteral(NaN)is a fully type-checked construction. The runtime can never succeed because the value check uses===, andNaN === NaNisfalse.The sibling
picklistschema disagrees, because it compares withArray.includes(SameValueZero):So
literalandpicklistgive opposite answers for the same value, which points toliteral's===being an oversight rather than a deliberate choice.Fix
library/src/schemas/literal/literal.tscomparesdataset.value === this.literal. Switch that single comparison toObject.is:Object.is(NaN, NaN)istrue, soliteral(NaN)now acceptsNaN.This matches the equality migration already in flight: #1517 moves
_merge(used byintersect) toObject.is, and #1477 movesvalue/notValue/values/notValuestoObject.is. Neither of those touches theliteralschema, so this closes the same NaN gap one layer up.Trade-off
Like #1517 and #1477,
Object.isalso distinguishes-0from+0. After this changeliteral(0)no longer accepts-0(under===they were treated as equal). This is consistent with the direction of those PRs. If matchingpicklist's SameValueZero (which keeps-0/+0loosely equal) is preferred instead, I can switch to that.Tests
Added one case to
library/src/schemas/literal/literal.test.ts:It fails on
main(literal(NaN)returns an issue) and passes with the fix. Fulllibrary/suite stays green: 272 files / 3057 tests.Summary by CodeRabbit
literalschema matching for special numeric values:literal(NaN)now correctly matches datasets containingNaN, and signed zero is handled so-0and+0are treated as equivalent.NaNproduce no schema issues when validating against a matchingliteral(NaN)schema.