Skip to content

Add transpileDeclaration API method #58261

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 8 commits into from
Apr 24, 2024
Merged

Add transpileDeclaration API method #58261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4e2febf
Add transpileDeclaration API entrypoint
weswigham Apr 17, 2024
425ffd0
Small changes, API baseline update
weswigham Apr 19, 2024
26cae5b
New transpile runner for harness, transpile tests and a bugfix
weswigham Apr 19, 2024
94ab02d
Merge branch 'main' into transpile-declarations
weswigham Apr 19, 2024
18c6810
Enable isolatedDeclarations, actually collect emit errors
weswigham Apr 19, 2024
b0a4b72
Lint feedback
weswigham Apr 19, 2024
6ee4762
Merge branch 'main' into transpile-declarations
weswigham Apr 23, 2024
764f861
Remove TODO comment, consensus is to keep these
weswigham Apr 23, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Enable isolatedDeclarations, actually collect emit errors
  • Loading branch information
@weswigham
weswigham committed Apr 19, 2024
commit 18c6810aa62c5ed2105998e31d29559c11ac4c1b
13 changes: 9 additions & 4 deletions src/services/transpile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,14 @@ const optionsRedundantWithVerbatimModuleSyntax = new Set([
* - declaration = false
*/
export function transpileModule(input: string, transpileOptions: TranspileOptions): TranspileOutput {
return transpileWorker(input, transpileOptions, /*declarations*/ false);
return transpileWorker(input, transpileOptions, /*declaration*/ false);
}

/*
* This function will create a declaration file from 'input' argument using specified compiler options.
* If no options are provided - it will use a set of default compiler options.
* Extra compiler options that will unconditionally be used by this function are:
* - isolatedDeclarations = true
* - isolatedModules = true
* - allowNonTsExtensions = true
* - noLib = true
Expand All @@ -78,7 +79,7 @@ export function transpileModule(input: string, transpileOptions: TranspileOption
* in that only types in the single input file are available to be used in the generated declarations.
*/
export function transpileDeclaration(input: string, transpileOptions: TranspileOptions): TranspileOutput {
return transpileWorker(input, transpileOptions, /*declarations*/ true);
return transpileWorker(input, transpileOptions, /*declaration*/ true);
}

// Declaration emit works without a `lib`, but some local inferences you'd expect to work won't without
Expand Down Expand Up @@ -106,7 +107,7 @@ interface Symbol {
readonly [Symbol.toStringTag]: string;
}`;
const barebonesLibName = "lib.d.ts";
Copy link
Member

Choose a reason for hiding this comment

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

Honestly this sorta weirds me out; is this something we should be formalizing in our real dts files somehow?
Copy link
Member Author

Choose a reason for hiding this comment

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

Eh, it's weirder that we can't typecheck unique symbol literals/calls without the skeleton of the global Symbol definitions present (since it's not like symbol types are ES6+ specific). That's probably the real underlying issue that needs fixing - we really aughta bake their presence into the checker like undefined if they're so central to checking a basic type-syntax construct. Though, there's probably some other annoying things, too, though - like I don't think we'll emit Omit types for generic spreads without the global Omit type present, which even this is currently missing. Eg,

export function f<T extends {x: number, y: number}>(x: T) {
    const {x: num, ...rest} = x;
    return rest;
}

has an inferred return type of Omit<T, "x">, but without Omit in the lib, it's just any (the error kind). There are a lot of "builtins" like this, whose fallback behavior when they aren't present isn't particularly good.

Personally, by default I think I'd rather load the fully correct lib for the options passed in, at least for transpileDeclaration, just because I think emitting export const a: any for export const a = Math.random() feels bad (even if it is an error under ID), but I don't know if that runs counter to the perf goals of the API (even if every invocation with the same lib could share cached lib AST copies). Still, I guess it comes down to a question of scope - loading a real lib to get the builtins to work is easy, but should you need to do that for isolatedDeclarations-compatible input? I appended this barebones lib to the input because I assumed at least unique symbol-related inferences are planned to work, and this is the minimal lib (well, OK, I could omit the empty Array interfaces and the like) to get that.

We could leave it up to the caller - and load the default lib for the options provided unless an override (like this minimal one or a completely empty/missing one) is passed in, or load no lib at all by default unless one is passed in as a transpile option. It's really a question of the intended usage, I suppose.
const barebonesLibSourceFile = createSourceFile(barebonesLibName, barebonesLibContent, {languageVersion: ScriptTarget.Latest});
const barebonesLibSourceFile = createSourceFile(barebonesLibName, barebonesLibContent, { languageVersion: ScriptTarget.Latest });

function transpileWorker(input: string, transpileOptions: TranspileOptions, declaration?: boolean): TranspileOutput {
const diagnostics: Diagnostic[] = [];
Expand Down Expand Up @@ -139,6 +140,7 @@ function transpileWorker(input: string, transpileOptions: TranspileOptions, decl
if (declaration) {
options.declaration = true;
options.emitDeclarationOnly = true;
options.isolatedDeclarations = true;
}
else {
options.declaration = false;
Expand Down Expand Up @@ -201,7 +203,10 @@ function transpileWorker(input: string, transpileOptions: TranspileOptions, decl
addRange(/*to*/ diagnostics, /*from*/ program.getOptionsDiagnostics());
}
// Emit
program.emit(/*targetSourceFile*/ undefined, /*writeFile*/ undefined, /*cancellationToken*/ undefined, /*emitOnlyDtsFiles*/ declaration, transpileOptions.transformers);
const result = program.emit(/*targetSourceFile*/ undefined, /*writeFile*/ undefined, /*cancellationToken*/ undefined, /*emitOnlyDtsFiles*/ declaration, transpileOptions.transformers, /*forceDtsEmit*/ declaration);
Copy link
Member

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 set forceDtsEmit this one generates d.ts file even if declaration options are not set correctly and thats not what we are looking for right? It also generates output even if there are diagnostics. is that intentional.

Eg normally d.ts is not generated for json but this will generate it and i dont think we want that.
Copy link
Member Author

@weswigham weswigham Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

is that intentional.

Yes. transpileModule only fails (with a debug assert!) when the output isn't emitted at all, and will happily emit buggy output alongside diagnostics (if you ask for them). This keeps that behavior aligned between the two.

forceDtsEmit this one generates d.ts file even if declaration options are not set correctly and thats not what we are looking for right?

We set those options right above. We need to set forceDtsEmit so we still get output when the input has a parse error (because being able to return those errors is contingent on there being output at all).
Copy link
Member Author

Choose a reason for hiding this comment

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

Eg normally d.ts is not generated for json but this will generate it and i dont think we want that.

Honestly, if someone explicitly passes json into this API, it's because they wanted a declaration file for it.
Copy link

Choose a reason for hiding this comment

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

JSON support sounds like a useful feature we might use. Our system treats JSON modules as frozen so potentially we could postprocess the output to express this readonly nature.
Copy link
Member

Choose a reason for hiding this comment

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

Just noting the differences between our normal emit vs setting forceDtsEmit which i had added for incremental checks

// TODO: Should this require `reportDiagnostics`?
Copy link
Member

Choose a reason for hiding this comment

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

Is it the case that the JS emit pipeline never reports errors?
Copy link
Member Author

Choose a reason for hiding this comment

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

Correct - emit pipeline errors were added for normal declaration emit when we ported them to transforms (and they add visibility errors very late) and are unused by js emit transforms -isolatedDeclarations continues to use the mechanism extensively. It's a longstanding issue I've wanted to fix, since it means we can't get declaration emit errors without actually running declaration emit (remind yourself how getPreEmitDiagnostics works some time 😑), but it's super annoying to try and decouple the errors from the node generation step.
addRange(/*to*/ diagnostics, /*from*/ result.diagnostics);

if (outputText === undefined) return Debug.fail("Output generation failed");

Expand Down
16 changes: 8 additions & 8 deletions src/testRunner/transpileRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ import {
TestCaseParser,
TestRunnerKind,
} from "./_namespaces/Harness";
import * as vpath from "./_namespaces/vpath";
import * as ts from "./_namespaces/ts";
import * as vpath from "./_namespaces/vpath";

export class TranspileRunner extends RunnerBase {
protected basePath: string = "tests/cases/transpile";
protected basePath = "tests/cases/transpile";
protected testSuiteName: TestRunnerKind = "transpile";

constructor() {
Expand Down Expand Up @@ -63,7 +63,7 @@ class TranspileTestCase {
const settingConfigurations = getFileBasedTestConfigurations(settings, TranspileTestCase.varyBy);
return settingConfigurations?.map(c => {
const desc = Object.entries(c).map(([key, value]) => `${key}=${value}`).join(",");
return new TranspileTestCase(`${justName}(${desc})`, ext, content, {...settings, ...c});
return new TranspileTestCase(`${justName}(${desc})`, ext, content, { ...settings, ...c });
}) || [new TranspileTestCase(justName, ext, content, settings)];
}

Expand Down Expand Up @@ -94,25 +94,25 @@ class TranspileTestCase {
baselineText += `//// [${unit.name}] ////\r\n`;
baselineText += unit.content;
if (!unit.content.endsWith("\n")) {
baselineText += "\r\n"
baselineText += "\r\n";
}
});

