Skip to content

Make JSDoc skipping public, plus more configurable #55739

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 26 commits into from
Sep 21, 2023
Merged

Make JSDoc skipping public, plus more configurable #55739

Show file tree
Hide file tree
Changes from 8 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 20 additions & 21 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ import {
JSDocOverloadTag,
JSDocOverrideTag,
JSDocParameterTag,
JSDocParsingMode,
JSDocPrivateTag,
JSDocPropertyLikeTag,
JSDocPropertyTag,
Expand Down Expand Up @@ -1324,10 +1325,7 @@ 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, jsDocParsingMode?: JSDocParsingMode): SourceFile {
tracing?.push(tracing.Phase.Parse, "createSourceFile", { path: fileName }, /*separateBeginAndEnd*/ true);
performance.mark("beforeParse");
let result: SourceFile;
Expand All @@ -1339,14 +1337,14 @@ export function createSourceFile(fileName: string, sourceText: string, languageV
impliedNodeFormat: format,
} = 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 +1584,7 @@ namespace Parser {
setParentNodes = false,
scriptKind?: ScriptKind,
setExternalModuleIndicatorOverride?: (file: SourceFile) => void,
skipNonSemanticJSDoc?: boolean,
jsDocParsingMode = JSDocParsingMode.KeepAll,
): SourceFile {
scriptKind = ensureScriptKind(fileName, scriptKind);
if (scriptKind === ScriptKind.JSON) {
Expand All @@ -1601,10 +1599,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 +1610,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.KeepAll);
// Prime the scanner.
nextToken();
const entityName = parseEntityName(/*allowReservedWords*/ true);
Expand All @@ -1623,7 +1620,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.KeepAll);
sourceFlags = contextFlags;

// Prime the scanner.
Expand Down Expand Up @@ -1711,7 +1708,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 +1749,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.KeepAll);

// Clear any data. We don't want to accidentally hold onto it for too long.
sourceText = undefined!;
Expand All @@ -1777,7 +1776,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 +1803,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 +8685,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.KeepAll);
scanner.setText(content, start, length);
currentToken = scanner.scan();
const jsDocTypeExpression = parseJSDocTypeExpression();
Expand Down Expand Up @@ -8736,7 +8735,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.KeepAll);
const jsDoc = doInsideOfContext(NodeFlags.JSDoc, () => parseJSDocCommentWorker(start, length));

