Make incomplete scanner APIs internal

[cubenoy22]

Suggestion

🔍 Search Terms

• scanner + label:API
• SourceFileLike
• getPositionOfLineAndCharacter
• getLineAndCharacterOfPosition

✅ Viability Checklist

My suggestion meets these guidelines:

• This wouldn't be a breaking change in existing TypeScript/JavaScript code
• This wouldn't change the runtime behavior of existing JavaScript code
• This could be implemented without emitting different JS based on the types of the expressions
• This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
• This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

• ts.getPositionOfLineAndCharacter should be marked with /* @internal */
• ts.getLineAndCharacterOfPosition should be marked with /* @internal */

These methods take an argument of SourceFileLike at first, and its type is defined as:

    /* @internal */
    /**
     * Subset of properties from SourceFile that are used in multiple utility functions
     */
    export interface SourceFileLike {
        readonly text: string;
        lineMap?: readonly number[];
        /* @internal */
        getPositionOfLineAndCharacter?(line: number, character: number, allowEdits?: true): number;
    }

in src/compiler/types.ts and also defined:

    export interface SourceFileLike {
        getLineAndCharacterOfPosition(pos: number): LineAndCharacter;
    }

in src/services/types.ts. so, required implementation of SourceFileLike in public is only getLineAndCharacterOfPosition. But two exposed methods are uses all internal-marked members in actual (occurs runtime error).

💻 Use Cases

After this changes, These methods in SourceFile is still available:

• getLineAndCharacterOfPosition
• getPositionOfLineAndCharacter

[DanielRosenwasser]

@sheetalkamat @sandersn any thoughts on this one?

[sandersn]

I'm not familiar with these functions.
@cubenoy22 Are you saying that both ts and SourceFile have members named (1) getLineAndCharacterOfPosition (2) getPositionOfLineAndCharacter?

If I'm reading your proposal correctly, I think you want to remove the properties on ts because they error when called. Is that correct?

[sheetalkamat]

I dont understand the suggestion here at all..
getPositionOfLineAndCharacter on ts handles if there is getPositionOfLineAndCharacter on sourceFileLike or not so why should that be issue.

[cubenoy22]

@sandersn

Are you saying that both ts and SourceFile have members named (1) getLineAndCharacterOfPosition (2) getPositionOfLineAndCharacter?

Exactly.

@sandersn @sheetalkamat

Let me explain about two functions.

SourceFile is declared here:

TypeScript/src/services/types.ts

Lines 90 to 104 in 3d24b85

	export interface SourceFile {
	/* @internal */ version: string;
	/* @internal */ scriptSnapshot: IScriptSnapshot | undefined;
	/* @internal */ nameTable: UnderscoreEscapedMap<number> | undefined;
	/* @internal */ getNamedDeclarations(): ESMap<string, readonly Declaration[]>;
	getLineAndCharacterOfPosition(pos: number): LineAndCharacter;
	getLineEndOfPosition(pos: number): number;
	getLineStarts(): readonly number[];
	getPositionOfLineAndCharacter(line: number, character: number): number;
	update(newText: string, textChangeRange: TextChangeRange): SourceFile;
	/* @internal */ sourceMapper?: DocumentPositionMapper;
	}

These methods are easy to use (just call them).

And There are also two exported functions in scanner.ts:

TypeScript/src/compiler/scanner.ts

Lines 387 to 394 in 5fde871

	export function getPositionOfLineAndCharacter(sourceFile: SourceFileLike, line: number, character: number): number;
	/* @internal */
	export function getPositionOfLineAndCharacter(sourceFile: SourceFileLike, line: number, character: number, allowEdits?: true): number; // eslint-disable-line @typescript-eslint/unified-signatures
	export function getPositionOfLineAndCharacter(sourceFile: SourceFileLike, line: number, character: number, allowEdits?: true): number {
	return sourceFile.getPositionOfLineAndCharacter ?
	sourceFile.getPositionOfLineAndCharacter(line, character, allowEdits) :
	computePositionOfLineAndCharacter(getLineStarts(sourceFile), line, character, sourceFile.text, allowEdits);
	}

TypeScript/src/compiler/scanner.ts

Lines 470 to 472 in 5fde871

	export function getLineAndCharacterOfPosition(sourceFile: SourceFileLike, position: number): LineAndCharacter {
	return computeLineAndCharacterOfPosition(getLineStarts(sourceFile), position);
	}

