fix(types): reject symbol operands across arithmetic/comparison/unary operators#1261
Open
nickna wants to merge 5 commits into
Open
fix(types): reject symbol operands across arithmetic/comparison/unary operators#1261nickna wants to merge 5 commits into
nickna wants to merge 5 commits into
Conversation
…rators Root cause: Symbol.for(key) typed as Any instead of symbol. Symbol() (bare call) was already special-cased to return TypeInfo.Symbol, but Symbol.for's callee shape (Expr.Get on the Symbol global, not a bare Expr.Variable call) didn't match that special case, so it fell through to evaluating the bare `Symbol` identifier (which resolves to Any for unrelated reasons) and calling `.for` on Any stayed Any. Every arithmetic/comparison/unary operand check downstream silently no-oped as a result, since Any bypasses all of them. Added Symbol.for/keyFor alongside the existing Symbol() case. With Symbol.for correctly typed, the operator checks themselves turned out to be missing or using the wrong diagnostic code for symbol operands specifically, across nearly every operator category: - Arithmetic/bitwise/unsigned-shift (*, /, %, <<, >>, >>>, &, ^, |) and their compound-assign forms: tsc reports each invalid operand independently (TS2362 left, TS2363 right — both can fire on one expression). New shared RecordInvalidArithmeticOperand helper. - Comparison (<, >, <=, >=): symbol gets its own single-diagnostic message (TS2469) instead of the generic two-type TS2365. - Unary -/~: same TS2469 branch. Unary + had NO handling at all (silently passed any operand through unchanged) — added the missing branch, scoped to just the symbol rejection. - Binary + and += (CheckPlusOperator/CheckCompoundAssign): most nuanced — tsc's message choice depends on the other operand type (symbol+symbol or symbol+number get the generic TS2365; symbol paired with string/any/a union merely containing symbol get the symbol-specific TS2469). New ContainsSymbolType helper (true for a union with *any* symbol constituent, unlike an all-constituents check) and IsBareNumberType (deliberately not Any-permissive, so `any` isn't mistaken for "the other side was a number"). Also fixes prefix/postfix increment/decrement's line attribution (no explicit line was passed, so the diagnostic landed on whatever statement `_currentStatementLine` happened to be instead of the ++/-- itself) and widens them to accept bigint per tsc's own message wording. Found and fixed a real regression along the way: an early version of RecordInvalidArithmeticOperand only recorded diagnostics (never threw), which silently broke exception-based single-shot checking (two existing xUnit tests, and the default interpreter path in general) for ANY invalid arithmetic operand, not just symbol. Fixed by recording the diagnostic that won't be thrown directly, then throwing the other — single-shot callers still see an exception, and recovery mode's per-statement catch still ends up with both diagnostics. Removed IsSymbolType as dead code once superseded by ContainsSymbolType everywhere; its Any-permissive construction had also caused a second bug (any wrongly "matched" the symbol predicate). Flips symbolType4/5/6/7/8/10/12 from Fail to Pass, 0 regressions.
209 -> 216 Pass. 0 regressions.
…bodies
A bare `?`-optional parameter (`function f(x?: T)`) was bound inside the
function/method/arrow BODY using its raw declared annotation type `T`,
never `T | undefined` — even though the caller can genuinely omit it.
`function f(x?: string) { x.toUpperCase() }` produced zero diagnostics,
while the explicit-union equivalent `function f(x: string | undefined)`
correctly flagged it. The signature type shown externally (`f?: T`, used
for arity/display) was never wrong; only the in-body binding was.
Root cause: BuildFunctionSignature (TypeChecker.cs) resolves each
parameter's declared type once and reuses it both for the function's own
callable signature and for defining the parameter in the checking scope.
A defaulted parameter (`x: T = default`) is correctly NOT widened — the
default substitutes a same-typed value before the body ever observes it,
so it's genuinely never undefined there.
Fix: new WidenOptionalParamsForBody helper, applied at every body-binding
call site (top-level functions, arrow/function expressions and object
method shorthand, and class methods in both the class-declaration and
class-expression paths — a constructor is just a method named
"constructor" so it's covered by the same fix).
…22/2723) A non-optional call (`f(42)`, not `f?.(42)`) on a callee whose type is a nullable union previously never errored: CheckCall's union-of-function- members branch explicitly added `undefined` to the result type instead of rejecting the call, on the theory that "calling undefined would fail at runtime, but for type checking we allow it" — which doesn't match tsc, which always rejects this. Adds the missing check right after the existing optional-call nullish- stripping logic: a non-optional call on a union containing null and/or undefined now throws TS2721 (null), TS2722 (undefined), or TS2723 (both), matching tsc's exact diagnostic selection. The now-unreachable "allow calling undefined" branch is removed as dead code — a non-optional call on a nullable union throws before reaching it, and an optional call has already stripped null/undefined by that point.
`x == null` / `x != null` / `x == undefined` / `x != undefined` (loose equality) never narrowed at all — only strict `===`/`!==` and truthiness did. This is a long-standing, general narrowing gap, unrelated to any specific type; it was simply never exercised by an existing test because nothing produced a genuinely-nullable value through this exact idiom until the optional-parameter body-widening fix started doing so — stdlib/node/path.ts's `ext != null && ext.length > 0` (ext is a bare optional parameter) newly failed to type-check once `ext` correctly became `string | undefined` in the body, surfacing this pre-existing gap as a 285-test xUnit regression. Root cause: AnalyzeNullCheck and AnalyzePathNullCheck's `checkingForNull` flag only ever excluded ONE of null/undefined (whichever literal was compared), with no way to express "exclude both" — the callers pass the same flag regardless of whether the source comparison was loose or strict. JS's abstract equality treats null and undefined as mutually `==`-equal, so `x == null` must exclude BOTH from the union, unlike `x === null` which excludes only null. Both helpers gain a `checkBoth` parameter, threaded from the call sites in AnalyzeTypeGuard and AnalyzePathTypeGuard based on whether the operator is the loose (EQUAL_EQUAL/BANG_EQUAL) or strict (EQUAL_EQUAL_EQUAL/BANG_EQUAL_EQUAL) variant. AnalyzePathTypeGuard was also missing `undefined`-literal patterns entirely (only `null`-literal comparisons were recognized for path/property narrowing) — added to match the existing bare-variable patterns.
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
1 participant
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.
Summary
Continuation of epic #99 / #80 Track-1 (TS conformance), building on the now-merged #1259/#1260.
Symbol operand rejection (arithmetic/comparison/unary/compound-assign)
Root cause:
Symbol.for(key)was typed asAnyinstead ofsymbol.Symbol()(bare call) was already special-cased to returnTypeInfo.Symbol, butSymbol.for's callee shape (Expr.Geton theSymbolglobal, not a bareExpr.Variablecall) didn't match that special case, so it fell through to evaluating the bareSymbolidentifier (which resolves toAnyfor unrelated reasons) and calling.foronAnystayedAny. Every arithmetic/comparison/unary operand check downstream silently no-oped as a result. AddedSymbol.for/Symbol.keyForalongside the existingSymbol()case.With
Symbol.forcorrectly typed, the operator checks themselves turned out to be missing or using the wrong diagnostic code forsymboloperands specifically, across nearly every operator category (arithmetic/bitwise/shift TS2362/2363-split, comparison/unary TS2469, nuanced binary+/+=). Also fixes prefix/postfix increment/decrement's line attribution and widens them to acceptbigint.Result: flips symbolType4/5/6/7/8/10/12 (7 tests) from Fail to Pass.
Optional-parameter body-widening + TS2721/2722/2723 + loose-equality narrowing
Investigating
logicalAssignment5(needs TS2722 "cannot invoke possibly-undefined") surfaced a much bigger, foundational gap: a bare?-optional parameter (function f(x?: T)) was bound inside the function body using its raw declared typeT, neverT | undefined, even though the caller can genuinely omit it —function f(x?: string) { x.toUpperCase() }produced zero diagnostics, while the explicit-union equivalent correctly errored. Fixed via a newWidenOptionalParamsForBodyhelper applied at every body-binding site (functions, arrows/function expressions/object method shorthand, class methods in both class-declaration and class-expression paths).Also added the missing TS2721/2722/2723 ("cannot invoke an object which is possibly 'null'/'undefined'/'null' or 'undefined'") checks for non-optional calls on a nullable callee — previously silently allowed.
Widening optional parameters exposed a 285-test xUnit regression: our own bundled stdlib (
stdlib/node/path.ts) uses the idiomaticext != null && ext.length > 0on an optional parameter, which never actually narrowed awayundefined— loose equality (==/!=) againstnull/undefinedwasn't recognized as a narrowing pattern at all (only strict===/!==and truthiness were). This is a general, pre-existing gap unrelated to any specific type; it was simply never exercised until optional parameters became genuinely nullable. Fixed bothAnalyzeNullCheckandAnalyzePathNullCheckto narrow away both null and undefined on a loose comparison against either literal (matching JS'snull == undefined), and added the previously-entirely-missingundefined-literal patterns toAnalyzePathTypeGuard.Left open:
logicalAssignment5's comma-expression case (f ??= (f.toString(), (a=>a))) still doesn't fully match tsc —f.toString()throwing during RHS evaluation aborts the rest of the statement (including narrowingfafterward) before the recovery wrapper catches it. Fixing this needs a separate, deeper architectural change to let type inference continue past an internal throw within one statement; out of scope here.Test plan
dotnet test(SharpTS.Tests) — 15452/15452 passing, 0 failuresSharpTS.Test262interpreted + compiled baselines — 0 failuresSharpTS.TypeScriptConformancebaseline — 216 Pass / 62 Fail / 0 ParseError, 0 regressions vs. committed baseline