Skip to content

feat(45010): handle unclosed fragment in getJsxClosingTagAtPosition #45532

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 8, 2021
Merged

feat(45010): handle unclosed fragment in getJsxClosingTagAtPosition #45532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08333d8
feat(45010): handle unclosed fragment in `getJsxClosingTagAtPosition`
hi-ogawa Aug 21, 2021
60498dd
Update tests
hi-ogawa Aug 22, 2021
90a35e0
Fix types of `JsxText.parent` and `JsxExpression.parent`
hi-ogawa Sep 8, 2021
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
4 changes: 2 additions & 2 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2593,14 +2593,14 @@ namespace ts {

export interface JsxExpression extends Expression {
readonly kind: SyntaxKind.JsxExpression;
readonly parent: JsxElement | JsxAttributeLike;
readonly parent: JsxElement | JsxFragment | JsxAttributeLike;
readonly dotDotDotToken?: Token<SyntaxKind.DotDotDotToken>;
readonly expression?: Expression;
}

export interface JsxText extends LiteralLikeNode {
readonly kind: SyntaxKind.JsxText;
readonly parent: JsxElement;
readonly parent: JsxElement | JsxFragment;
readonly containsOnlyTriviaWhiteSpaces: boolean;
}

Expand Down
2 changes: 1 addition & 1 deletion src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1180,7 +1180,7 @@ namespace ts.Completions {
case SyntaxKind.CaseKeyword:
return getSwitchedType(cast(parent, isCaseClause), checker);
case SyntaxKind.OpenBraceToken:
return isJsxExpression(parent) && parent.parent.kind !== SyntaxKind.JsxElement ? checker.getContextualTypeForJsxAttribute(parent.parent) : undefined;
return isJsxExpression(parent) && !isJsxElement(parent.parent) && !isJsxFragment(parent.parent) ? checker.getContextualTypeForJsxAttribute(parent.parent) : undefined;
default:
const argInfo = SignatureHelp.getArgumentInfoForCompletions(previousToken, position, sourceFile);
return argInfo ?
Expand Down
11 changes: 10 additions & 1 deletion src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2088,10 +2088,15 @@ namespace ts {
const token = findPrecedingToken(position, sourceFile);
if (!token) return undefined;
const element = token.kind === SyntaxKind.GreaterThanToken && isJsxOpeningElement(token.parent) ? token.parent.parent
: isJsxText(token) ? token.parent : undefined;
: isJsxText(token) && isJsxElement(token.parent) ? token.parent : undefined;
Copy link
Contributor Author

@hi-ogawa hi-ogawa Aug 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

To comment on why this change is necessary, I found that JsxText.parent is not necessarily JsxElement but it also can be JsxFragment, even though typing-wise it's supposed to be JsxElement according to

TypeScript/src/compiler/types.ts

Lines 2601 to 2605 in 8362a0f

export interface JsxText extends LiteralLikeNode {
readonly kind: SyntaxKind.JsxText;
readonly parent: JsxElement;
readonly containsOnlyTriviaWhiteSpaces: boolean;
}

Without this change, isUnclosedTag({ openingElement, ... }) below will lead to runtime error since openingElement becomes undefined, for example, in the test case const x = <> foo/*1*/ </> (see @Filename: /1.tsx in autoCloseFragment.ts).

Probably, a legitimate fix would be something like defining type JsxParent = JsxElement | JsxFragment and replacing existing member types parent: JsxElement with parent: JsxParant, but I'm not so sure yet that is what's supposed to be since I'm very new to the code base.
I would appreciate if someone familar with these parts can comment on this.

Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, I think this is a good way to fix this problem. I think maybe the original code just didn't consider JsxFragments when it was written.

Copy link
Member

Choose a reason for hiding this comment

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

@hi-ogawa did you try changing the type of parent here to be JsxElement | JsxFragment? I agree we should really fix that type. Would you mind trying it and seeing if you can resolve any other errors that come up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!
Well, I didn't try anything yet, but I'll see what I can do and get back to you with my findings.

Copy link
Contributor Author

@hi-ogawa hi-ogawa Sep 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@andrewbranch Following your request, I addressed the issue with this commit 90a35e0.
There wasn't any type error related to JsxText.parent so it looks alright.
I also changed the type of JsxExpression.parent to include JsxFragment since I believe that's what it should be, then there was one type error in src/services/completions.ts, so I did a trivial fix for that.
Overall, I think there aren't much use JsxText.parent or JsxExpression.parent in the code base currently, so hopefully the type change won't have much impact.
But, one thing I worry about is that it's going to change public API (as you can see from baselines/references/api), so the library users will be also affected by this change.

if (element && isUnclosedTag(element)) {
return { newText: `</${element.openingElement.tagName.getText(sourceFile)}>` };
}
const fragment = token.kind === SyntaxKind.GreaterThanToken && isJsxOpeningFragment(token.parent) ? token.parent.parent
: isJsxText(token) && isJsxFragment(token.parent) ? token.parent : undefined;
if (fragment && isUnclosedFragment(fragment)) {
return { newText: "</>" };
}
}

function getLinesForRange(sourceFile: SourceFile, textRange: TextRange) {
Expand Down Expand Up @@ -2334,6 +2339,10 @@ namespace ts {
isJsxElement(parent) && tagNamesAreEquivalent(openingElement.tagName, parent.openingElement.tagName) && isUnclosedTag(parent);
}

function isUnclosedFragment({ closingFragment, parent }: JsxFragment): boolean {
return !!(closingFragment.flags & NodeFlags.ThisNodeHasError) || (isJsxFragment(parent) && isUnclosedFragment(parent));
}

function getSpanOfEnclosingComment(fileName: string, position: number, onlyMultiLine: boolean): TextSpan | undefined {
const sourceFile = syntaxTreeCache.getCurrentSourceFile(fileName);
const range = formatting.getRangeOfEnclosingComment(sourceFile, position);
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1381,13 +1381,13 @@ declare namespace ts {
}
export interface JsxExpression extends Expression {
readonly kind: SyntaxKind.JsxExpression;
readonly parent: JsxElement | JsxAttributeLike;
readonly parent: JsxElement | JsxFragment | JsxAttributeLike;
readonly dotDotDotToken?: Token<SyntaxKind.DotDotDotToken>;
readonly expression?: Expression;
}
export interface JsxText extends LiteralLikeNode {
readonly kind: SyntaxKind.JsxText;
readonly parent: JsxElement;
readonly parent: JsxElement | JsxFragment;
readonly containsOnlyTriviaWhiteSpaces: boolean;
}
export type JsxChild = JsxText | JsxExpression | JsxElement | JsxSelfClosingElement | JsxFragment;
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1381,13 +1381,13 @@ declare namespace ts {
}
export interface JsxExpression extends Expression {
readonly kind: SyntaxKind.JsxExpression;
readonly parent: JsxElement | JsxAttributeLike;
readonly parent: JsxElement | JsxFragment | JsxAttributeLike;
readonly dotDotDotToken?: Token<SyntaxKind.DotDotDotToken>;
readonly expression?: Expression;
}
export interface JsxText extends LiteralLikeNode {
readonly kind: SyntaxKind.JsxText;
readonly parent: JsxElement;
readonly parent: JsxElement | JsxFragment;
readonly containsOnlyTriviaWhiteSpaces: boolean;
}
export type JsxChild = JsxText | JsxExpression | JsxElement | JsxSelfClosingElement | JsxFragment;
Expand Down
51 changes: 51 additions & 0 deletions tests/cases/fourslash/autoCloseFragment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/// <reference path='fourslash.ts' />

// Using separate files for each example to avoid unclosed JSX tags affecting other tests.

// @Filename: /0.tsx
////const x = <>/*0*/;

// @Filename: /1.tsx
////const x = <> foo/*1*/ </>;

// @Filename: /2.tsx
////const x = <></>/*2*/;

// @Filename: /3.tsx
////const x = </>/*3*/;

// @Filename: /4.tsx
////const x = <div>
//// <>/*4*/
//// </div>
////</>;

// @Filename: /5.tsx
////const x = <> text /*5*/;

// @Filename: /6.tsx
////const x = <>
//// <>/*6*/
////</>;

// @Filename: /7.tsx
////const x = <div>
//// <>/*7*/
////</div>;

// @Filename: /8.tsx
////const x = <div>
//// <>/*8*/</>
////</div>;

verify.jsxClosingTag({
0: { newText: "</>" },
1: undefined,
2: undefined,
3: undefined,
4: { newText: "</>" },
5: { newText: "</>" },
6: { newText: "</>" },
7: { newText: "</>" },
8: undefined,
});