Both functions take sourceFile: SourceFileLike parameter at first.
Of course, It is OK to pass SourceFile instances despite of SourceFile has own methods.
But I thought SourceFileLike is constructable like PromiseLike and ArrayLike:

  ts.getPositionOfLineAndCharacter({
    getLineAndCharacterOfPosition: () => /* ... */,
  }, line, number);

And I found a construction case in this project:

TypeScript/src/services/textChanges.ts

Line 1025 in db01e84

const file: SourceFileLike = { text, getLineAndCharacterOfPosition(pos) { return getLineAndCharacterOfPosition(this, pos); } };

My above code is buildable despite of lacking text: string on any projects using TypeScript because SourceFileLike as public is here:

TypeScript/lib/typescript.d.ts

Lines 5339 to 5341 in 5fde871

	interface SourceFileLike {
	getLineAndCharacterOfPosition(pos: number): LineAndCharacter;
	}

But as I wrote in description, Two functions (not methods of SourceFile) use /** internal */ marked members of SourceFileLike (I explain on below).

---

Should we expose only text: string on SourceFileLike instead of marking /** @internal */ to two functions?

getLineStarts function which is called in getLineAndCharacterOfPosition and getPositionOfLineAndCharacter mutates lineMap of SourceFileLike to cache. So lineMap?: readonly number[]; should be exposed too I think.

[sheetalkamat]

Thank you for explaination.. now i see what you mean.. The actual definition of SourceFileLike in compiler is internal but not in services so public one doesnt have the right definition. Got it

[sheetalkamat]

I think we should make SourceFileLike in compiler\types.ts not internal but make probably make lineMap internal.

[cubenoy22]

Thank you for your reply. Let me write my thoughts. SourceFileLike in compiler\types.ts is here:

TypeScript/src/compiler/types.ts

Lines 3460 to 3469 in 9aa50d6

	/* @internal */
	/**
	* Subset of properties from SourceFile that are used in multiple utility functions
	*/
	export interface SourceFileLike {
	readonly text: string;
	lineMap?: readonly number[];
	/* @internal */
	getPositionOfLineAndCharacter?(line: number, character: number, allowEdits?: true): number;
	}

• Removing /** @internal */ at L.3460 exposes text and lineMap properties
  • L.3466 lineMap?: readonly number[]; is wrong definition (Built-in functions mutate this property if not exists), so applying /** @internal */ looks good for me
    • Adding lines field by the library is not expected behavior for users, so should we change its name to _lines and so on?
• This changes public APIs so "acknowledge required" before we make a pull request

Impact to internal codes

• Exposing and marking /** @internal */ cause no changes to codes
  • We need to change baseline
• If we rename lines to _lines, It must be fully cared

Impact to external codes

• I searched SourceFileLike in typescript.d.ts and I found another function using SourceFileLike here
  , so only three functions expose SourceFileLike to public now.
