Skip to content
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

add jsx fragments to callLikeExpression #59933

Merged
merged 19 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
344 changes: 208 additions & 136 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

5 changes: 4 additions & 1 deletion src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3976,6 +3976,10 @@
"category": "Error",
"code": 2878
},
"Using JSX fragments requires fragment factory '{0}' to be in scope, but it could not be found.": {
"category": "Error",
"code": 2879
},

"Import declaration '{0}' is using private name '{1}'.": {
"category": "Error",
Expand Down Expand Up @@ -5530,7 +5534,6 @@
"category": "Message",
"code": 6243
},

"Modules": {
"category": "Message",
"code": 6244
Expand Down
9 changes: 6 additions & 3 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3126,7 +3126,7 @@ export type CallLikeExpression =
| NewExpression
| TaggedTemplateExpression
| Decorator
| JsxOpeningLikeElement
| JsxCallLike
| InstanceofExpression;

export interface AsExpression extends Expression {
Expand Down Expand Up @@ -3187,6 +3187,10 @@ export type JsxOpeningLikeElement =
| JsxSelfClosingElement
| JsxOpeningElement;

export type JsxCallLike =
| JsxOpeningLikeElement
| JsxOpeningFragment;
Comment on lines +3190 to +3192
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldnt it make sense to include JsxOpeningFragment as part of JsxOpeningLikeElement?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iisaduan can you address that suggestion?

Copy link
Member Author

@iisaduan iisaduan Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay in response. It's a good question and I wanted something detailed for PR description.

TLDR keeping a type that has only opening element and self closing element, is still beneficial, and adding JsxOpeningFragment to JsxOpeningLikeElement requires a much bigger change than is needed to add this fragment type check. The biggest reason is because this add causes crashes in expression checking. We would either need to rework what fragments are, in a way that is making them almost identical to an JsxOpeningElement, or we would need to update the majority of places JsxOpeningLikeElement is referenced to ignore fragments anyway (and if we don't want to ignore them, we'd need to handle fragments specially, which could cause us to do redundant/unnecessary work).

In my first iteration of this PR, I did try to add fragments to openingLike, and the biggest issue was that we run into crashes during expression checking (probably due to several series of no longer correct assertions). Since the fragment check is now fully added, I tried again to fix these crashes over the last day or so, but I still ran into layers of them, so it just ends up being much more work than is necessary for this type check.

Also regarding the use of JsxOpeningLikeElement-- they have many more checks than what we need to check fragments, so adding fragments would require us to then ignore fragments in many uses of openinglike. For example, after this PR, just in checker, there are ~30 functions that would still error because of fragment incompatibilty with what JsxOpeningLikeElement is currently. These functions would almost always be updated to ignore fragments anyways, since ~20 are related to checking tagName or attributes. While the rest may have checks applicable to fragments, they aren't performing any different checks than what we already do (so we should probably ignore fragments because we don't need to do the work for them again). This is without taking into account other parts of the compiler/services, and anyone else using JsxOpeningLikeElement


export type JsxAttributeLike =
| JsxAttribute
| JsxSpreadAttribute;
Expand Down Expand Up @@ -6208,19 +6212,18 @@ export interface NodeLinks {
resolvedType?: Type; // Cached type of type node
resolvedSignature?: Signature; // Cached signature of signature node or call expression
resolvedSymbol?: Symbol; // Cached name resolution result
resolvedIndexInfo?: IndexInfo; // Cached indexing info resolution result
effectsSignature?: Signature; // Signature with possible control flow effects
enumMemberValue?: EvaluatorResult; // Constant value of enum member
isVisible?: boolean; // Is this node visible
containsArgumentsReference?: boolean; // Whether a function-like declaration contains an 'arguments' reference
hasReportedStatementInAmbientContext?: boolean; // Cache boolean if we report statements in ambient context
jsxFlags: JsxFlags; // flags for knowing what kind of element/attributes we're dealing with
resolvedJsxElementAttributesType?: Type; // resolved element attributes type of a JSX openinglike element
resolvedJsxElementAllAttributesType?: Type; // resolved all element attributes type of a JSX openinglike element
resolvedJSDocType?: Type; // Resolved type of a JSDoc type reference
switchTypes?: Type[]; // Cached array of switch case expression types
jsxNamespace?: Symbol | false; // Resolved jsx namespace symbol for this node
jsxImplicitImportContainer?: Symbol | false; // Resolved module symbol the implicit jsx import of this file should refer to
jsxFragmentType?: Type; // Type of the JSX fragment element, set per SourceFile if a jsxFragment is checked in the file
contextFreeType?: Type; // Cached context-free type used by the first pass of inference; used when a function's return is partially contextually sensitive
deferredNodes?: Set<Node>; // Set of nodes whose checking has been deferred
capturedBlockScopeBindings?: Symbol[]; // Block-scoped bindings captured beneath this part of an IterationStatement
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3383,6 +3383,8 @@ export function getInvokedExpression(node: CallLikeExpression): Expression | Jsx
return node.tagName;
case SyntaxKind.BinaryExpression:
return node.right;
case SyntaxKind.JsxOpeningFragment:
return node;
default:
return node.expression;
}
Expand Down
15 changes: 13 additions & 2 deletions src/compiler/utilitiesPublic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ import {
JSDocTypedefTag,
JSDocTypeTag,
JsxAttributeLike,
JsxCallLike,
JsxChild,
JsxExpression,
JsxOpeningLikeElement,
Expand Down Expand Up @@ -1957,13 +1958,16 @@ export function isCallLikeOrFunctionLikeExpression(node: Node): node is CallLike

export function isCallLikeExpression(node: Node): node is CallLikeExpression {
switch (node.kind) {
case SyntaxKind.JsxOpeningElement:
case SyntaxKind.JsxSelfClosingElement:
case SyntaxKind.CallExpression:
case SyntaxKind.NewExpression:
case SyntaxKind.TaggedTemplateExpression:
case SyntaxKind.Decorator:
case SyntaxKind.JsxOpeningElement:
case SyntaxKind.JsxSelfClosingElement:
case SyntaxKind.JsxOpeningFragment:
return true;
case SyntaxKind.BinaryExpression:
return (node as BinaryExpression).operatorToken.kind === SyntaxKind.InstanceOfKeyword;
default:
return false;
}
Expand Down Expand Up @@ -2479,6 +2483,13 @@ export function isJsxOpeningLikeElement(node: Node): node is JsxOpeningLikeEleme
|| kind === SyntaxKind.JsxSelfClosingElement;
}

export function isJsxCallLike(node: Node): node is JsxCallLike {
const kind = node.kind;
return kind === SyntaxKind.JsxOpeningElement
|| kind === SyntaxKind.JsxSelfClosingElement
|| kind === SyntaxKind.JsxOpeningFragment;
}

// Clauses

export function isCaseOrDefaultClause(node: Node): node is CaseOrDefaultClause {
Expand Down
4 changes: 3 additions & 1 deletion tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5148,7 +5148,7 @@ declare namespace ts {
interface InstanceofExpression extends BinaryExpression {
readonly operatorToken: Token<SyntaxKind.InstanceOfKeyword>;
}
type CallLikeExpression = CallExpression | NewExpression | TaggedTemplateExpression | Decorator | JsxOpeningLikeElement | InstanceofExpression;
type CallLikeExpression = CallExpression | NewExpression | TaggedTemplateExpression | Decorator | JsxCallLike | InstanceofExpression;
interface AsExpression extends Expression {
readonly kind: SyntaxKind.AsExpression;
readonly expression: Expression;
Expand Down Expand Up @@ -5184,6 +5184,7 @@ declare namespace ts {
readonly closingElement: JsxClosingElement;
}
type JsxOpeningLikeElement = JsxSelfClosingElement | JsxOpeningElement;
type JsxCallLike = JsxOpeningLikeElement | JsxOpeningFragment;
type JsxAttributeLike = JsxAttribute | JsxSpreadAttribute;
type JsxAttributeName = Identifier | JsxNamespacedName;
type JsxTagNameExpression = Identifier | ThisExpression | JsxTagNamePropertyAccess | JsxNamespacedName;
Expand Down Expand Up @@ -8800,6 +8801,7 @@ declare namespace ts {
function isJsxAttributeLike(node: Node): node is JsxAttributeLike;
function isStringLiteralOrJsxExpression(node: Node): node is StringLiteral | JsxExpression;
function isJsxOpeningLikeElement(node: Node): node is JsxOpeningLikeElement;
function isJsxCallLike(node: Node): node is JsxCallLike;
function isCaseOrDefaultClause(node: Node): node is CaseOrDefaultClause;
/** True if node is of a kind that may contain comment text. */
function isJSDocCommentContainingNode(node: Node): boolean;
Expand Down
15 changes: 12 additions & 3 deletions tests/baselines/reference/inlineJsxAndJsxFragPragma.errors.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
preacty-no-fragment.tsx(5,12): error TS6133: 'Fragment' is declared but its value is never read.
preacty-only-fragment-no-jsx.tsx(6,1): error TS2874: This JSX tag requires 'h' to be in scope, but it could not be found.
snabbdomy-only-fragment-no-jsx.tsx(4,1): error TS2874: This JSX tag requires 'jsx' to be in scope, but it could not be found.
snabbdomy-only-fragment-no-jsx.tsx(4,1): error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.
snabbdomy-only-fragment.tsx(4,1): error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.
snabbdomy.tsx(4,1): error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.


==== renderer.d.ts (0 errors) ====
Expand All @@ -23,11 +26,13 @@ snabbdomy-only-fragment-no-jsx.tsx(4,1): error TS2874: This JSX tag requires 'js
import {h, Fragment} from "./renderer";
<><div></div></>

==== snabbdomy.tsx (0 errors) ====
==== snabbdomy.tsx (1 errors) ====
/* @jsx jsx */
/* @jsxfrag null */
import {jsx} from "./renderer";
<><span></span></>
~~
!!! error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.

==== preacty-only-fragment.tsx (0 errors) ====
/**
Expand All @@ -37,11 +42,13 @@ snabbdomy-only-fragment-no-jsx.tsx(4,1): error TS2874: This JSX tag requires 'js
import {h, Fragment} from "./renderer";
<></>

==== snabbdomy-only-fragment.tsx (0 errors) ====
==== snabbdomy-only-fragment.tsx (1 errors) ====
/* @jsx jsx */
/* @jsxfrag null */
import {jsx} from "./renderer";
<></>
~~
!!! error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.

==== preacty-only-fragment-no-jsx.tsx (1 errors) ====
/**
Expand All @@ -53,13 +60,15 @@ snabbdomy-only-fragment-no-jsx.tsx(4,1): error TS2874: This JSX tag requires 'js
~~
!!! error TS2874: This JSX tag requires 'h' to be in scope, but it could not be found.

==== snabbdomy-only-fragment-no-jsx.tsx (1 errors) ====
==== snabbdomy-only-fragment-no-jsx.tsx (2 errors) ====
/* @jsx jsx */
/* @jsxfrag null */
import {} from "./renderer";
<></>
~~
!!! error TS2874: This JSX tag requires 'jsx' to be in scope, but it could not be found.
~~
!!! error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.

==== preacty-no-fragment.tsx (1 errors) ====
/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
snabbdomy.tsx(6,1): error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.


==== react.d.ts (0 errors) ====
declare global {
namespace JSX {
interface IntrinsicElements {
[e: string]: any;
}
}
}
export function createElement(): void;
export function Fragment(): void;

==== preact.d.ts (0 errors) ====
export function h(): void;
export function Frag(): void;

==== snabbdom.d.ts (0 errors) ====
export function h(): void;

==== reacty.tsx (0 errors) ====
import {createElement, Fragment} from "./react";
<><span></span></>

==== preacty.tsx (0 errors) ====
/**
* @jsx h
* @jsxFrag Frag
*/
import {h, Frag} from "./preact";
<><div></div></>

==== snabbdomy.tsx (1 errors) ====
/**
* @jsx h
* @jsxfrag null
*/
import {h} from "./snabbdom";
<><div></div></>
~~
!!! error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.

==== mix-n-match.tsx (0 errors) ====
/* @jsx h */
/* @jsxFrag Fragment */
import {h} from "./preact";
import {Fragment} from "./react";
<><span></span></>
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ import {createElement, Fragment} from "./react";
> : ^^^^^^

<><span></span></>
><><span></span></> : error
><span></span> : error
><><span></span></> : any
> : ^^^
><span></span> : any
> : ^^^
>span : any
> : ^^^
>span : any
Expand All @@ -62,8 +64,10 @@ import {h, Frag} from "./preact";
> : ^^^^^^

<><div></div></>
><><div></div></> : error
><div></div> : error
><><div></div></> : any
> : ^^^
><div></div> : any
> : ^^^
>div : any
> : ^^^
>div : any
Expand All @@ -79,8 +83,10 @@ import {h} from "./snabbdom";
> : ^^^^^^

<><div></div></>
><><div></div></> : error
><div></div> : error
><><div></div></> : any
> : ^^^
><div></div> : any
> : ^^^
>div : any
> : ^^^
>div : any
Expand All @@ -98,8 +104,10 @@ import {Fragment} from "./react";
> : ^^^^^^

<><span></span></>
><><span></span></> : error
><span></span> : error
><><span></span></> : any
> : ^^^
><span></span> : any
> : ^^^
>span : any
> : ^^^
>span : any
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
index.tsx(3,1): error TS2874: This JSX tag requires 'React' to be in scope, but it could not be found.
index.tsx(3,1): error TS2879: Using JSX fragments requires fragment factory 'React' to be in scope, but it could not be found.
index.tsx(3,1): error TS17017: An @jsxFrag pragma is required when using an @jsx pragma with JSX fragments.
reacty.tsx(3,1): error TS17017: An @jsxFrag pragma is required when using an @jsx pragma with JSX fragments.

Expand All @@ -19,11 +20,13 @@ reacty.tsx(3,1): error TS17017: An @jsxFrag pragma is required when using an @js
<><h></h></>
~~~~~~~~~~~~
!!! error TS17017: An @jsxFrag pragma is required when using an @jsx pragma with JSX fragments.
==== index.tsx (2 errors) ====
==== index.tsx (3 errors) ====
/** @jsx dom */
import { dom } from "./renderer";
<><h></h></>
~~
!!! error TS2874: This JSX tag requires 'React' to be in scope, but it could not be found.
~~
!!! error TS2879: Using JSX fragments requires fragment factory 'React' to be in scope, but it could not be found.
~~~~~~~~~~~~
!!! error TS17017: An @jsxFrag pragma is required when using an @jsx pragma with JSX fragments.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
jsxFactoryAndJsxFragmentFactoryNull.tsx(3,1): error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.


==== jsxFactoryAndJsxFragmentFactoryNull.tsx (1 errors) ====
declare var h: any;

<></>;
~~
!!! error TS2879: Using JSX fragments requires fragment factory 'null' to be in scope, but it could not be found.
<><span>1</span><><span>2.1</span><span>2.2</span></></>;
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,31 @@
=== jsxFactoryAndJsxFragmentFactoryNull.tsx ===
declare var h: any;
>h : any
> : ^^^

<></>;
><></> : error
><></> : any
> : ^^^

<><span>1</span><><span>2.1</span><span>2.2</span></></>;
><><span>1</span><><span>2.1</span><span>2.2</span></></> : error
><span>1</span> : error
><><span>1</span><><span>2.1</span><span>2.2</span></></> : any
> : ^^^
><span>1</span> : any
> : ^^^
>span : any
> : ^^^
>span : any
> : ^^^
><><span>2.1</span><span>2.2</span></> : error
><span>2.1</span> : error
><><span>2.1</span><span>2.2</span></> : any
> : ^^^
><span>2.1</span> : any
> : ^^^
>span : any
> : ^^^
>span : any
> : ^^^
><span>2.2</span> : error
><span>2.2</span> : any
> : ^^^
>span : any
> : ^^^
>span : any
Expand Down
22 changes: 22 additions & 0 deletions tests/baselines/reference/jsxFragmentWrongType.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
a.tsx(6,28): error TS2322: Type '{ children: () => string; }' is not assignable to type '{ children?: ReactNode; }'.
Types of property 'children' are incompatible.
Type '() => string' is not assignable to type 'ReactNode'.
a.tsx(7,47): error TS2322: Type '() => string' is not assignable to type 'ReactNode'.


==== a.tsx (2 errors) ====
/// <reference path="/.lib/react18/react18.d.ts" />
/// <reference path="/.lib/react18/global.d.ts" />

const test = () => "asd";

const jsxWithJsxFragment = <>{test}</>;
~~
!!! error TS2322: Type '{ children: () => string; }' is not assignable to type '{ children?: ReactNode; }'.
!!! error TS2322: Types of property 'children' are incompatible.
!!! error TS2322: Type '() => string' is not assignable to type 'ReactNode'.
const jsxWithReactFragment = <React.Fragment>{test}</React.Fragment>;
~~~~
!!! error TS2322: Type '() => string' is not assignable to type 'ReactNode'.
!!! related TS6212 a.tsx:7:47: Did you mean to call this expression?

Loading