Skip to content

Ensure that JSDoc parsing happens within a ParsingContext #52710

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 10 commits into from
May 4, 2023
Merged

Ensure that JSDoc parsing happens within a ParsingContext #52710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a84f0b9
Add failing test
jakebailey Feb 9, 2023
bd85412
Ensure that JSDoc parsing happens within a ParsingContext
jakebailey Feb 10, 2023
472bf7e
Eliminate a helper
jakebailey Feb 10, 2023
81c697a
Rename
jakebailey Feb 10, 2023
6b2888a
Drop reuse
jakebailey Feb 10, 2023
ba1da57
Rename
jakebailey Feb 10, 2023
0acd844
Merge branch 'main' into fix-47537
jakebailey Feb 10, 2023
d71cc52
Merge branch 'main' into fix-47537
jakebailey Mar 10, 2023
13fde97
Merge branch 'main' into fix-47537
jakebailey Apr 6, 2023
961c6c1
Merge branch 'main' into fix-47537
jakebailey May 4, 2023
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
30 changes: 25 additions & 5 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1491,6 +1491,8 @@ namespace Parser {
var identifiers: Map<string, string>;
var identifierCount: number;

// TODO(jakebailey): This type is a lie; this value actually contains the result
// of ORing a bunch of `1 << ParsingContext.XYZ`.
var parsingContext: ParsingContext;

var notParenthesizedArrow: Set<number> | undefined;
Expand Down Expand Up @@ -2872,9 +2874,13 @@ namespace Parser {
return tokenIsIdentifierOrKeyword(token()) || token() === SyntaxKind.OpenBraceToken;
case ParsingContext.JsxChildren:
return true;
case ParsingContext.JSDocComment:
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'.");
Copy link
Member

Choose a reason for hiding this comment

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

how did this (mostly) work before? Do we never callisListElement inside jsdoc? (Unlikely). Did currentNode(parsingContext) always return a value, so the switch never executed? Was there some other incorrect parsing context set during jsdoc parsing, except for the bug case?

Copy link
Member Author

Choose a reason for hiding this comment

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

This code was actually never hit for JSDoc, because the loop in isInSomeParsingContext checks each bit and then uses it. Unlike source files (which are always inside of a statement list), the standalone JSDoc parser was not, and so its parsing context was zero, and skipped all of this code (which was the bug).

Copy link
Member Author

Choose a reason for hiding this comment

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

Another way to think about this particular block is that it's checking equiality on the context, which are values like { 1, 2, 3, 4 ... }... but the context actually stored in the parser are actually 1 << { 1, 2, 3, 4 ... }, so anyone trying to use this code has to un-flag-ify the value before using it, and that's the other broken thing I found in this code.

}

return Debug.fail("Non-exhaustive case in 'isListElement'.");
}

function isValidHeritageClauseObjectLiteral() {
Expand Down Expand Up @@ -3010,6 +3016,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 JSDocComment 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)) {
Expand Down Expand Up @@ -3385,6 +3394,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.JSDocComment: return parseErrorAtCurrentToken(Diagnostics.Identifier_expected);
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean to provide an error in a JSDoc parsing comment? Where would we issue this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of recovery in parseList/parseDelimitedList and feasibly only only happens when a list ends prematurely.

Copy link
Member

Choose a reason for hiding this comment

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

You probably don't need an error but it's fine. In a follow-up PR, I'd be curious to see what happens if it Debug.fail()s.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, yeah, you can just Debug.fail() and this PR also works. Maybe I'll just do that.

But, I am probably close to eliminating this entire parse mode entirely too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably this doesn't break because the tests aren't good enough.

case ParsingContext.Count: return Debug.fail("ParsingContext.Count used as a context"); // Not a real context, only a marker.
default: Debug.assertNever(context);
}
Expand Down Expand Up @@ -7289,6 +7299,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.
Comment on lines +7302 to +7303
Copy link
Member Author

Choose a reason for hiding this comment

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

All of the other uses get this right, at least, so node reuse has feasibly only been broken for ambient declarations.

Copy link
Member

Choose a reason for hiding this comment

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

You're right - what happens if you pass in 0b111111111? Which tests break? My hope is that something tested node reuse in an ambient context.

Copy link
Member Author

Choose a reason for hiding this comment

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

Passing in 0b111111111 won't change anything because it uses equality checks, so they'll all just end up being false and no node reuse will occur.