this.units.testUnitData.forEach(unit => {
const opts: ts.CompilerOptions = {};
Compiler.setCompilerOptionsFromHarnessSetting(this.settings, opts);
const result = (kind === TranspileKind.Module ? ts.transpileModule : ts.transpileDeclaration)(unit.content, { compilerOptions: opts, fileName: unit.name, reportDiagnostics: this.settings.reportDiagnostics === "true" });
baselineText += `//// [${ts.changeExtension(unit.name, kind === TranspileKind.Module ? this.getJsOutputExtension(unit.name) : ts.getDeclarationEmitExtensionForPath(unit.name) )}] ////\r\n`;

baselineText += `//// [${ts.changeExtension(unit.name, kind === TranspileKind.Module ? this.getJsOutputExtension(unit.name) : ts.getDeclarationEmitExtensionForPath(unit.name))}] ////\r\n`;
baselineText += result.outputText;
if (!result.outputText.endsWith("\n")) {
baselineText += "\r\n"
baselineText += "\r\n";
}
if (result.diagnostics && result.diagnostics.length) {
baselineText += "\r\n\r\n//// [Diagnostics reported]\r\n";
baselineText += Compiler.getErrorBaseline([{ content: unit.content, unitName: unit.name }], result.diagnostics, !!opts.pretty);
if (!baselineText.endsWith("\n")) {
baselineText += "\r\n"
baselineText += "\r\n";
}
}
});
Expand Down
27 changes: 27 additions & 0 deletions tests/baselines/reference/transpile/declarationBasicSyntax.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,33 @@ export declare abstract class Baz {
abstract method(): void;
}
export {};


