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.

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

Jump to bottom

Make JSDoc skipping public, plus more configurable #55739

Merged
merged 26 commits into from Sep 21, 2023
Merged

Make JSDoc skipping public, plus more configurable #55739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
442b467
Make JSDoc skipping more configurable
jakebailey Sep 13, 2023
9119d2f
I guess it's a big binary expression now
jakebailey Sep 13, 2023
ba9dcab
An attempt at ProjectService plumbing, not sure what should be public…
jakebailey Sep 14, 2023
8a4837c
Merge branch 'main' into skip-jsdoc-2
jakebailey Sep 15, 2023
66a0b09
Preemptive bikeshedding
jakebailey Sep 15, 2023
46bbf0e
Temporarily make stuff required to try and get plumbing correct
jakebailey Sep 15, 2023
88bfd9e
Revert by moving parameters back to optional
jakebailey Sep 15, 2023
2abd21b
revert a formatting change
jakebailey Sep 15, 2023
b691830
Remove TODOs, tests pass
jakebailey Sep 15, 2023
9e2f8c1
Some PR feedback
jakebailey Sep 15, 2023
3d60c92
One more revert
jakebailey Sep 15, 2023
fb22a3e
remove redundant assignments
jakebailey Sep 15, 2023
626ae3e
Make option slighttly more granular
jakebailey Sep 18, 2023
b6379cb
Reorder so that the surefire enum members are first
jakebailey Sep 18, 2023
a273a91
Drop option from LS Host
jakebailey Sep 18, 2023
a430b30
Add docs to new enum
jakebailey Sep 18, 2023
3ebdfb6
Fix registry key
jakebailey Sep 18, 2023
3673f9e
Forgot baselines
jakebailey Sep 18, 2023
f393631
Revert "Fix registry key"
jakebailey Sep 19, 2023
40b0cc4
update baselines
jakebailey Sep 19, 2023
12f5903
Move jsDocParsingMode to options bag
jakebailey Sep 19, 2023
735c17c
Revert "Drop option from LS Host"
jakebailey Sep 20, 2023
b3e23a9
Move onto host
jakebailey Sep 20, 2023
db14c33
Drop out of create functions in favor of prop setting
jakebailey Sep 20, 2023
517f289
Remove one more change
jakebailey Sep 20, 2023
9392280
Rename global tsc JSDocParsingMode for better tsc.js emit
jakebailey Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 22 additions & 21 deletions src/compiler/parser.ts
View file
Open in desktop
Expand Up @@ -187,6 +187,7 @@ import {
JSDocOverloadTag,
JSDocOverrideTag,
JSDocParameterTag,
JSDocParsingMode,
JSDocPrivateTag,
JSDocPropertyLikeTag,
JSDocPropertyTag,
Expand Down Expand Up @@ -1318,16 +1319,14 @@ export interface CreateSourceFileOptions {
setExternalModuleIndicator?: (file: SourceFile) => void;
/** @internal */ packageJsonLocations?: readonly string[];
/** @internal */ packageJsonScope?: PackageJsonInfo;
jsDocParsingMode?: JSDocParsingMode;
}

function setExternalModuleIndicator(sourceFile: SourceFile) {
sourceFile.externalModuleIndicator = isFileProbablyExternalModule(sourceFile);
}

export function createSourceFile(fileName: string, sourceText: string, languageVersionOrOptions: ScriptTarget | CreateSourceFileOptions, setParentNodes?: boolean, scriptKind?: ScriptKind): SourceFile;
/** @internal */
export function createSourceFile(fileName: string, sourceText: string, languageVersionOrOptions: ScriptTarget | CreateSourceFileOptions, setParentNodes?: boolean, scriptKind?: ScriptKind, skipNonSemanticJSDoc?: boolean): SourceFile;
export function createSourceFile(fileName: string, sourceText: string, languageVersionOrOptions: ScriptTarget | CreateSourceFileOptions, setParentNodes = false, scriptKind?: ScriptKind, skipNonSemanticJSDoc?: boolean): SourceFile {
export function createSourceFile(fileName: string, sourceText: string, languageVersionOrOptions: ScriptTarget | CreateSourceFileOptions, setParentNodes = false, scriptKind?: ScriptKind): SourceFile {
tracing?.push(tracing.Phase.Parse, "createSourceFile", { path: fileName }, /*separateBeginAndEnd*/ true);
performance.mark("beforeParse");
let result: SourceFile;
Expand All @@ -1337,16 +1336,17 @@ export function createSourceFile(fileName: string, sourceText: string, languageV
languageVersion,
setExternalModuleIndicator: overrideSetExternalModuleIndicator,
impliedNodeFormat: format,
jsDocParsingMode,
} = typeof languageVersionOrOptions === "object" ? languageVersionOrOptions : ({ languageVersion: languageVersionOrOptions } as CreateSourceFileOptions);
if (languageVersion === ScriptTarget.JSON) {
result = Parser.parseSourceFile(fileName, sourceText, languageVersion, /*syntaxCursor*/ undefined, setParentNodes, ScriptKind.JSON, noop, skipNonSemanticJSDoc);
result = Parser.parseSourceFile(fileName, sourceText, languageVersion, /*syntaxCursor*/ undefined, setParentNodes, ScriptKind.JSON, noop, jsDocParsingMode);
}
else {
const setIndicator = format === undefined ? overrideSetExternalModuleIndicator : (file: SourceFile) => {
file.impliedNodeFormat = format;
return (overrideSetExternalModuleIndicator || setExternalModuleIndicator)(file);
};
result = Parser.parseSourceFile(fileName, sourceText, languageVersion, /*syntaxCursor*/ undefined, setParentNodes, scriptKind, setIndicator, skipNonSemanticJSDoc);
result = Parser.parseSourceFile(fileName, sourceText, languageVersion, /*syntaxCursor*/ undefined, setParentNodes, scriptKind, setIndicator, jsDocParsingMode);
}
perfLogger?.logStopParseSourceFile();

Expand Down Expand Up @@ -1586,7 +1586,7 @@ namespace Parser {
setParentNodes = false,
scriptKind?: ScriptKind,
setExternalModuleIndicatorOverride?: (file: SourceFile) => void,
skipNonSemanticJSDoc?: boolean,
jsDocParsingMode = JSDocParsingMode.ParseAll,
): SourceFile {
scriptKind = ensureScriptKind(fileName, scriptKind);
if (scriptKind === ScriptKind.JSON) {
Expand All @@ -1601,10 +1601,9 @@ namespace Parser {
return result;
}

skipNonSemanticJSDoc = !!skipNonSemanticJSDoc && scriptKind !== ScriptKind.JS && scriptKind !== ScriptKind.JSX;
initializeState(fileName, sourceText, languageVersion, syntaxCursor, scriptKind, skipNonSemanticJSDoc);
initializeState(fileName, sourceText, languageVersion, syntaxCursor, scriptKind, jsDocParsingMode);

const result = parseSourceFileWorker(languageVersion, setParentNodes, scriptKind, setExternalModuleIndicatorOverride || setExternalModuleIndicator, skipNonSemanticJSDoc);
const result = parseSourceFileWorker(languageVersion, setParentNodes, scriptKind, setExternalModuleIndicatorOverride || setExternalModuleIndicator, jsDocParsingMode);

clearState();

Expand All @@ -1613,7 +1612,7 @@ namespace Parser {

export function parseIsolatedEntityName(content: string, languageVersion: ScriptTarget): EntityName | undefined {
// Choice of `isDeclarationFile` should be arbitrary
initializeState("", content, languageVersion, /*syntaxCursor*/ undefined, ScriptKind.JS, /*skipNonSemanticJSDoc*/ false);
initializeState("", content, languageVersion, /*syntaxCursor*/ undefined, ScriptKind.JS, JSDocParsingMode.ParseAll);
// Prime the scanner.
nextToken();
const entityName = parseEntityName(/*allowReservedWords*/ true);
Expand All @@ -1623,7 +1622,7 @@ namespace Parser {
}

export function parseJsonText(fileName: string, sourceText: string, languageVersion: ScriptTarget = ScriptTarget.ES2015, syntaxCursor?: IncrementalParser.SyntaxCursor, setParentNodes = false): JsonSourceFile {
initializeState(fileName, sourceText, languageVersion, syntaxCursor, ScriptKind.JSON, /*skipNonSemanticJSDoc*/ false);
initializeState(fileName, sourceText, languageVersion, syntaxCursor, ScriptKind.JSON, JSDocParsingMode.ParseAll);
sourceFlags = contextFlags;

// Prime the scanner.
Expand Down Expand Up @@ -1711,7 +1710,7 @@ namespace Parser {
return result;
}

function initializeState(_fileName: string, _sourceText: string, _languageVersion: ScriptTarget, _syntaxCursor: IncrementalParser.SyntaxCursor | undefined, _scriptKind: ScriptKind, _skipNonSemanticJSDoc: boolean) {
function initializeState(_fileName: string, _sourceText: string, _languageVersion: ScriptTarget, _syntaxCursor: IncrementalParser.SyntaxCursor | undefined, _scriptKind: ScriptKind, _jsDocParsingMode: JSDocParsingMode) {
NodeConstructor = objectAllocator.getNodeConstructor();
TokenConstructor = objectAllocator.getTokenConstructor();
IdentifierConstructor = objectAllocator.getIdentifierConstructor();
Expand Down Expand Up @@ -1752,15 +1751,17 @@ namespace Parser {
scanner.setOnError(scanError);
scanner.setScriptTarget(languageVersion);
scanner.setLanguageVariant(languageVariant);
scanner.setSkipNonSemanticJSDoc(_skipNonSemanticJSDoc);
scanner.setScriptKind(scriptKind);
scanner.setJSDocParsingMode(_jsDocParsingMode);
}

function clearState() {
// Clear out the text the scanner is pointing at, so it doesn't keep anything alive unnecessarily.
scanner.clearCommentDirectives();
scanner.setText("");
scanner.setOnError(undefined);
scanner.setSkipNonSemanticJSDoc(false);
scanner.setScriptKind(ScriptKind.Unknown);
scanner.setJSDocParsingMode(JSDocParsingMode.ParseAll);

// Clear any data. We don't want to accidentally hold onto it for too long.
sourceText = undefined!;
Expand All @@ -1777,7 +1778,7 @@ namespace Parser {
topLevel = true;
}

function parseSourceFileWorker(languageVersion: ScriptTarget, setParentNodes: boolean, scriptKind: ScriptKind, setExternalModuleIndicator: (file: SourceFile) => void, skipNonSemanticJSDoc: boolean): SourceFile {
function parseSourceFileWorker(languageVersion: ScriptTarget, setParentNodes: boolean, scriptKind: ScriptKind, setExternalModuleIndicator: (file: SourceFile) => void, jsDocParsingMode: JSDocParsingMode): SourceFile {
const isDeclarationFile = isDeclarationFileName(fileName);
if (isDeclarationFile) {
contextFlags |= NodeFlags.Ambient;
Expand All @@ -1804,7 +1805,7 @@ namespace Parser {
sourceFile.identifierCount = identifierCount;
sourceFile.identifiers = identifiers;
sourceFile.parseDiagnostics = attachFileToDiagnostics(parseDiagnostics, sourceFile);
sourceFile.skipNonSemanticJSDoc = skipNonSemanticJSDoc;
sourceFile.jsDocParsingMode = jsDocParsingMode;
if (jsDocDiagnostics) {
sourceFile.jsDocDiagnostics = attachFileToDiagnostics(jsDocDiagnostics, sourceFile);
}
Expand Down Expand Up @@ -8686,7 +8687,7 @@ namespace Parser {

export namespace JSDocParser {
export function parseJSDocTypeExpressionForTests(content: string, start: number | undefined, length: number | undefined): { jsDocTypeExpression: JSDocTypeExpression; diagnostics: Diagnostic[]; } | undefined {
initializeState("file.js", content, ScriptTarget.Latest, /*syntaxCursor*/ undefined, ScriptKind.JS, /*skipNonSemanticJSDoc*/ false);
initializeState("file.js", content, ScriptTarget.Latest, /*syntaxCursor*/ undefined, ScriptKind.JS, JSDocParsingMode.ParseAll);
scanner.setText(content, start, length);
currentToken = scanner.scan();
const jsDocTypeExpression = parseJSDocTypeExpression();
Expand Down Expand Up @@ -8736,7 +8737,7 @@ namespace Parser {
}

export function parseIsolatedJSDocComment(content: string, start: number | undefined, length: number | undefined): { jsDoc: JSDoc; diagnostics: Diagnostic[]; } | undefined {
initializeState("", content, ScriptTarget.Latest, /*syntaxCursor*/ undefined, ScriptKind.JS, /*skipNonSemanticJSDoc*/ false);
initializeState("", content, ScriptTarget.Latest, /*syntaxCursor*/ undefined, ScriptKind.JS, JSDocParsingMode.ParseAll);
const jsDoc = doInsideOfContext(NodeFlags.JSDoc, () => parseJSDocCommentWorker(start, length));

const sourceFile = { languageVariant: LanguageVariant.Standard, text: content } as SourceFile;
Expand Down Expand Up @@ -9804,7 +9805,7 @@ namespace IncrementalParser {
if (sourceFile.statements.length === 0) {
// If we don't have any statements in the current source file, then there's no real
// way to incrementally parse. So just do a full parse instead.
return Parser.parseSourceFile(sourceFile.fileName, newText, sourceFile.languageVersion, /*syntaxCursor*/ undefined, /*setParentNodes*/ true, sourceFile.scriptKind, sourceFile.setExternalModuleIndicator, sourceFile.skipNonSemanticJSDoc);
return Parser.parseSourceFile(sourceFile.fileName, newText, sourceFile.languageVersion, /*syntaxCursor*/ undefined, /*setParentNodes*/ true, sourceFile.scriptKind, sourceFile.setExternalModuleIndicator, sourceFile.jsDocParsingMode);
}

// Make sure we're not trying to incrementally update a source file more than once. Once
Expand Down Expand Up @@ -9867,7 +9868,7 @@ namespace IncrementalParser {
// inconsistent tree. Setting the parents on the new tree should be very fast. We
// will immediately bail out of walking any subtrees when we can see that their parents
// are already correct.
const result = Parser.parseSourceFile(sourceFile.fileName, newText, sourceFile.languageVersion, syntaxCursor, /*setParentNodes*/ true, sourceFile.scriptKind, sourceFile.setExternalModuleIndicator, sourceFile.skipNonSemanticJSDoc);
const result = Parser.parseSourceFile(sourceFile.fileName, newText, sourceFile.languageVersion, syntaxCursor, /*setParentNodes*/ true, sourceFile.scriptKind, sourceFile.setExternalModuleIndicator, sourceFile.jsDocParsingMode);
result.commentDirectives = getNewCommentDirectives(
sourceFile.commentDirectives,
result.commentDirectives,
Expand Down
10 changes: 4 additions & 6 deletions src/compiler/program.ts
View file
Open in desktop
Expand Up @@ -402,7 +402,6 @@ export function createGetSourceFile(
readFile: ProgramHost<any>["readFile"],
getCompilerOptions: () => CompilerOptions,
setParentNodes: boolean | undefined,
skipNonSemanticJSDocParsing: boolean | undefined,
): CompilerHost["getSourceFile"] {
return (fileName, languageVersionOrOptions, onError) => {
let text: string | undefined;
Expand All @@ -418,7 +417,7 @@ export function createGetSourceFile(
}
text = "";
}
return text !== undefined ? createSourceFile(fileName, text, languageVersionOrOptions, setParentNodes, /*scriptKind*/ undefined, skipNonSemanticJSDocParsing) : undefined;
return text !== undefined ? createSourceFile(fileName, text, languageVersionOrOptions, setParentNodes) : undefined;
};
}

Expand Down Expand Up @@ -459,7 +458,6 @@ export function createWriteFileMeasuringIO(
export function createCompilerHostWorker(
options: CompilerOptions,
setParentNodes?: boolean,
skipNonSemanticJSDocParsing?: boolean,
system: System = sys,
): CompilerHost {
const existingDirectories = new Map<string, boolean>();
Expand All @@ -482,7 +480,7 @@ export function createCompilerHostWorker(
const newLine = getNewLineCharacter(options);
const realpath = system.realpath && ((path: string) => system.realpath!(path));
const compilerHost: CompilerHost = {
getSourceFile: createGetSourceFile(fileName => compilerHost.readFile(fileName), () => options, setParentNodes, skipNonSemanticJSDocParsing),
getSourceFile: createGetSourceFile(fileName => compilerHost.readFile(fileName), () => options, setParentNodes),
sheetalkamat marked this conversation as resolved.
Show resolved Hide resolved
getDefaultLibLocation,
getDefaultLibFileName: options => combinePaths(getDefaultLibLocation(), getDefaultLibFileName(options)),
writeFile: createWriteFileMeasuringIO(
Expand Down Expand Up @@ -3551,8 +3549,8 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg
const languageVersion = getEmitScriptTarget(options);
const setExternalModuleIndicator = getSetExternalModuleIndicator(options);
return typeof result === "object" ?
{ ...result, languageVersion, setExternalModuleIndicator } :
{ languageVersion, impliedNodeFormat: result, setExternalModuleIndicator };
{ ...result, languageVersion, setExternalModuleIndicator, jsDocParsingMode: host.jsDocParsingMode } :
{ languageVersion, impliedNodeFormat: result, setExternalModuleIndicator, jsDocParsingMode: host.jsDocParsingMode };
}

function findSourceFileWorker(fileName: string, isDefaultLib: boolean, ignoreNoDefaultLib: boolean, reason: FileIncludeReason, packageId: PackageId | undefined): SourceFile | undefined {
Expand Down
49 changes: 38 additions & 11 deletions src/compiler/scanner.ts
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ import {
DiagnosticMessage,
Diagnostics,
identity,
JSDocParsingMode,
JSDocSyntaxKind,
JsxTokenSyntaxKind,
KeywordSyntaxKind,
Expand All @@ -21,6 +22,7 @@ import {
parsePseudoBigInt,
positionIsSynthesized,
PunctuationOrKeywordSyntaxKind,
ScriptKind,
ScriptTarget,
SourceFileLike,
SyntaxKind,
Expand Down Expand Up @@ -95,6 +97,8 @@ export interface Scanner {
setOnError(onError: ErrorCallback | undefined): void;
setScriptTarget(scriptTarget: ScriptTarget): void;
setLanguageVariant(variant: LanguageVariant): void;
setScriptKind(scriptKind: ScriptKind): void;
setJSDocParsingMode(kind: JSDocParsingMode): void;
/** @deprecated use {@link resetTokenState} */
setTextPos(textPos: number): void;
resetTokenState(pos: number): void;
Expand All @@ -114,8 +118,6 @@ export interface Scanner {
// callback returns something truthy, then the scanner state is not rolled back. The result
// of invoking the callback is returned from this function.
tryScan<T>(callback: () => T): T;
/** @internal */
setSkipNonSemanticJSDoc(skip: boolean): void;
}

/** @internal */
Expand Down Expand Up @@ -345,10 +347,7 @@ const commentDirectiveRegExSingleLine = /^\/\/\/?\s*@(ts-expect-error|ts-ignore)
*/
const commentDirectiveRegExMultiLine = /^(?:\/|\*)*\s*@(ts-expect-error|ts-ignore)/;

/**
* Test for whether a comment contains a JSDoc tag needed by the checker when run in tsc.
*/
const semanticJSDocTagRegEx = /@(?:see|link)/i;
const jsDocSeeOrLink = /@(?:see|link)/i;

function lookupInUnicodeMap(code: number, map: readonly number[]): boolean {
// Bail out quickly if it couldn't possibly be in the map.
Expand Down Expand Up @@ -1008,7 +1007,8 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
var commentDirectives: CommentDirective[] | undefined;
var inJSDocType = 0;

var skipNonSemanticJSDoc = false;
var scriptKind = ScriptKind.Unknown;
var jsDocParsingMode = JSDocParsingMode.ParseAll;

setText(text, start, length);

Expand Down Expand Up @@ -1054,14 +1054,15 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
setText,
setScriptTarget,
setLanguageVariant,
setScriptKind,
setJSDocParsingMode,
setOnError,
resetTokenState,
setTextPos: resetTokenState,
setInJSDocType,
tryScan,
lookAhead,
scanRange,
setSkipNonSemanticJSDoc,
};
/* eslint-enable no-var */

Expand Down Expand Up @@ -2003,7 +2004,7 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
}
}

if (isJSDoc && (!skipNonSemanticJSDoc || semanticJSDocTagRegEx.test(text.slice(fullStartPos, pos)))) {
if (isJSDoc && shouldParseJSDoc()) {
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

tokenFlags |= TokenFlags.PrecedingJSDocComment;
}

Expand Down Expand Up @@ -2288,6 +2289,28 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
}
}

function shouldParseJSDoc() {
switch (jsDocParsingMode) {
case JSDocParsingMode.ParseAll:
return true;
case JSDocParsingMode.ParseNone:
return false;
}

if (scriptKind !== ScriptKind.TS && scriptKind !== ScriptKind.TSX) {
// If outside of TS, we need JSDoc to get any type info.
Copy link
Member

@weswigham weswigham Sep 18, 2023 •
edited

Choose a reason for hiding this comment

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

ScriptKind.JSON? AFAIK none of the comments we may read in a JSON document can modify the expression types, but we might actually accidentally allow it. ScriptKind.External is probably also safe to bail on. ScriptKind.Deferred I don't really know.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we ever actually make it to this check at all for those file types, so I felt safe not checking for them (and choosing the backwards-compatible option). But, I'll verify.

Copy link
Member Author

Choose a reason for hiding this comment

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

Flipping this to be "=== JS, === JSX" doesn't change any tests, but truthfully I don't know when the others are even used. I would feel safest with the conservative I have now, but am not strongly attached if someone knows better.

return true;
}

if (jsDocParsingMode === JSDocParsingMode.ParseForTypeInfo) {
// If we're in TS, but we don't need to produce reliable errors,
// we don't need to parse to find @see or @link.
return false;
}

return jsDocSeeOrLink.test(text.slice(fullStartPos, pos));
}

function reScanInvalidIdentifier(): SyntaxKind {
Debug.assert(token === SyntaxKind.Unknown, "'reScanInvalidIdentifier' should only be called when the current token is 'SyntaxKind.Unknown'.");
pos = tokenStart = fullStartPos;
Expand Down Expand Up @@ -2788,8 +2811,12 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
languageVariant = variant;
}

function setSkipNonSemanticJSDoc(skip: boolean) {
skipNonSemanticJSDoc = skip;
function setScriptKind(kind: ScriptKind) {
scriptKind = kind;
}

function setJSDocParsingMode(kind: JSDocParsingMode) {
jsDocParsingMode = kind;
}

function resetTokenState(position: number) {
Expand Down