Copy link
Member

Choose a reason for hiding this comment

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

won't change anything because it uses equality checks, so they'll all just end up being false and no node reuse will occur.

But we have tests that ensure that some node reuse occurs, don't we?

Copy link
Member Author

Choose a reason for hiding this comment

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

All fourslash server tests which edit their files are incremental, which covers things in some aspect, but the incrementalParser.ts unittest file doesn't appear to test ambient declarations too much.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add an incremental test with an ambient context where node reuse should happen, and which fails when you pass in 0b11111111?

Copy link
Member Author

Choose a reason for hiding this comment

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

In trying to make a test case for this, I've found that this can only be a problem if there's an ambient declaration inside of something else, like:

{
    declare module "foo" {
        // ...
    }
}

This is technically illegal code, and this bug here means we never use any nodes from this tree.

Ambients are only being reused now because they can only be a child of the SourceFile itself, and its ParsingContext happens to be ParsingContext.SourceElements = 0. Then, the top level variable has value 1 << 0 or 1, which is ParsingContext.BlockStatements which just so happens to also be reusable, so it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I could fix the bug, but it'd only fix broken trees that probably never happen. But it's still totally wrong and only works because of a fluke!

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I'm wrong, the "ambient in a block" also works because of a fluke; you get parsingContext 0b11 which equals ParsingContext.SwitchClauseStatements and that is also reusable, so the code passes. 🙃

const node = currentNode(parsingContext, pos);
if (node) {
return consumeNode(node) as Statement;
Expand Down Expand Up @@ -8492,7 +8504,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.
JSDocComment, // Parsing via JSDocParser
Count // Number of parsing contexts
}

Expand Down Expand Up @@ -8598,6 +8611,9 @@ namespace Parser {
}

function parseJSDocCommentWorker(start = 0, length: number | undefined): JSDoc | undefined {
const saveParsingContext = parsingContext;
parsingContext |= 1 << ParsingContext.JSDocComment;

const content = sourceText;
const end = length === undefined ? content.length : start + length;
length = end - start;
Expand All @@ -8620,7 +8636,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;
Expand Down Expand Up @@ -8726,7 +8746,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) : trimmedComments.length ? trimmedComments : undefined, tagsArray), start, end);
});
}

function removeLeadingNewlines(comments: string[]) {
while (comments.length && (comments[0] === "\n" || comments[0] === "\r")) {
Expand Down
26 changes: 1 addition & 25 deletions tests/baselines/reference/jsDocDontBreakWithNamespaces.baseline
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
// zee('');
// ^
// | ----------------------------------------------------------------------
// | zee(**arg0: any**, arg1: any, arg2: any): any
// | zee(**arg0: any**, arg1: any): any
Copy link
Member Author

Choose a reason for hiding this comment

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

This function is:

/** @type {function(module:xxxx, module:xxxx): module:xxxxx} */
function zee() { }

Clearly, this has two parameters (not three!), and now we get that right.

// | ----------------------------------------------------------------------

[
Expand Down Expand Up @@ -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": [],
Expand Down
20 changes: 4 additions & 16 deletions tests/baselines/reference/jsdocFunctionTypeFalsePositive.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

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

This function is:

/** @param {<} x */
function f(x) {}

Clearly this has a parameter named x, and now we no longer spuriously complain about that.

/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) {}

4 changes: 2 additions & 2 deletions tests/baselines/reference/jsdocFunctionTypeFalsePositive.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
=== /a.js ===
/** @param {<} x */
function f(x) {}
>f : (x: any) => void
Copy link
Member Author

Choose a reason for hiding this comment

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

IDK if this is bad or not; this is clearly malformed JSDoc but before we just got any. Now we think we're starting type parameters (which is normal for parsing a type) and then assume it's probably a function signature which is the only thing that can start with < like this.

Copy link
Member

Choose a reason for hiding this comment

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

Looks fine to me.

>x : any
>f : (x: () => any) => void
>x : () => any

11 changes: 11 additions & 0 deletions tests/cases/fourslash/jsdocWithBarInTypeLiteral.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/// <reference path="fourslash.ts" />

// @filename: index.js
//// class I18n {
//// /**
//// * @param {{dot|fulltext}} [stringMode] - which mode our translation keys use
//// */
//// constructor(options = {}) {}
//// }

verify.encodedSyntacticClassificationsLength(69);