//// [Diagnostics reported]
class.ts(1,7): error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.


==== class.ts (1 errors) ====
const i = Symbol();
~
!!! error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.
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'm sure this specific pattern (and the Symbol.for variant) is on the backlog of things to remove the isolatedDeclarations error from, right? Since unique symbols are so hard to use without const x = Symbol() allowing an inferred unique symbol type.
Copy link
Member

Choose a reason for hiding this comment

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

Yes; the problem is symbol freshness/widening. I tried my hand at fixing it but was unsuccessful. See also #55943 #55901
!!! related TS9027 class.ts:1:7: Add a type annotation to the variable i.
export class Bar {
a: string;
b?: string;
declare c: string;
#d: string;
public e: string;
protected f: string;
private g: string;
["h"]: string;
[i]: string;
}

export abstract class Baz {
abstract a: string;
abstract method(): void;
}
//// [namespace.d.ts] ////
export declare namespace ns {
namespace internal {
Expand Down
26 changes: 26 additions & 0 deletions tests/baselines/reference/transpile/declarationCrossFileInferences.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,31 @@ export declare class A {
}
//// [consumes.d.ts] ////
export declare function create(): any;


//// [Diagnostics reported]
consumes.ts(2,17): error TS9007: Function must have an explicit return type annotation with --isolatedDeclarations.


==== consumes.ts (1 errors) ====
import {A} from "./defines.js";
export function create() {
~~~~~~
!!! error TS9007: Function must have an explicit return type annotation with --isolatedDeclarations.
!!! related TS9031 consumes.ts:2:17: Add a return type to the function declaration.
return new A();
}
//// [exposes.d.ts] ////
export declare const value: any;


//// [Diagnostics reported]
exposes.ts(2,14): error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.


==== exposes.ts (1 errors) ====
import {create} from "./consumes.js";
export const value = create();
~~~~~
!!! error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.
!!! related TS9027 exposes.ts:2:14: Add a type annotation to the variable value.
11 changes: 11 additions & 0 deletions tests/baselines/reference/transpile/declarationSingleFileHasErrors.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,14 @@
export const a number = "missing colon";
//// [declarationSingleFileHasErrors.d.ts] ////
export declare const a: any, number = "missing colon";


//// [Diagnostics reported]
declarationSingleFileHasErrors.ts(1,14): error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.


==== declarationSingleFileHasErrors.ts (1 errors) ====
export const a number = "missing colon";
~
!!! error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.
!!! related TS9027 declarationSingleFileHasErrors.ts:1:14: Add a type annotation to the variable a.
6 changes: 5 additions & 1 deletion tests/baselines/reference/transpile/declarationSingleFileHasErrorsReported.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,14 @@ export declare const a: any, string = "missing colon";


//// [Diagnostics reported]
declarationSingleFileHasErrorsReported.ts(1,14): error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.
declarationSingleFileHasErrorsReported.ts(1,16): error TS1005: ',' expected.


==== declarationSingleFileHasErrorsReported.ts (1 errors) ====
==== declarationSingleFileHasErrorsReported.ts (2 errors) ====
export const a string = "missing colon";
~
!!! error TS9010: Variable must have an explicit type annotation with --isolatedDeclarations.
!!! related TS9027 declarationSingleFileHasErrorsReported.ts:1:14: Add a type annotation to the variable a.
~~~~~~
!!! error TS1005: ',' expected.