const sourceFile = { languageVariant: LanguageVariant.Standard, text: content } as SourceFile;
Expand Down Expand Up @@ -9804,7 +9803,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 +9866,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
13 changes: 7 additions & 6 deletions src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ import {
isStringLiteral,
isStringLiteralLike,
isTraceEnabled,
JSDocParsingMode,
JsonSourceFile,
JsxEmit,
length,
Expand Down Expand Up @@ -393,16 +394,16 @@ export function computeCommonSourceDirectoryOfFilenames(fileNames: readonly stri
return getPathFromPathComponents(commonPathComponents);
}

export function createCompilerHost(options: CompilerOptions, setParentNodes?: boolean): CompilerHost {
return createCompilerHostWorker(options, setParentNodes);
export function createCompilerHost(options: CompilerOptions, setParentNodes?: boolean, jsDocParsingMode?: JSDocParsingMode): CompilerHost {
Copy link
Member

@sheetalkamat sheetalkamat Sep 15, 2023

Choose a reason for hiding this comment

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

I dont think we should be threading in this to defaults. User of this API can always call thius method and override getSourceFile on their own .. there is nothing special here.

Copy link
Member Author

@jakebailey jakebailey Sep 15, 2023

Choose a reason for hiding this comment

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

I think that not providing this parameter will make it very inconvenient; this function and createCompilerHostWorker are what call createGetSourceFile, which is internal and non-trivial, and then wrap it again in another closure. Setting the JSDoc parsing mode is a lot like setting a default setParentNodes which this function already does.

Copy link
Member Author

@jakebailey jakebailey Sep 15, 2023

Choose a reason for hiding this comment

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

See: https://github.com/typescript-eslint/typescript-eslint/blob/7826910d8c6ba4d2d40b8d55fcf7fd39fe290d88/packages/typescript-estree/src/create-program/useProvidedPrograms.ts#L84

Copy link
Member Author

@jakebailey jakebailey Sep 15, 2023

Choose a reason for hiding this comment

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

Generally speaking I think this means that anywhere we can expect to find setParentNodes, we probably need to be expecting jsDocParsingMode.

Copy link
Member

@sheetalkamat sheetalkamat Sep 18, 2023

Choose a reason for hiding this comment

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

Hmm.. Generally i like to go with conservative approach and avoid having to add additional surface area that we have to keep maintaining. This is ok i guess though , user can just create source file from file text easily to override but ok.

Copy link
Member

@weswigham weswigham Sep 18, 2023

Choose a reason for hiding this comment

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

If the option is fed in via the CreateSourceFileOptions, rather than a top-level field, callers will already have the opportunity to request it on a per-file basis when making the file, rather than needing to set it on the host as a whole.

return createCompilerHostWorker(options, setParentNodes, /*system*/ undefined, jsDocParsingMode);
}

/** @internal */
export function createGetSourceFile(
readFile: ProgramHost<any>["readFile"],
getCompilerOptions: () => CompilerOptions,
setParentNodes: boolean | undefined,
skipNonSemanticJSDocParsing: boolean | undefined,
jsDocParsingMode: JSDocParsingMode | undefined,
): CompilerHost["getSourceFile"] {
return (fileName, languageVersionOrOptions, onError) => {
let text: string | undefined;
Expand All @@ -418,7 +419,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, /*scriptKind*/ undefined, jsDocParsingMode) : undefined;
};
}

Expand Down Expand Up @@ -459,8 +460,8 @@ export function createWriteFileMeasuringIO(
export function createCompilerHostWorker(
options: CompilerOptions,
setParentNodes?: boolean,
skipNonSemanticJSDocParsing?: boolean,
system: System = sys,
jsDocParsingMode?: JSDocParsingMode,
): CompilerHost {
const existingDirectories = new Map<string, boolean>();
const getCanonicalFileName = createGetCanonicalFileName(system.useCaseSensitiveFileNames);
Expand All @@ -482,7 +483,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, jsDocParsingMode),
getDefaultLibLocation,
getDefaultLibFileName: options => combinePaths(getDefaultLibLocation(), getDefaultLibFileName(options)),
writeFile: createWriteFileMeasuringIO(
Expand Down
29 changes: 21 additions & 8 deletions src/compiler/scanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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 @@ -1008,7 +1010,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.KeepAll;

setText(text, start, length);

Expand Down Expand Up @@ -1054,14 +1057,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,8 +2007,13 @@ export function createScanner(languageVersion: ScriptTarget, skipTrivia: boolean
}
}

if (isJSDoc && (!skipNonSemanticJSDoc || semanticJSDocTagRegEx.test(text.slice(fullStartPos, pos)))) {
tokenFlags |= TokenFlags.PrecedingJSDocComment;
if (isJSDoc) {
const shouldParseJSDoc = jsDocParsingMode === JSDocParsingMode.SkipAll ? false
: jsDocParsingMode === JSDocParsingMode.KeepSemanticOnly ? (scriptKind !== ScriptKind.TS && scriptKind !== ScriptKind.TSX) || semanticJSDocTagRegEx.test(text.slice(fullStartPos, pos))
: true;
if (shouldParseJSDoc) {
tokenFlags |= TokenFlags.PrecedingJSDocComment;
}
}

commentDirectives = appendIfCommentDirective(commentDirectives, text.slice(lastLineStart, pos), commentDirectiveRegExMultiLine, lastLineStart);
Expand Down Expand Up @@ -2788,8 +2797,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
Loading