-
Notifications
You must be signed in to change notification settings - Fork 65
fix and-narrowing (#1169) #1170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,8 +113,18 @@ export default function emberTemplateImportsEnvironment( | |
| // discriminated unions — e.g. `{{#if (eq foo.kind "a")}}` narrows | ||
| // `foo` to the matching variant inside the block, which a | ||
| // boolean-returning helper type cannot do. | ||
| // | ||
| // The same reasoning applies to `and`/`or`: emitting them as the | ||
| // native `&&`/`||` operators lets `{{#if (and (isNumber x) ...)}}` | ||
| // propagate the type predicates of its operands into the block, which | ||
| // a value-returning helper type cannot do (typed-ember/glint#1169). | ||
| ...(hasEmber71BuiltIns() | ||
| ? ({ eq: '===', neq: '!==' } satisfies GlintSpecialFormConfig['globals']) | ||
| ? ({ | ||
| eq: '===', | ||
| neq: '!==', | ||
| and: '&&', | ||
| or: '||', | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note This reply is from Claude This is the line that actually fixes #1169. It has to be a native operator: a |
||
| } satisfies GlintSpecialFormConfig['globals']) | ||
| : {}), | ||
| ...additionalGlobalSpecialForms, | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -242,8 +242,16 @@ export function templateToTypescript( | |
| formInfo: SpecialFormInfo, | ||
| node: AST.MustacheStatement | AST.SubExpression, | ||
| position: InvokePosition, | ||
| inTestPosition = false, | ||
| ): void { | ||
| if (formInfo.requiresConsumption) { | ||
| // Logical forms (`&&`/`||`) manage their own keyword consumption: wrapping | ||
| // the whole operator chain in a `(noop(kw), <chain>)` comma defeats | ||
| // TypeScript's control-flow narrowing of an `&&`/`||` chain of type | ||
| // guards. Instead they ride the consumption on the first operand only | ||
| // (see `emitLogicalExpression`), keeping the chain at the top level. | ||
| const isLogicalForm = formInfo.form === '&&' || formInfo.form === '||'; | ||
|
|
||
| if (formInfo.requiresConsumption && !isLogicalForm) { | ||
| mapper.text('(__glintDSL__.noop('); | ||
| emitExpression(node.path); | ||
| mapper.text('), '); | ||
|
|
@@ -281,7 +289,7 @@ export function templateToTypescript( | |
|
|
||
| case '&&': | ||
| case '||': | ||
| emitLogicalExpression(formInfo, node); | ||
| emitLogicalExpression(formInfo, node, inTestPosition); | ||
| break; | ||
|
|
||
| case '!': | ||
|
|
@@ -293,7 +301,7 @@ export function templateToTypescript( | |
| mapper.text('undefined'); | ||
| } | ||
|
|
||
| if (formInfo.requiresConsumption) { | ||
| if (formInfo.requiresConsumption && !isLogicalForm) { | ||
| mapper.text(')'); | ||
| } | ||
| } | ||
|
|
@@ -424,7 +432,7 @@ export function templateToTypescript( | |
| ); | ||
|
|
||
| mapper.text('('); | ||
| emitExpression(node.params[0]); | ||
| emitExpression(node.params[0], true); | ||
| mapper.text(') ? ('); | ||
| emitExpression(node.params[1]); | ||
| mapper.text(') : ('); | ||
|
|
@@ -450,7 +458,7 @@ export function templateToTypescript( | |
| ); | ||
|
|
||
| mapper.text('!('); | ||
| emitExpression(node.params[0]); | ||
| emitExpression(node.params[0], true); | ||
| mapper.text(') ? ('); | ||
| emitExpression(node.params[1]); | ||
| mapper.text(') : ('); | ||
|
|
@@ -492,20 +500,47 @@ export function templateToTypescript( | |
| function emitLogicalExpression( | ||
| formInfo: SpecialFormInfo, | ||
| node: AST.MustacheStatement | AST.SubExpression, | ||
| inTestPosition: boolean, | ||
| ): void { | ||
| mapper.forNode(node, () => { | ||
| assert( | ||
| node.hash.pairs.length === 0, | ||
| () => `{{${formInfo.name}}} only accepts positional parameters`, | ||
| ); | ||
| assert( | ||
| node.params.length >= 2, | ||
| () => `{{${formInfo.name}}} requires at least two parameters`, | ||
| ); | ||
|
|
||
| if (!inTestPosition || node.params.length < 2) { | ||
| // Outside a condition we emit the keyword as a helper invocation | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note This reply is from Claude This branch is the heart of the design, and the subtlety worth understanding:
Net effect: narrowing in conditions, runtime-accurate types everywhere else. The cost is that an empty array is treated as truthy in a condition — the same approximation glint already makes for plain |
||
| // rather than a native operator chain. The helper's return type | ||
| // (`FirstFalsy`/`FirstTruthy`) reproduces Ember's runtime truthiness — | ||
| // e.g. empty arrays and `{ isTruthy: false }` are falsy — which the | ||
| // JavaScript `&&`/`||` operators do not. The helper also enforces the | ||
| // "at least two operands" requirement as an ordinary type error, so it | ||
| // handles the too-few-operands case too. (This matches what the | ||
| // keyword would emit if it were a plain helper rather than a special | ||
| // form, so value-position behavior is unchanged.) | ||
| emitResolve(node, 'resolve'); | ||
| return; | ||
| } | ||
|
|
||
| // In a condition, emit the native `&&`/`||` operator chain so that | ||
| // TypeScript's control-flow analysis narrows the operands' type guards | ||
| // inside the block (e.g. `{{#if (and (isNumber x) (isNumber y))}}`). | ||
| mapper.text('('); | ||
| for (const [index, param] of node.params.entries()) { | ||
| emitExpression(param); | ||
| if (index === 0 && formInfo.requiresConsumption) { | ||
| // Ride the keyword consumption on the first operand | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note This reply is from Claude A glint detail that makes this look stranger than it is. For a documented keyword, glint emits a throwaway reference to it ( The non-obvious part is where that reference goes. Wrapping the whole chain as |
||
| // (`(noop(kw), <first>) && <rest>`) so the operator chain stays at | ||
| // the top level of the expression. Wrapping the whole chain in the | ||
| // comma instead would discard TypeScript's narrowing of an `&&`/`||` | ||
| // chain of type guards. | ||
| mapper.text('(__glintDSL__.noop('); | ||
| emitExpression(node.path); | ||
| mapper.text('), '); | ||
| emitExpression(param); | ||
| mapper.text(')'); | ||
| } else { | ||
| emitExpression(param); | ||
| } | ||
|
|
||
| if (index < node.params.length - 1) { | ||
| mapper.text(` ${formInfo.form} `); | ||
|
|
@@ -570,13 +605,18 @@ export function templateToTypescript( | |
| return null; | ||
| } | ||
|
|
||
| function emitExpression(node: AST.Expression): void { | ||
| // `inTestPosition` marks expressions emitted as the condition of an | ||
| // `if`/`unless`. It only affects `and`/`or`: in a condition they emit as | ||
| // native `&&`/`||` so TypeScript narrows their operands, whereas in any | ||
| // other (value) position they emit as helper invocations to preserve their | ||
| // exact runtime return type (see `emitLogicalExpression`). | ||
| function emitExpression(node: AST.Expression, inTestPosition = false): void { | ||
| switch (node.type) { | ||
| case 'PathExpression': | ||
| return emitPath(node); | ||
|
|
||
| case 'SubExpression': | ||
| return emitSubExpression(node); | ||
| return emitSubExpression(node, inTestPosition); | ||
|
|
||
| case 'BooleanLiteral': | ||
| case 'NullLiteral': | ||
|
|
@@ -1157,7 +1197,7 @@ export function templateToTypescript( | |
| ); | ||
|
|
||
| mapper.text('if ('); | ||
| emitExpression(node.params[0]); | ||
| emitExpression(node.params[0], true); | ||
| mapper.text(') {'); | ||
| mapper.newline(); | ||
| mapper.indent(); | ||
|
|
@@ -1191,7 +1231,7 @@ export function templateToTypescript( | |
| ); | ||
|
|
||
| mapper.text('if (!('); | ||
| emitExpression(node.params[0]); | ||
| emitExpression(node.params[0], true); | ||
| mapper.text(')) {'); | ||
| mapper.newline(); | ||
| mapper.indent(); | ||
|
|
@@ -1309,10 +1349,10 @@ export function templateToTypescript( | |
| scope.pop(); | ||
| } | ||
|
|
||
| function emitSubExpression(node: AST.SubExpression): void { | ||
| function emitSubExpression(node: AST.SubExpression, inTestPosition = false): void { | ||
| let specialFormInfo = checkSpecialForm(node); | ||
| if (specialFormInfo) { | ||
| emitSpecialFormExpression(specialFormInfo, node, 'sexpr'); | ||
| emitSpecialFormExpression(specialFormInfo, node, 'sexpr', inTestPosition); | ||
| return; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having and / or be in this list makes me nervous, because it's a deviation from runtime / the RFC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
This reply is from Claude
The nervousness is justified, so here's the precise scope of the deviation — it's narrower and more asymmetric than "and/or now deviate from runtime":
Value position: no deviation. This PR only emits the operator in an
if/unlesscondition. In every other positionand/orstill emit as the helper invocation, so their return type (FirstFalsy/FirstTruthy) matches the RFC /ember-truth-helpersexactly — empty array falsy,isTruthyhonored,(or [] x)types asx. That behavior is unchanged by this PR.Condition position: a bounded truthiness deviation only.
{{#if (and a b)}}becomesif (a && b), which uses JavaScript truthiness — so an empty array reads as truthy, where Ember'stoBoolwould read it as falsy. Two things bound how much this matters:{{#if anArray}}already emitsif (anArray)(JS-truthy empty array), not EmbertoBool. Soand/orin a condition deviate to exactly the degreeif/unlessalready do — this PR doesn't introduce a new kind of inaccuracy, it extends an existing one.booleans, and JS vs. Ember truthiness agree on booleans. The empty-array/proxy divergence only appears with non-boolean operands — i.e. the cases where you weren't narrowing anyway.Why the operator is unavoidable for the fix. Narrowing is a control-flow property of the emitted TS, and a TS type predicate (
x is T) can only narrow a function's own parameter. Whenandis a helper call, the tested variable lives inside its argument, already collapsed toboolean— so noandsignature (generic, callback, assertion) can recover it. Native&&in the emitted condition is the only construct TS will narrow through. (Same reasoneq/neqare emitted as===/!==rather than typed helpers.)So the real question to decide is narrow: is the existing
{{#if}}truthiness approximation acceptable to extend toand/orconditions, in exchange for narrowing? If not, the alternative is to keepand/oras helpers everywhere and close #1169 as "use nested{{#if}}s" — there's no third option where a helper call also narrows.