• SourceFileLike is not usable alone because of lacking text property (No one use these functions or pass SourceFile instances, I think)
• text property is a member of SourceFile:

  • TypeScript/src/compiler/types.ts

    Lines 3456 to 3565 in 3d24b85

    	export interface SourceFile extends Declaration {
    	readonly kind: SyntaxKind.SourceFile;
    	readonly statements: NodeArray<Statement>;
    	readonly endOfFileToken: Token<SyntaxKind.EndOfFileToken>;
    	fileName: string;
    	/* @internal */ path: Path;
    	text: string;
    	/** Resolved path can be different from path property,
    	* when file is included through project reference is mapped to its output instead of source
    	* in that case resolvedPath = path to output file
    	* path = input file's path
    	*/
    	/* @internal */ resolvedPath: Path;
    	/** Original file name that can be different from fileName,
    	* when file is included through project reference is mapped to its output instead of source
    	* in that case originalFileName = name of input file
    	* fileName = output file's name
    	*/
    	/* @internal */ originalFileName: string;
    	/**
    	* If two source files are for the same version of the same package, one will redirect to the other.
    	* (See `createRedirectSourceFile` in program.ts.)
    	* The redirect will have this set. The redirected-to source file will be in `redirectTargetsMap`.
    	*/
    	/* @internal */ redirectInfo?: RedirectInfo;
    	amdDependencies: readonly AmdDependency[];
    	moduleName?: string;
    	referencedFiles: readonly FileReference[];
    	typeReferenceDirectives: readonly FileReference[];
    	libReferenceDirectives: readonly FileReference[];
    	languageVariant: LanguageVariant;
    	isDeclarationFile: boolean;
    	// this map is used by transpiler to supply alternative names for dependencies (i.e. in case of bundling)
    	/* @internal */
    	renamedDependencies?: ReadonlyESMap<string, string>;
    	/**
    	* lib.d.ts should have a reference comment like
    	*
    	* /// <reference no-default-lib="true"/>
    	*
    	* If any other file has this comment, it signals not to include lib.d.ts
    	* because this containing file is intended to act as a default library.
    	*/
    	hasNoDefaultLib: boolean;
    	languageVersion: ScriptTarget;
    	/* @internal */ scriptKind: ScriptKind;
    	/**
    	* The first "most obvious" node that makes a file an external module.
    	* This is intended to be the first top-level import/export,
    	* but could be arbitrarily nested (e.g. `import.meta`).
    	*/
    	/* @internal */ externalModuleIndicator?: Node;
    	// The first node that causes this file to be a CommonJS module
    	/* @internal */ commonJsModuleIndicator?: Node;
    	// JS identifier-declarations that are intended to merge with globals
    	/* @internal */ jsGlobalAugmentations?: SymbolTable;
    	/* @internal */ identifiers: ESMap<string, string>; // Map from a string to an interned string
    	/* @internal */ nodeCount: number;
    	/* @internal */ identifierCount: number;
    	/* @internal */ symbolCount: number;
    	// File-level diagnostics reported by the parser (includes diagnostics about /// references
    	// as well as code diagnostics).
    	/* @internal */ parseDiagnostics: DiagnosticWithLocation[];
    	// File-level diagnostics reported by the binder.
    	/* @internal */ bindDiagnostics: DiagnosticWithLocation[];
    	/* @internal */ bindSuggestionDiagnostics?: DiagnosticWithLocation[];
    	// File-level JSDoc diagnostics reported by the JSDoc parser
    	/* @internal */ jsDocDiagnostics?: DiagnosticWithLocation[];
    	// Stores additional file-level diagnostics reported by the program
    	/* @internal */ additionalSyntacticDiagnostics?: readonly DiagnosticWithLocation[];
    	// Stores a line map for the file.
    	// This field should never be used directly to obtain line map, use getLineMap function instead.
    	/* @internal */ lineMap: readonly number[];
    	/* @internal */ classifiableNames?: ReadonlySet<__String>;
    	// Comments containing @ts-* directives, in order.
    	/* @internal */ commentDirectives?: CommentDirective[];
    	// Stores a mapping 'external module reference text' -> 'resolved file name' | undefined
    	// It is used to resolve module names in the checker.
    	// Content of this field should never be used directly - use getResolvedModuleFileName/setResolvedModuleFileName functions instead
    	/* @internal */ resolvedModules?: ESMap<string, ResolvedModuleFull | undefined>;
    	/* @internal */ resolvedTypeReferenceDirectiveNames: ESMap<string, ResolvedTypeReferenceDirective | undefined>;
    	/* @internal */ imports: readonly StringLiteralLike[];
    	// Identifier only if `declare global`
    	/* @internal */ moduleAugmentations: readonly (StringLiteral | Identifier)[];
    	/* @internal */ patternAmbientModules?: PatternAmbientModule[];
    	/* @internal */ ambientModuleNames: readonly string[];
    	/* @internal */ checkJsDirective?: CheckJsDirective;
    	/* @internal */ version: string;
    	/* @internal */ pragmas: ReadonlyPragmaMap;
    	/* @internal */ localJsxNamespace?: __String;
    	/* @internal */ localJsxFragmentNamespace?: __String;
    	/* @internal */ localJsxFactory?: EntityName;
    	/* @internal */ localJsxFragmentFactory?: EntityName;
    	/* @internal */ exportedModulesFromDeclarationEmit?: ExportedModulesFromDeclarationEmit;
    	/* @internal */ endFlowNode?: FlowNode;
    	}

  • So, exposing text: string causes no impact to any codes passing SourceFile instances

[cubenoy22]

And I'll try this fix. if acknowledged to change public API.