From a84f0b98eefd1bc1a67a67a7e033b4bcd177d9de Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 8 Feb 2023 19:18:43 -0800 Subject: [PATCH 1/6] Add failing test --- tests/cases/fourslash/badJSDocNoCrash.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 tests/cases/fourslash/badJSDocNoCrash.ts diff --git a/tests/cases/fourslash/badJSDocNoCrash.ts b/tests/cases/fourslash/badJSDocNoCrash.ts new file mode 100644 index 0000000000000..5d38ac5f8f6c6 --- /dev/null +++ b/tests/cases/fourslash/badJSDocNoCrash.ts @@ -0,0 +1,11 @@ +/// + +// @filename: index.js +//// class I18n { +//// /** +//// * @param {{dot|fulltext}} [stringMode] - which mode our translation keys use +//// */ +//// constructor(options = {}) {} +//// } + +verify.encodedSyntacticClassificationsLength(84); From bd854127b77057638476a56c1107c4c667c91909 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Thu, 9 Feb 2023 17:14:47 -0800 Subject: [PATCH 2/6] Ensure that JSDoc parsing happens within a ParsingContext --- src/compiler/parser.ts | 29 +++++++++++++++++-- .../jsDocDontBreakWithNamespaces.baseline | 26 +---------------- .../jsdocFunctionTypeFalsePositive.errors.txt | 20 +++---------- .../jsdocFunctionTypeFalsePositive.types | 4 +-- tests/cases/fourslash/badJSDocNoCrash.ts | 2 +- 5 files changed, 34 insertions(+), 47 deletions(-) diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index 0baf8b65bc412..4d37cb9b2acc1 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -1446,6 +1446,8 @@ namespace Parser { let identifiers: Map; let identifierCount: number; + // TODO(jakebailey): This type is a lie; this value actually contains the result + // of ORing a bunch of `1 << ParsingContext.XYZ`. let parsingContext: ParsingContext; let notParenthesizedArrow: Set | undefined; @@ -2824,9 +2826,13 @@ namespace Parser { return tokenIsIdentifierOrKeyword(token()) || token() === SyntaxKind.OpenBraceToken; case ParsingContext.JsxChildren: return true; + case ParsingContext.StandaloneJSDoc: + return true; + case ParsingContext.Count: + return Debug.fail("ParsingContext.Count used as a context"); // Not a real context, only a marker. + default: + Debug.assertNever(parsingContext, "Non-exhaustive case in 'isListElement'."); } - - return Debug.fail("Non-exhaustive case in 'isListElement'."); } function isValidHeritageClauseObjectLiteral() { @@ -2962,6 +2968,9 @@ namespace Parser { // True if positioned at element or terminator of the current list or any enclosing list function isInSomeParsingContext(): boolean { + // We should be in at least one parsing context, be it SourceElements while parsing + // a SourceFile, or StandaloneJSDoc when lazily parsing JSDoc. + Debug.assert(parsingContext, "Missing parsing context"); for (let kind = 0; kind < ParsingContext.Count; kind++) { if (parsingContext & (1 << kind)) { if (isListElement(kind, /*inErrorRecovery*/ true) || isListTerminator(kind)) { @@ -3078,6 +3087,7 @@ namespace Parser { case ParsingContext.VariableDeclarations: case ParsingContext.JSDocParameters: case ParsingContext.Parameters: + case ParsingContext.StandaloneJSDoc: // TODO(jakebailey): is it? return true; } return false; @@ -3337,6 +3347,7 @@ namespace Parser { case ParsingContext.JsxAttributes: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); case ParsingContext.JsxChildren: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); case ParsingContext.AssertEntries: return parseErrorAtCurrentToken(Diagnostics.Identifier_or_string_literal_expected); // AssertionKey. + case ParsingContext.StandaloneJSDoc: return; // TODO(jakebailey): any error here? case ParsingContext.Count: return Debug.fail("ParsingContext.Count used as a context"); // Not a real context, only a marker. default: Debug.assertNever(context); } @@ -7210,6 +7221,8 @@ namespace Parser { function tryReuseAmbientDeclaration(pos: number): Statement | undefined { return doInsideOfContext(NodeFlags.Ambient, () => { + // TODO(jakebailey): this is totally wrong; `parsingContext` is the result of ORing a bunch of `1 << ParsingContext.XYZ`. + // The enum should really be a bunch of flags. const node = currentNode(parsingContext, pos); if (node) { return consumeNode(node) as Statement; @@ -8397,7 +8410,8 @@ namespace Parser { TupleElementTypes, // Element types in tuple element type list HeritageClauses, // Heritage clauses for a class or interface declaration. ImportOrExportSpecifiers, // Named import clause's import specifier list, - AssertEntries, // Import entries list. + AssertEntries, // Import entries list. + StandaloneJSDoc, // TODO(jakebailey): describe Count // Number of parsing contexts } @@ -8503,6 +8517,15 @@ namespace Parser { } function parseJSDocCommentWorker(start = 0, length: number | undefined): JSDoc | undefined { + const saveParsingContext = parsingContext; + parsingContext |= 1 << ParsingContext.StandaloneJSDoc; + const jsdoc = parseJSDocCommentWorkerWorker(start, length); + parsingContext = saveParsingContext; + return jsdoc; + } + + // TODO(jakebailey): name + function parseJSDocCommentWorkerWorker(start: number, length: number | undefined): JSDoc | undefined { const content = sourceText; const end = length === undefined ? content.length : start + length; length = end - start; diff --git a/tests/baselines/reference/jsDocDontBreakWithNamespaces.baseline b/tests/baselines/reference/jsDocDontBreakWithNamespaces.baseline index 6193417fbcfbe..5f2b3405c4b32 100644 --- a/tests/baselines/reference/jsDocDontBreakWithNamespaces.baseline +++ b/tests/baselines/reference/jsDocDontBreakWithNamespaces.baseline @@ -25,7 +25,7 @@ // zee(''); // ^ // | ---------------------------------------------------------------------- -// | zee(**arg0: any**, arg1: any, arg2: any): any +// | zee(**arg0: any**, arg1: any): any // | ---------------------------------------------------------------------- [ @@ -259,30 +259,6 @@ ], "isOptional": false, "isRest": false - }, - { - "name": "arg2", - "documentation": [], - "displayParts": [ - { - "text": "arg2", - "kind": "parameterName" - }, - { - "text": ":", - "kind": "punctuation" - }, - { - "text": " ", - "kind": "space" - }, - { - "text": "any", - "kind": "keyword" - } - ], - "isOptional": false, - "isRest": false } ], "documentation": [], diff --git a/tests/baselines/reference/jsdocFunctionTypeFalsePositive.errors.txt b/tests/baselines/reference/jsdocFunctionTypeFalsePositive.errors.txt index b50b49c2e4572..f3bf77a4156b1 100644 --- a/tests/baselines/reference/jsdocFunctionTypeFalsePositive.errors.txt +++ b/tests/baselines/reference/jsdocFunctionTypeFalsePositive.errors.txt @@ -1,24 +1,12 @@ +/a.js(1,13): error TS1098: Type parameter list cannot be empty. /a.js(1,14): error TS1139: Type parameter declaration expected. -/a.js(1,17): error TS1003: Identifier expected. -/a.js(1,17): error TS1110: Type expected. -/a.js(1,17): error TS8024: JSDoc '@param' tag has name '', but there is no parameter with that name. -/a.js(1,18): error TS1005: '>' expected. -/a.js(1,18): error TS1005: '}' expected. -==== /a.js (6 errors) ==== +==== /a.js (2 errors) ==== /** @param {<} x */ + ~~ +!!! error TS1098: Type parameter list cannot be empty. ~ !!! error TS1139: Type parameter declaration expected. - -!!! error TS1003: Identifier expected. - -!!! error TS1110: Type expected. - -!!! error TS8024: JSDoc '@param' tag has name '', but there is no parameter with that name. - -!!! error TS1005: '>' expected. - -!!! error TS1005: '}' expected. function f(x) {} \ No newline at end of file diff --git a/tests/baselines/reference/jsdocFunctionTypeFalsePositive.types b/tests/baselines/reference/jsdocFunctionTypeFalsePositive.types index 29b3b4d2e1c80..a1b20646143b5 100644 --- a/tests/baselines/reference/jsdocFunctionTypeFalsePositive.types +++ b/tests/baselines/reference/jsdocFunctionTypeFalsePositive.types @@ -1,6 +1,6 @@ === /a.js === /** @param {<} x */ function f(x) {} ->f : (x: any) => void ->x : any +>f : (x: () => any) => void +>x : () => any diff --git a/tests/cases/fourslash/badJSDocNoCrash.ts b/tests/cases/fourslash/badJSDocNoCrash.ts index 5d38ac5f8f6c6..f291335ca4fae 100644 --- a/tests/cases/fourslash/badJSDocNoCrash.ts +++ b/tests/cases/fourslash/badJSDocNoCrash.ts @@ -8,4 +8,4 @@ //// constructor(options = {}) {} //// } -verify.encodedSyntacticClassificationsLength(84); +verify.encodedSyntacticClassificationsLength(69); From 472bf7e4448d976732c4eff6a91e4149cfa03218 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Thu, 9 Feb 2023 17:50:21 -0800 Subject: [PATCH 3/6] Eliminate a helper --- src/compiler/parser.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index 4d37cb9b2acc1..c73ee8977192b 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -3347,7 +3347,7 @@ namespace Parser { case ParsingContext.JsxAttributes: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); case ParsingContext.JsxChildren: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); case ParsingContext.AssertEntries: return parseErrorAtCurrentToken(Diagnostics.Identifier_or_string_literal_expected); // AssertionKey. - case ParsingContext.StandaloneJSDoc: return; // TODO(jakebailey): any error here? + case ParsingContext.StandaloneJSDoc: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); case ParsingContext.Count: return Debug.fail("ParsingContext.Count used as a context"); // Not a real context, only a marker. default: Debug.assertNever(context); } @@ -8519,13 +8519,7 @@ namespace Parser { function parseJSDocCommentWorker(start = 0, length: number | undefined): JSDoc | undefined { const saveParsingContext = parsingContext; parsingContext |= 1 << ParsingContext.StandaloneJSDoc; - const jsdoc = parseJSDocCommentWorkerWorker(start, length); - parsingContext = saveParsingContext; - return jsdoc; - } - // TODO(jakebailey): name - function parseJSDocCommentWorkerWorker(start: number, length: number | undefined): JSDoc | undefined { const content = sourceText; const end = length === undefined ? content.length : start + length; length = end - start; @@ -8548,7 +8542,11 @@ namespace Parser { const parts: JSDocComment[] = []; // + 3 for leading /**, - 5 in total for /** */ - return scanner.scanRange(start + 3, length - 5, () => { + const result = scanner.scanRange(start + 3, length - 5, doJSDocScan); + parsingContext = saveParsingContext; + return result; + + function doJSDocScan() { // Initially we can parse out a tag. We also have seen a starting asterisk. // This is so that /** * @type */ doesn't parse. let state = JSDocState.SawAsterisk; @@ -8651,7 +8649,7 @@ namespace Parser { if (parts.length && tags) Debug.assertIsDefined(commentsPos, "having parsed tags implies that the end of the comment span should be set"); const tagsArray = tags && createNodeArray(tags, tagsPos, tagsEnd); return finishNode(factory.createJSDocComment(parts.length ? createNodeArray(parts, start, commentsPos) : comments.length ? comments.join("") : undefined, tagsArray), start, end); - }); + } function removeLeadingNewlines(comments: string[]) { while (comments.length && (comments[0] === "\n" || comments[0] === "\r")) { From 81c697a7815b7bd38c3a05544a79a9bb18d64e07 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Thu, 9 Feb 2023 17:51:37 -0800 Subject: [PATCH 4/6] Rename --- src/compiler/parser.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index c73ee8977192b..faee14114c22f 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -2826,7 +2826,7 @@ namespace Parser { return tokenIsIdentifierOrKeyword(token()) || token() === SyntaxKind.OpenBraceToken; case ParsingContext.JsxChildren: return true; - case ParsingContext.StandaloneJSDoc: + case ParsingContext.JSDocComment: return true; case ParsingContext.Count: return Debug.fail("ParsingContext.Count used as a context"); // Not a real context, only a marker. @@ -2969,7 +2969,7 @@ namespace Parser { // True if positioned at element or terminator of the current list or any enclosing list function isInSomeParsingContext(): boolean { // We should be in at least one parsing context, be it SourceElements while parsing - // a SourceFile, or StandaloneJSDoc when lazily parsing JSDoc. + // a SourceFile, or JSDocComment when lazily parsing JSDoc. Debug.assert(parsingContext, "Missing parsing context"); for (let kind = 0; kind < ParsingContext.Count; kind++) { if (parsingContext & (1 << kind)) { @@ -3087,7 +3087,7 @@ namespace Parser { case ParsingContext.VariableDeclarations: case ParsingContext.JSDocParameters: case ParsingContext.Parameters: - case ParsingContext.StandaloneJSDoc: // TODO(jakebailey): is it? + case ParsingContext.JSDocComment: // TODO(jakebailey): is it? return true; } return false; @@ -3347,7 +3347,7 @@ namespace Parser { case ParsingContext.JsxAttributes: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); case ParsingContext.JsxChildren: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); case ParsingContext.AssertEntries: return parseErrorAtCurrentToken(Diagnostics.Identifier_or_string_literal_expected); // AssertionKey. - case ParsingContext.StandaloneJSDoc: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); + case ParsingContext.JSDocComment: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected); case ParsingContext.Count: return Debug.fail("ParsingContext.Count used as a context"); // Not a real context, only a marker. default: Debug.assertNever(context); } @@ -8411,7 +8411,7 @@ namespace Parser { HeritageClauses, // Heritage clauses for a class or interface declaration. ImportOrExportSpecifiers, // Named import clause's import specifier list, AssertEntries, // Import entries list. - StandaloneJSDoc, // TODO(jakebailey): describe + JSDocComment, // Parsing via JSDocParser Count // Number of parsing contexts } @@ -8518,7 +8518,7 @@ namespace Parser { function parseJSDocCommentWorker(start = 0, length: number | undefined): JSDoc | undefined { const saveParsingContext = parsingContext; - parsingContext |= 1 << ParsingContext.StandaloneJSDoc; + parsingContext |= 1 << ParsingContext.JSDocComment; const content = sourceText; const end = length === undefined ? content.length : start + length; From 6b2888a8dd75cdbce7b1ea8a442ebbed887631c4 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Fri, 10 Feb 2023 11:26:33 -0800 Subject: [PATCH 5/6] Drop reuse --- src/compiler/parser.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index faee14114c22f..741e6b29fe85d 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -3087,7 +3087,6 @@ namespace Parser { case ParsingContext.VariableDeclarations: case ParsingContext.JSDocParameters: case ParsingContext.Parameters: - case ParsingContext.JSDocComment: // TODO(jakebailey): is it? return true; } return false; From ba1da57c4ab69067f0c7382ed9c5329f28f5b834 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Fri, 10 Feb 2023 11:26:50 -0800 Subject: [PATCH 6/6] Rename --- .../{badJSDocNoCrash.ts => jsdocWithBarInTypeLiteral.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/cases/fourslash/{badJSDocNoCrash.ts => jsdocWithBarInTypeLiteral.ts} (100%) diff --git a/tests/cases/fourslash/badJSDocNoCrash.ts b/tests/cases/fourslash/jsdocWithBarInTypeLiteral.ts similarity index 100% rename from tests/cases/fourslash/badJSDocNoCrash.ts rename to tests/cases/fourslash/jsdocWithBarInTypeLiteral.ts