Isolated declarations

[dragomirtitian]

Fixes #47947

Design Goals

When adding this new flag to TypeScript, these are the design goals we had in mind:

1. No inferred type information should be used in declaration emit.
2. Declaration emit should be a syntactic transformation that adds type information present in the current statement only. (i.e. It does not use information from outside the file, such as types of globals or imports).
3. Declaration emit can be done on a per-file basis, without information in other files.
4. An external declaration emitter tool can emit the exact same declaration file as TypeScript if the original source code does not have syntactic or semantic errors.
5. An external declaration emitter tool should be able to make a best effort to emit the same declarations as TypeScript if the original source code has semantic or syntactic errors.

Non goals:

1. Replicate the inference behavior of TypeScript. Even for trivial cases a declaration emitter should not be required to infer types from values. If values can’t be preserved in declaration files, explicit type annotations will be required.
2. Preserve the current declaration emit.

Status

The work-in-progress branch is here which implements the flag:
https://github.com/bloomberg/TypeScript/tree/isolated-declarations

The new standalone declaration emitter is located as a sub-package here:
https://github.com/bloomberg/TypeScript/tree/isolated-declarations/external-declarations

There are three deliverables:

1. Error generation in the compiler to enforce the opt-in flag-enabled source code
   restrictions.
   • These error conditions are described below as "Restrictions".
2. A standalone declaration emitter
   • This implementation uses the the parser and printer from the TypeScript code base but replaces the binder and emit resolver with rewritten versions that do not depend on the type checker.
   • We should decide if this emitter should remain in the TS repository or be spinned out into its own repository. Code changes are still needed to it in either scenario.
3. Conformance tests for the new declaration emit
   • These are designed to be reusable to enable the ecosystem to create compliant declaration emitters, e.g. in the esbuild and swc open-source compilers.

Restrictions

When --isolatedDeclarations is enabled, TypeScript will enforce stricter rules than usual on the user's source code. These restrictions are necessary in order to keep the Declaration Emit to be a simple one-in-one-out file transform.

R1. Do not print synthetic types

This is the main restriction that enables isolated declarations to work. Type annotations will be required on:

• Function/Method/Accessor parameters
• Function/Method/Accessor return types
• Variables
• Public/protected class fields

The requirement only exists if the symbol makes it into the declaration file (directly by exporting or indirectly by referencing in another exported symbol)

For example these are now errors with isolated declarations:

export class Test {
    x // error under isolated declarations
    private y = 0; // no error, private field types are not serialized in declarations
    #z = 1;// no error, fields is not present in declarations
    constructor(x: number) {
        this.x = 1;
    }
    get a() { // error under isolated declarations
        return 1;
    }
    set a(value) { // error under isolated declarations
        this.x = 1;
    }
}


export function test () { // error under isolated declarations
    return 1;
}


function privateFn() {} // no error, not exported


function indirect() { return 0; } // error, makes it into declarations via ReturnTypeOfIndirect
export type ReturnTypeOfIndirect = ReturnType<typeof indirect>

The workaround is to add explicit type annotations as required.

R2. Default exports

Default exports can’t have a type annotation. If the default export is an expression, TypeScript currently transforms it to a variable with a synthesized type annotation and rewrites the default export to use this new variable:

// index.ts
export default {
    foo: 1
}


// index.d.ts
declare const _default: {
    foo: number;
};
export default _default;

Isolated declarations can’t synthesize new types so such a construct is effectively forbidden. To work around this, users will have to do manually what declaration emit does automatically, namely create a separate variable declaration and add an explicit type annotation to that variable.

// index.ts
const _default: {
    foo: number
} = {
    foo: 1
}
export default _default

See Possible DX improvements for potential improvements.

R3. Expando functions

What we mean by expando functions are functions that have extra properties attached to them:

// index.ts
function myFunction() {
  return 0;
}
myFunction.foo = "";


// index.d.ts
declare function myFunction(): number;
declare namespace myFunction {
    var foo: string;
}

While determining the properties to be added to the generated namespace associated with the function is a syntactic process that could be supported in isolated declarations, there is no place to place annotations on the added properties (ex foo). Since there is no place to add the type annotation on these extra members there is no way to preserve this behavior in isolated declaration mode.

The workaround is to use namespace-function merging to declare the properties. This was the recommendation to achieve this before expado-functions were supported.

// index.ts
function myFunction() {
  return 0;
}
declare namespace myFunction {
    var foo: string;
}
myFunction.foo = "";

See Possible DX improvements for potential improvements.

R4. No synthetic extends clauses

For patterns where the extends clause of a class is an expression, current declaration would produce an new variable and add the type of the extends clause to the new variable, and then change the extends

//index.ts
export function mixin<T extends new (...a: any[]) => any>(cls: T): T {
    return class extends cls { }
}


export class Base { }
export class Mixed extends mixin(Base) { }


// index.d.ts
export declare function mixin<T extends new(...a: any[])=>any>(cls: T): T;
export declare class Base { }
declare const Mixed_base: typeof Base;
export declare class Mixed extends Mixed_base { }

Isolated declarations emit can’t synthetic types for the generated variable, so this pattern can’t really be supported in this mode.

The workaround is to manually create a base class variable in code and use that in the extends clause (possibly using some for of relative typing to not have to duplicate the class type):

//index.ts
export function mixin<T extends new (...a: any[]) => any>(cls: T): T {
    return class extends cls { }
}


class Base {}
const Mixed_Base: ReturnType<typeof mixin<typeof Base>> = mixin(Base);
export class Mixed extends Mixed_Base { }

R5. No expressions in enums

TypeScript will compute the value of enum members that are assigned a value.
Isolated declarations can’t easily allow this as the expressions can contain cross module expressions:

//index.ts
export enum E1 { A = 1 }


export enum E2 { A = E1.A }

Under isolated declarations we will get an error in E2 as we are assigning a computed value of E1.A

See Possible DX improvements for potential improvements.

R6. Destructuring in declarations

Currently declaration emit flattens out variables that result from destructuring:

// index.ts
type T = [{ foo: string }]
export const [{foo}]: T = null!
// index.d.ts
export declare const foo: string;

This approach requires type information that an external tool would not have. A workaround would be to not use destructuring as part of anything that makes it into the declaration file:

// index.ts
type T = [{ foo: string }]
const o: T = null!
export const foo: string = o[0].foo

See Possible DX improvements for potential improvements.

R7. Import required by augmentation is an error

Currently TypeScript removes imports from declarations if none of their types as used in the declaration. This is generally generally safe, except for the case when the import actually augments the module, such as in this example (taken from
tests\cases\compiler\declarationEmitForModuleImportingModuleAugmentationRetainsImport.ts)

// @declaration: true
// @filename: child1.ts
import { ParentThing } from './parent';


declare module './parent' {
    interface ParentThing {
        add: (a: number, b: number) => number;
    }
}

export function child1(prototype: ParentThing) {
    prototype.add = (a: number, b: number) => a + b;
}

// @filename: parent.ts
import { child1 } from './child1'; // this import should still exist in some form in the output, since it augments this module

export class ParentThing implements ParentThing {}

child1(ParentThing.prototype);

The current version of isolated declarations would make this code an error, as there is no way from parent.ts to tell that the import of child1 is required for its augmentation of parent.ts and thus will be removed by a tool without knowledge of this.

Emit Changes

These are notable changes in the generated output from the new standalone Declaration Emitter compared to the pre-existing declaration emit.

E1. Do not remove property aliases from binding elements in function parameters

Typescript will attempt to remove the renaming of binding element in function parameters, but is inconsistent about doing this:

// index.ts
function test({ foo: value} : {foo: string}) {
    value; // remove this and value is preserved in declarations
}
// index.d.ts if value is used in the function body
declare function test({ foo: value }: { foo: string; }): void;
// index.d.ts if value NOT is used in the function body
declare function test({ foo }: { foo: string; }): void;

The example above is due to the fact that when deciding whether to preserve the rename or not the declaration emitter uses information about general usage of the variable rather than information about whether it is used in the function signature (which does not exist at present)

In order to ensure consistency, at least for now this rename is always preserved in declaration files even if it is unused. Improving the situation around this is a separate issue that when implemented can be supported in isolated declaration. (An isolated declaration emitter could track the usage of the renamed symbol in the function signature)

E2. Print initializers as written

TypeScript will normalize literal values in declaration files:

// index.ts
const n1 = 0xFF;
const n2 = 1_000_000;
const y = `A`
const z = 'B'


// index.d.ts
declare const n1 = 255;
declare const n2 = 1000000;
declare const y = "A";
declare const z = "B";

To simplify emit, under isolated declarations initializers are printed as written. Potentially we could expand this to regular declaration emit. This seems like a better DX, the initializers usually use a specific syntax for reasons of readability so preserving the original text might be a good idea.

E3. Do not consider external symbol meaning in isolated declarations

This means that if an imported symbol has a meaning (type vs value) that is different from the meaning it is used in the declaration, we might keep unused imports that are preserved in the declaration file:

// @filename: /ref.d.ts
export interface $ { x }


// @filename: /types/lib/index.d.ts
declare let $: { x: number }


// @filename: /app.ts
/// <reference types="lib"/>
import {$} from "./ref";
export interface A {
    x: typeof $;
}

The test above will generate for app.ts the following declaration under isolated declarations:

// /app.d.ts
/// <reference types="lib" />
import { $ } from "./ref";
export interface A {
    x: typeof $;
}

But this declaration without isolated declarations:

// @filename: /app.d.ts
/// <reference types="lib" />
export interface A {
    x: typeof $;
}

This is because $ is used as a value, but imported as a type. Using the type checker, the compiler can figure out that the imported $ is a type and thus the import is unused in the resulting declaration. In isolated declaration mode, we don’t actually have this information. Leaving the import behind seems bening as it will essentially be unused.

Note: A suggestion to ensure this is not a problem is to use verbatim imports with isolated declarations

Implementation notes

I1. Transformations of paths in path reference directives

In order to generate the same declarations as TypeScript, we have to perform some manipulation of paths in /// <reference path="..." /> directives:

Remove paths that include node_modules

Test that express this behavior: umd-augmentation-2,`umd-augmentation-4``
This behavior is hardcoded in declaration emit:

// src\compiler\transformers\declarations.ts > mapReferencesIntoArray
// omit references to files from node_modules (npm may disambiguate module
// references when installing this package, making the path is unreliable).
if (startsWith(fileName, "node_modules/") || pathContainsNodeModules(fileName)) {
    return;
}

Remove references to files that are not part of the current project

This requires knowing beforehand what files are part of the project. This does not violate the design goal of not depending on information in other files as it does not require looking into the file itself, only knowing what other files exist.

When trying to find files, if the referenced file does not have an extension we will try to find a file based on a list of known extension (.ts, .d.ts, .cts, .d.cts, .d.mts, .d.mts).

Test that show this behavior: duplicateIdentifierRelatedSpans6, duplicateIdentifierRelatedSpans7, fileReferencesWithNoExtensions, moduleAugmentationDuringSyntheticDefaultCheck

If the file is not found the /// <reference path="..." /> directive should be removed.

Test that show this behavior

declarationEmitInvalidReference2, declarationEmitInvalidReferenceAllowJs, invalidTripleSlashReference, selfReferencingFile2, parserRealSource1, parserRealSource10, parserRealSource11, parserRealSource12, parserRealSource13, parserRealSource14, parserRealSource2, parserRealSource3, parserRealSource4, parserRealSource5, parserRealSource6, parserRealSource7, parserRealSource8, parserRealSource9, parserharness, parserindenter, scannertest1, consumer, matchFiles

Change extension to appropriate declaration extension

After finding the file in the current project, if the extension is not a declaration file extension (a file that ends with .d.ts, .d.mts, .d.cts or .d.ext.ts, .d.ext.mts, .d.ext.cts - the latter 3 are for --allowArbitraryExtensions flag) change the file extension to a declaration file extension appropriate for the original file type ( .mjs and .mts become .d.mts, .cjs and .cts become .d.cts, other extension become .d.ts)

Example:

/// <reference path="aliasOnMergedModuleInterface_0.ts" />

turns to

/// <reference path="aliasOnMergedModuleInterface_0.d.ts" />

Test that show this behavior

aliasOnMergedModuleInterface, aliasUsedAsNameValue, ambientExternalModuleWithInternalImportDeclaration, ambientExternalModuleWithoutInternalImportDeclaration, arrayOfExportedClass, commentOnAmbientClass1, commentOnAmbientEnum, commentOnAmbientModule, commentOnAmbientVariable2, commentOnAmbientfunction, commentOnElidedModule1, commentOnInterface1, commentOnSignature1, constDeclarations-access5, crashInResolveInterface, declFileAmbientExternalModuleWithSingleExportedModule, declFileForExportedImport, doNotEmitTripleSlashCommentsInEmptyFile, doNotEmitTripleSlashCommentsOnNotEmittedNode, doNotemitTripleSlashComments, duplicateIdentifierRelatedSpans6, duplicateIdentifierRelatedSpans7, emitMemberAccessExpression, emitTopOfFileTripleSlashCommentOnNotEmittedNodeIfRemoveCommentsIsFalse, enumFromExternalModule, exportAssignClassAndModule, exportAssignedTypeAsTypeAnnotation, exportAssignmentOfDeclaredExternalModule, exportAssignmentOfGenericType1, exportEqualCallable, exportEqualErrorType, exportEqualMemberMissing, externalModuleAssignToVar, externalModuleRefernceResolutionOrderInImportDeclaration, fileReferencesWithNoExtensions, genericWithCallSignatures1, importAliasFromNamespace, importDecl, importDeclarationUsedAsTypeQuery, importUsedInExtendsList1, import_unneeded-require-when-referenecing-aliased-type-throug-array, instanceOfInExternalModules, jsFileCompilationErrorOnDeclarationsWithJsFileReferenceWithNoOut, jsFileCompilationErrorOnDeclarationsWithJsFileReferenceWithOut, jsFileCompilationErrorOnDeclarationsWithJsFileReferenceWithOutDir, jsFileCompilationNoErrorWithoutDeclarationsWithJsFileReferenceWithNoOut, jsFileCompilationNoErrorWithoutDeclarationsWithJsFileReferenceWithOut, localAliasExportAssignment, memberAccessMustUseModuleInstances, mergedInterfaceFromMultipleFiles1, missingImportAfterModuleImport, moduleAliasAsFunctionArgument, moduleAugmentationDuringSyntheticDefaultCheck, moduleInTypePosition1, moduleSymbolMerging, nodeResolution4, outModuleTripleSlashRefs, privacyCannotNameAccessorDeclFile, privacyCannotNameVarTypeDeclFile, privacyFunctionCannotNameParameterTypeDeclFile, privacyFunctionCannotNameReturnTypeDeclFile, privacyTopLevelAmbientExternalModuleImportWithExport, privacyTopLevelAmbientExternalModuleImportWithoutExport, propertyIdentityWithPrivacyMismatch, protoAsIndexInIndexExpression, requireEmitSemicolon, reuseInnerModuleMember, selfReferencingFile, selfReferencingFile3, sourceMapWithMultipleFilesWithCopyright, staticInstanceResolution3, tripleSlashReferenceAbsoluteWindowsPath, typeofAmbientExternalModules, underscoreTest1, voidAsNonAmbiguousReturnType, withImportDecl, ambientDeclarationsExternal, topLevelAmbientModule, topLevelModuleDeclarationAndFile

Ensure path is relative to directory of current file

All paths should be normalized (ie use / as a directory separator) and they should be expressed as paths relative to the current directory (no ./ required before files in the current directory, absolute paths should be converted)

Examples:

// from:  duplicateIdentifierRelatedSpans6
/// <reference path="./file1" />
// becomes 
/// <reference path="file1.d.ts" />

// from conditionalTypeVarianceBigArrayConstraintsPerformance
/// <reference path="/.lib/react16.d.ts" />
// becomes 
/// <reference path="../.lib/react16.d.ts" />

Test that show this behavior

ambientExportDefaultErrors, conditionalTypeVarianceBigArrayConstraintsPerformance, duplicateIdentifierRelatedSpans6, duplicateIdentifierRelatedSpans7, errorInfoForRelatedIndexTypesNoConstraintElaboration, fileReferencesWithNoExtensions, importAliasFromNamespace, maxNodeModuleJsDepthDefaultsToZero, moduleAugmentationDuringSyntheticDefaultCheck, moduleNodeImportRequireEmit, moduleNodeImportRequireEmit, moduleNodeImportRequireEmit, moduleNodeImportRequireEmit, outModuleTripleSlashRefs, selfReferencingFile3, styledComponentsInstantiaionLimitNotReached, tripleSlashReferenceAbsoluteWindowsPath, typeReferenceDirectives4, typeReferenceDirectives6, untypedModuleImport_vsAmbient

DX Improvement Opportunities

This is a non-exhaustive list of further improvements we could make to the current implementation to require fewer type annotations. These enhancements would ease the adoption of the feature by existing codebases and reduce the verbosity of the source code.

D1. Allow Object and Array literals in const assertions in declarations

Currently the following is an error under isolated declarations:

export const x = {
  value: 0,
  array: [1, 2, 3],
} as const;

While allowing any arbitrary expression in the initializer would be confusing, we can extend the current ability to preserve primitive literals in initializers to also preserve object and array literals if they are subject to a const assertion. So the above code should generate the following declaration:

export declare const x = {
  value: 0,
  array: [1, 2, 3],
} as const;

Also allow function signatures in the initializer

We can also consider expanding the above syntax to allow method signatures in object literals. This would be a minimal transform. This is however an addition to syntax that looks like JS syntax, although I would argue once you put the declare word in front of the variable, we are in the TS grammar scope not the JS one.

With this proposal the following object literal will be present in declarations:

// index.ts
export const x2 = {
  value: 0,
  array: [1, 2, 3],
  fn(value: string): number { return 0 },
} as const;


// index.d.ts
export declare const x = {
  value: 0,
  array: [1, 2, 3],
  fn(value: string): number,
} as const;

D2. Allow inference from expression if there is no need to use non-local type info

This is an alternative to D1. Instead of expanding what we allow in initializers we could allow inference from expressions if the inference only uses type information in the current expression. This restriction should simplify the work a declaration emitter would need to do to a small subset of TypeScript inference that can safely be re-implemented across several tools

This would mean that expressions that use object literals, array literals, and primitive literals can have their types inferred. As well as allowing inference of any methods/ functions in the object without requiring new syntax (as D1 does).

Examples of code that would be inferred:

// foo: number
let foo = 1;


// bar: stirng
let bar = "";


// o: { value: number, array: number[] }
let o = { value: 1, array: [1, 2, 3] }


// arr: ({ foo: number; bar?: undefined; } | { bar: string; foo?: undefined;})[]
let arr = [{ foo: 1 }, { bar: "" }]


// oConst: { readonly value: 1; readonly array: readonly [1, 2, 3]; }
let oConst = { value: 1, array: [1, 2, 3] } as const;


// readonly [{ readonly foo: 1; }, { readonly bar: ""; }]
let arrConst = [{ foo: 1 }, { bar: "" }] as const;


// oWithMethod: { value: number; method(s: string): string; }
let oWithMethod = {
    value: 1,
    method(s: string): string { return s.toLocaleLowerCase() }
}
// sym: unique symbol; 
const sym = Symbol()

Examples of code that would not be inferred:

// depends on the type of a global
let v0 = Promise.resolve(0)


// depends on the type of a global constructor
let v1 = new AbortController();


import { foo } from './foo'
// depends on type of import
let v2 = foo;


// depends on non local information (we could consider typeof v1 see next section)
let v3 = v1;

Use typeof operator

We could use the typeof type operator to infer types for assignments from other symbols accessible in the file:

const v0 = Symbol()

// publicValue: typeof v0
export const publicValue = v0


import { foo } from './foo'


// bar: typeof foo
export const bar = foo
export class Foo {
    // field: typeof foo
    readonly field = foo
}

The above code uses const and readonly fields to demonstrate this behavior. This is because let and mutable fields will widen the type of the assigned variable, so using typeof without some sort of intrinsic Widen type would not replicate TS behavior:

// v0: 1
const v0 = 1

// ts: v1: number
// could be: v1: Widen<typeof v1>
export let v1 = v0

For the example above TypeScript infers for v1 the type number because the type is widened. If we add an intrinsic Widen type we could use the typeof type operator to type v1 relative to v0 using Widen<typeof v1>. The Widen type would replicate any type widening behavior TypeScript has when assigning a value of the specified type to a mutable variable.

The suggestion to use typeof has the unwanted consequence that the declarations might expose more implementation details than it currently does, but the DX improvements might be worth it.

D1 vs D2

The two improvement proposals are competing to solve the same issues and only one should be selected.

D1 allows declaration emitters to remain simpler, at the cost of DX (more type annotations are needed) and more changes to current declaration emit are needed to support some scenarios.

D2 allows more scenarios and requires less changes to current declaration emit, at the cost of putting more complexity in declaration emitters. There is also the risk of TypeScript changing rules with regard to variable inference. This happens infrequently but when it does it will require declarations emitter to add support for any such changes.

D3. Allow Basic Arithmetic Operators in Enum Expressions

As described above any non primitive literal expression is an error under isolated declarations. To alleviate this problem we can allow expressions that contain arithmetic operators, with primitive and dotted identifier operands.
So for example these would be legal with this proposal:

function nr() { return 1;}
enum E {
    A = 1 + 1, // ok
    B = nr(), // error
    C = E.A + 2, // ok
    D = ~A // ok
}

D4. Extract function type from function expression assignment.

Currently this would be an error under isolated declarations:

const fn = (value: string) : number => {
    return value.length;
}

This is because the fn variable does not have an explicit type annotation.

Synthesizing a type from the assignment is however trivial, as all the information is already present in the arrow function signature (both parameter and return type are specified)

This could apply equally to arrow functions and function expressions.

This could also apply to class fields that are initialized with a function.

D5. Destructured elements - Do not flatten binding elements

As detailed above the following code would currently be an error:

type T = [{ foo: string }]
export const [{foo}]: T = null!

An improvement would be to keep the binding element ‘as is’ in the declaration file:

type T = [{ foo: string }]
export declare const [{foo}]: T;

Allow initialisers

A problem might occur if we also have initializers as the type of the initializer can contribute to the type of the variable:

// index.ts
type T = [{ foo: string | undefined }]
export const [{foo = ""}]: T = null!;


// index.d.ts
export declare const foo: string;

Similarly with the way we allow const declarations to keep their initializers we can allow initializers to be preserved in destructuring for const declarations. The above code would create the following declaration:

// index.d.ts
type T = [{ foo: string | undefined }]
export declare const [{foo = ""}]: T;

D6. Default Exports

Currently there is no way to be explicit about the type of a default export expression. Adding the ability to specify a type annotation to a default export would help the DX in isolated declarations. The proposal to add this syntax is tracked here,

D7. Extract type from type assertions

Currently type assertions are not considered during declaration emit, so the following code would be an error under isolated declarations:

let x = 1 as number

We could take the type in the assertion and use it as the type of the variable.

This approach could also be used for expando functions:

function myFunction() {
  return 0;
}
myFunction.foo = "" as string;

The problem with this approach is that it encourages over use of type assertions, which are not safe. We could encourage the use of expr satisfies T as T which seems to provide as close to a type annotation as possible by both forcing the expression to have a specific type and checking that the expression satisfies the given type. Ex:

interface Foo { n: string }


function component() {}


component.foo = { n: "" } satisfies Foo as Foo

A similar approach could be use for default exports even in the absence of explicit syntax to annotate the default export:

type Union = { v: 1 | 2 }
export default { v: 1 } satisfies Union as Union;

D8. Mark internal APIs

We could also allow users to opt into ignoring isolated declaration errors on a case by case basis, either using @internal or @ts-expect-error/ @ts-ignore. This would allow users to mark APIs that aren’t actually exposed to the outside world (because they are not reachable reachable through public end points)

When generating the declaration unknown can be used as the type for any missing annotation

// index.ts
// @internal
export function test () {
    return 1;
}
// index.d.ts
// @internal
export declare function test (): unknown

Open issues

These open questions require a decision to be made before the solution can be considered complete.

O1. Computed property names

TypeScript currently uses the type of the expression in a computed property name to tell if two members belong to the same symbol. This does rely on type information being present and so can’t be duplicated in isolatedDeclarations mode. This problem is that for method overloads it is difficult to identify what constitutes a method group.

Example of the problem

const A = "A";
const B = "A";

export class Foo {
    [A](): void
    [B](): void
    [A](): void {


    }
}

Currently the external isolated declaration relies on using the same identifier (or dotted identifier chain) for the grouping of methods.

A proposed solution is that we could add a restriction that overloaded methods must use the same identifier in the computed property name expression.

O2. No transformation of type reference directives

O2.a. Adding/removing directives

TypeScript adds/removes type reference directives. This happens based on the usage of symbols in the declaration file. This is an analysis an external declaration emitter can’t do. It requires loading all library files in order to have access to the symbols, which is antithetical to the “allow per file declaration emit” design goal.

Example of references being removed: typeReferenceDirectives10

// @declaration: true
// @typeRoots: /types
// @traceResolution: true
// @currentDirectory: /


// @filename: /ref.d.ts
export interface $ { x }


// @filename: /types/lib/index.d.ts
declare let $: { x: number }


// @filename: /app.ts
/// <reference types="lib"/>
import {$} from "./ref";
export interface A {
    x: $
}

Declaration emit for app.ts

// missing directive here
import { $ } from "./ref";
export interface A {
    x: $;
}

Some other tests that are impacted by this behavior

declarationFilesWithTypeReferences4, moduleResolutionWithSymlinks_preserveSymlinks, moduleResolutionWithSymlinks_referenceTypes, tripleSlashTypesReferenceWithMissingExports, typeReferenceDirectives10, typeReferenceDirectives13, typeReferenceDirectives3, typeReferenceDirectives4, typeReferenceDirectives5, typeReferenceDirectives7, typeReferenceRelatedFiles, nodeModulesTripleSlashReferenceModeDeclarationEmit6, nodeModulesTripleSlashReferenceModeDeclarationEmit7, nodeModulesTripleSlashReferenceModeOverride1, nodeModulesTripleSlashReferenceModeOverride2, nodeModulesTripleSlashReferenceModeOverride3, nodeModulesTripleSlashReferenceModeOverride4, nodeModulesTripleSlashReferenceModeOverride5, nodeModulesTripleSlashReferenceModeOverrideModeError, nodeModulesTripleSlashReferenceModeOverrideOldResolutionError, library-reference-*, library-reference-scoped-packages, typingsLookup1, typingsLookup3

Example of references being added: typeReferenceDirectives11

// @typeRoots: /types
// @types: lib


// @filename: /types/lib/index.d.ts
interface Lib { x }


// @filename: /mod1.ts
export function foo(): Lib { return {x: 1} }

Declaration emit for mod1.ts

// /mod1.d.ts
/// <reference types="lib" />
export declare function foo(): Lib;

Some other tests that are impacted by this behavior

declarationEmitHasTypesRefOnNamespaceUse, declarationFilesWithTypeReferences2, typeReferenceDirectives11, typeReferenceDirectives2, typeReferenceDirectives8

Conclusion: Adding the reference depends on knowing about global types defined in other files, while removing them is based on those library types not being used in the file. Both of these require information from other files which we can’t access in isolated declaration mode.

Options: Here are some options we have

1. Silently remove all such directives and assume the types in the client will work out. If the client needs some types they should include them
2. Do not add any directives and error in TS when they are needed. Keep all existing directives.

Option 1 would make it difficult for clients to know what libs are required by the types. Option 2 would be more compatible, but can potentially require declarations that are only needed for type checking but are not need for declarations, there is simply not enough information to perform this analysis in isolated declaration.

Note: After discussing with Daniel Rosenwasser, Option 1 seems preferable, but we need to explore what impact this has on projects that consume such declarations.

O2.b. resolution-mode in directives

Some type directives have their resolution mode changed during printing (either changed or removed). This is not a problem in itself but if we include type directives, this is something that is not obvious from the reference declaration emitter.

TS printer code:

const resolutionMode = directive.resolutionMode && directive.resolutionMode !== currentSourceFile?.impliedNodeFormat
    ? `resolution-mode="${directive.resolutionMode === ModuleKind.ESNext ? "import" : "require"}"`
    : "";

Some other tests that are impacted by this behavior

nodeModulesTripleSlashReferenceModeDeclarationEmit1, nodeModulesTripleSlashReferenceModeDeclarationEmit2, nodeModulesTripleSlashReferenceModeDeclarationEmit5

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[typescript-bot]

The TypeScript team hasn't accepted the linked issue #47947. If you can get it accepted, this PR will have a better chance of being reviewed.

[Jack-Works]

export class Test {
    x // error under isolated declarations
    private y = 0; // no error, private field types are not serialized in declarations
    #z = 1;// no error, fields is not present in declarations
    constructor(x: number) {
        this.x = 1;
    }
    get a() { // error under isolated declarations
        return 1;
    }
    set a(value) { // error under isolated declarations
        this.x = 1;
    }
}

you cannot emit private or #private fields, but emitting them is easy. they're emitted as any;

this is what ts emit today:

export declare class Test {
    #private;
    x: number;
    private y;
    constructor(x: number);
    get a(): number;
    set a(value: number);
}

the private/#private declaration is used to make those classes become a nominal typed class.

[DanielRosenwasser]

@typescript-bot pack this

[typescript-bot]

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at ed568e2. You can monitor the build here.

[fatcerberus]

Wow that is one massive wall of text in the OP

[frigus02]

This is super exciting.

I tested --isolatedDeclarations on Google’s code base (based on ed568e2). About 36% of our libraries failed to compile with at least one TS9007 error. That looks scary. On the flip side, this means 64% of our libraries are already sufficiently annotated, which is nice.

I tried to get an idea about the impact of the DX Improvement Opportunities. Sadly, those 36% are too many targets to triage manually.

I expect that D1 and D2 could have the largest impact. D3 - D8 feel slightly more niche, though I don’t have data to back that up. I did some crude regex searching on the error diagnostics and found that:

• Inference for primitive types (boolean, number, string) would remove 9.9% of the errors.

  Patterns searched for:

  foo = true|false
  foo = [0-9]
  foo = ["'`]
  

• Use of typeof operator would remove 3.1% of the errors.

  Patterns searched for:

  const foo = bar;
  readonly foo = bar;
  

This suggests that D2 could remove >=13% of the errors, especially if it also accounted for arrays and objects.

Separately, I noticed that ~4% of the errors used override. That makes me wonder if overridden members could elide the type annotation or something along those lines.

Another data point: Last year we asked TypeScript developers at Google how they feel about adding explicitly type annotations on exported symbols. Of ~160 responses, 40% liked it, 40% disliked it and 20% didn’t care. From comments we gathered that the dislike often stemmed from having to write what feels like trivial type annotations. D1 or D2 could hopefully address those concerns.

Again, this is really exciting. Thank you for working on this!

[mihailik]

Declaration emit should be a syntactic transformation that adds type information present in the current statement only. (i.e. It does not use information from outside the file, such as types of globals or imports).

What happens to DOM declarations? Or are you excusing all lib.d.ts from this rule?

[mihailik]

It feels this compiler flag is targeting pretty narrow corner case.

And if it's enabled unintentionally creates confused set of syntactic rules that would produce a deluge of inexplicable compiler errors.

Can the behavior be achieved by wrapping and invoking TS APIs instead of introducing new mode into the compiler itself?

[dragomirtitian]

@mihailik

Declaration emit should be a syntactic transformation that adds type information present in the current statement only. (i.e. It does not use information from outside the file, such as types of globals or imports).

What happens to DOM declarations? Or are you excusing all lib.d.ts from this rule?

I'm not sure what you mean. Declaration files do not need to be emitted again (and they are fully typed anyway). Symbols defined in the lib files will be subject to the same rules. For example let a = Math.random() will now require an annotation on a (let a:number = Math.random();)

It feels this compiler flag is targeting pretty narrow corner case.

This change is targeted for monorepos. There are several monorepo tools that could take advantage of this change today. Also in the future typescript composite projects could take advantage of this. So there are users out there that can benefit from this, albeit in large multi project workspaces. Improving the workflow in these scenarios is not without merit IMO.

And if it's enabled unintentionally creates confused set of syntactic rules that would produce a deluge of inexplicable compiler errors.

I don't find this a compelling reason. If you enabled it after the code is written and you get a deluge of errors, you will quickly find the last ts config change, or you will be able to search for the errors and quickly find that they are related to this flag. (Also I don't think it is very common for people to accidentally enable compiler flags)

Can the behavior be achieved by wrapping and invoking TS APIs instead of introducing new mode into the compiler itself?

The idea is to enable other tools to produce declaration files. If we bring the type checker into it (ie use the compiler API) the external tools will be as slow as TS since you need to type check (alt least in part) before you can synthesize types. If we proceeded without the type checker some changes need to be made to how code is written as to not require types that can't easily be created without full type info. Without a common understanding of what those changes are it is difficult for other tools to make investments in this space. It is also difficult for users to adopt such tools as their restrictions might be different (and so swapping them out is difficult, leading to concerns about if a project is abandoned are we stuck with an unsupported tool)

[bradzacher]

D2. Allow inference from expression if there is no need to use non-local type info

I think it's okay to infer the types of simple literals (number, string, boolean, null, undefined).

However I don't think you really want to be inferring types for all object cases or any array cases.

Objects

Performance

(From my understanding) Most object cases would produce better TS perf/memory use to use a named type as opposed to an anonymous one.

Error Messages

Named types also produce better error messages.

For example (I know this an array base but it serves as a good example of error message improvements):

let arr = [{ foo: 1 }, { bar: "" }];
arr.push({ baz: true });
// Argument of type '{ baz: boolean; }' is not assignable to parameter of type '{ foo: number; bar?: undefined; } | { bar: string; foo?: undefined; }'.
//  Object literal may only specify known properties, and 'baz' does not exist in type '{ foo: number; bar?: undefined; } | { bar: string; foo?: undefined; }'.

type T = { foo: number; bar?: undefined; } | { bar: string; foo?: undefined;};
let arr2: T[] = arr;
arr2.push({ baz: true });
// Argument of type '{ baz: boolean; }' is not assignable to parameter of type 'T'.
//  Object literal may only specify known properties, and 'baz' does not exist in type 'T'.

Ambiguous / untyped properties

Unlike exported variables with simple values, non-readonly exported objects are inherently mutable from other modules.

The simple literal cases are fine to infer (literal types become their parent type) and functions can be enforced to be typed as if they were directly exported (eg must have parameter types and a return type), but what should be inferred when the value is something like null or undefined?

If you infer the literal type then you create a scenario where a downstream consumer may be unable to use the object as intended.

export const x = { prop: null };

x.prop = 1; // Type '1' is not assignable to type 'null'

One could definitely argue that this is fine because that's how TS currently works - though I would personally prefer that this option forces you to be less ambiguous here and either as const the literal to say "Yes, I really mean this to be null/undefined" or add a wider type manually.

There's ofc the array case as well (see below).

Arrays

Arrays are much the same as above, though things can be more ambiguous, IMO.

In OP you describe this case:

let arr = [{ foo: 1 }, { bar: "" }]

Your example types this code as this:

let arr: ({ foo: number; bar?: undefined; } | { bar: string; foo?: undefined;})[];

Which is how TS infers the type.

However one could also argue that the type could instead be the stricter and safer

let arr: ({ foo: number } | { bar: string })[];

The difference being that the former allows arr.push({ foo: 1, bar: undefined }), whereas the latter does not - which might be an important distinction, for example if downstream code wants to do a check like 'bar' in value to refine the type.

One could also argue that it was intended to be the wider type

let arr: ({ foo?: number; bar?: string })[];

There's a lot of ambiguity here and allowing inference for these cases would mean that tools would ultimately just seek to align with what TS infers (which is a complex thing to replicate exactly).

I would personally propose that array literals are treated the same as any function/constructor call - which is to say they need to be explicitly annotated.

[bradzacher]

Use typeof operator

Personally I don't think using typeof is a great idea because it creates a layer of indirection that TS has to resolve at typecheck time when constructing the module signature.

IMO it'd be better to skip the middle-man and enforce an explicit type annotation.

I think if the user wants to explicitly use typeof - that's another story (one that's necessary in some cases, like symbols) - but I don't think it's a win to permit/expect it implicitly.

It also complicates the translation algorithm somewhat because you would need to use scope analysis to follow the declaration of the variable to determine if it's correctly typed.

For example imagine:

let a = Math.min(1, 1);
export const b = a;

A transpiler can't just do export const b: typeof a; because a's type is not specified.

Additionally this would result in excess code being required in many cases. For example if the above example was correctly annotated you would generate:

declare let a: number;
export const b: typeof a;

instead of the more concise and simpler:

export const b: number;

One could argue that excess code is fine and a minification step could be implemented to reduce cruft - but I personally think you'd just want to be strict and simple by default.

[bradzacher]

O1. Computed property names
Currently the external isolated declaration relies on using the same identifier (or dotted identifier chain) for the grouping of methods.

I don't think it's safe to do this as it would allow declarations that would be errors:

declare const A: string;

export declare class Foo {
    [A](): void;
    // A computed property name in a method overload must refer to an expression whose type is a literal type or a 'unique symbol' type.
    [A](): string;
    // A computed property name in a method overload must refer to an expression whose type is a literal type or a 'unique symbol' type.
}

playground

This would also be impossible to handle in isolation if, for example, A is an imported name.

---

There's also a weird case with computed name methods in that currently TS will not error, but it will omit the property from its declaration entirely:

declare const A: string;

export class Foo {
    [A](): number {
        return 1;
    }
}
export class Bar {
    [A as "A"](): number {
        return 1;
    }
}

declare const B: 'B';
export class Baz {
    [B](): number {
        return 1;
    }
}

/////// Current .d.ts output ///////

export declare class Foo {
}
export declare class Bar {
}
declare const B: 'B';
export declare class Baz {
    [B](): number;
}
export {};

playground

IDK if this is a bug in TS or not considering that if the member is instead a property it will explicitly error.

[sandersn]

I read through the comment history here and have read through half of emitBinder.ts. So far the code seems intended as a separate, smaller, faster re-implementation of everything but the parser. If so, why is this inside tsc? The original post mentions plans to split this into something separate. Is that still planned?

Putting a new implementation of Typescript next to the existing one is both slow and confusing for future development.

[dragomirtitian]

@sandersn We discussed this and the reasons we decided to keep this in TS were:

1. For testing. transpileDeclaration is proof that a non type checker implementation of declaration emit is possible and that TS will not add any features that will prevent this (without putting them behind the isolatedDeclarations flag)
2. Allowing other tools to use the fast API for declaration emit
3. Offering a refence implementation others can use to implement their own declaration emit, as well as the.

The whole implementation is about 1.5K likes with the binder being the biggest part of it at about 900 lines, so it's not a huge amount of extra code.

[sandersn]

Some comments about emitBinder.ts, only about half way through with it so far.

[sandersn]

is this the way the binder does it?
later: no, it's defined in code, by calling declareSymbol with particular arguments. So I guess this way is more organised.

[sandersn]

what is DTE? What does it stand for?
edit: @jakebailey pointed me to his earlier question. The term DTE needs to be changed to something simpler and more meaningful.

In "DTE needs to detect", is DTE a separate executable? A separate compilation code path invoked by a flag? Or just a separate emit code path?

[robpalme]

The term DTE was defined as a ".d.ts-emitter" by @RyanCavanaugh in this post from March 2023 and has been used in the context of this project semi-frequently since then.

It refers to a tool that can take exactly one .ts file and emit exactly one .d.ts file as a simple syntactic transform. It is expected that all future DTEs will match the reference implementation in TypeScript.

[sandersn]

patching id onto node is probably going to be slow. Could nodeLinks look up some other way, maybe by object identity?

[dragomirtitian]

The as any is left over from when this code was outside TSC. TSC already uses id in getNodeId. This just gets the node links if they exist, as opposed to getNodeLinks which will create them if they don't exist. This is modeled on getNodeLinks in checker.

[sandersn]

this is a very weird type

edit: and a pretty suspicious function -

• when is it passed string, which is unlike the other types?
• when is it passed undefined; would it possible to make sure its inputs are always defined?
• would it be possible to re-use existing functions and add the prefixes on in a smaller function with fewer cases?

[sandersn]

note this code is actually based on checker, not binder

[dragomirtitian]

That is true. The resolve* fuctions would probbalymake more sense to be moved in the resolver but there are resolveName is used during binding so it's a bit hard to extarct without a circular dependency.

[sandersn]

why does this assign to node instead of the node's links?

[dragomirtitian]

node.symbol is accessed in a lot of places in declaration emit. We fill declarations and valueDeclaration to allow everything to work. I can try to see if we can remove the need for those, but it will probably mean adding more members to the emit resolver.

[sandersn]

anonymous symbols don't get added to the table? why not?

[sandersn]

there's a lot of repeated code here and in createAnonymousEmitSymbol/getSymbol. Comparing those with this raises quite a few questions. Probably it would be better to de-dupe the code, making it all behave the same way.

[sandersn]

The binder issues an errors in this case and merges anyway. How does creating a new symbol affect emit?

[dragomirtitian]

I don't think it does always merge. From what I understand here, the binder will create a new symbol if symbol.flags & excludes. I did spend quite a bit of time trying to emulate the same emit behavior, but I'm ultimately not sure how much this matters anyway since these will be error cases and emulating teh same behavior in an error is not crucial

[sandersn]

looks like @ is a name-mangling prefix

[jakebailey]

isolated is not the right distinction though, since when running the original tests both outputs are with isolated declarations on (to make sure we detect the same error cases). It's with-type-checker vs no-type-checker.

I feel like I'm misunderstanding something, then. I thought the diffs were specifically "here's the dts emitted without isolatedDeclarations, here's with isolatedDeclarations, and here's the diff. If there's no diff, then there was no change and we are good to go".

[andrewbranch]

I thought I heard you think out loud about the naming of isolatedModules and how it could be improved. Any ideas on isolated declarations?

I have more of a problem with “modules” than “isolated” in isolatedModules, so I’m not really against isolatedDeclarations, especially not at this point in the life of this PR, much less the life of isolatedModules.

If I were starting from scratch, I’d probably make isolatedModules behavior always-on and not have a flag for it. Barring that, I might call them something like ensureParallelizableEmit and ensureParallelizableDeclarationEmit, except that “parallelizable” is so hard to say 🙃

[andrewbranch]

I'm sort of struggling to understand the changes to declaration emit (declarations.ts) that occur when --isolatedModules is on but context.kind === .FullContext, which IIUC is what happens with tsc --isolatedModules and is distinct from running e.g. ts.transpileDeclaration. The model with --isolatedModules is that it doesn't change emit; it just errors when it detects that someone without full context wouldn't be able to emit. Here, it seems that simply enabling --isolatedModules does change emit, but it's potentially not equivalent to the emit you'd get with ts.transpileDeclaration?

[andrewbranch]

Got through a good bit of the code, but stopped reviewing in order to get myself up to speed on the fairly fundamental question of “why does --isolatedDeclarations actually affect emit, not just issue diagnostics”

[andrewbranch]

Suggested change

	"Ensure that each file can have declaration emit generated without type information": {
	"Ensure that each file can have declaration emit generated without reading other files": {

?

[dragomirtitian]

Both of these are true. it does allow one file at a time declaration emit, but it also ensures that not types need to be created by the type checker. I feel the latter constraint is the stricter one since it forbids most inference even if the information is available in the file.

[andrewbranch]

I think what’s going on in localInferenceResolver.ts is “type information,” but I take your point that needing to annotate function return types is probably more visible to users than the lack of cross-file information.

[andrewbranch]

Nit:

Suggested change

	"Function must have an explicit return type annotation with --isolatedDeclarations.": {
	"Function must have an explicit return type annotation with '--isolatedDeclarations'.": {

is the typical style. My ear also slightly prefers in '--isolatedDeclarations' over with, while the wordier when '--isolatedDeclarations' is enabled is more established... I don’t have a very strong opinion here.

[andrewbranch]

Nit: “cannot” is typical

[andrewbranch]

Nit: transpileDeclaration sounds to me like it takes a declaration file and transpiles it to... something else? (I always conceptualized ts.transpileModule as taking a module, although it also happens to return one.) I don’t immediately have a suggestion for a better name, though 🤔

On that note, what happens when the sourceFile you provide is a declaration file?

Also, this being the most important API addition, I think it deserves some JSDoc description.

[andrewbranch]

Silly idea: I was thinking you could statically validate this list against keyof Function as it exists during the compiler’s own compilation. However, trying that out shows that this list omits arguments, caller, and name. Is that intentional?

[andrewbranch]

I’m not quite following this. This function doesn't just get called for literal triple slash references in the input file; it also gets called when you write an import that resolves to an ambient module. And returning early here changes the emit in a potentially dangerous way, without an accompanying isolatedDeclarations error. For example, if I have @types/node installed and a source file like

export { readFile } from "fs";

This is legal according to the PR as it stands, but the emit under --isolatedModules loses the triple-slash reference that makes "fs" resolvable:

- /// <reference types="node" />
export { readFile } from "fs";

Based on the analog to isolatedModules, my expectation for how this flag would work was that

1. Enabling the flag wouldn’t change emit with tsc --declaration
2. Enabling the flag would leverage whole-program info to issue diagnostics in cases like the above, where information from other files (or too-complex type inference) was necessary to produce the correct emit.

[dragomirtitian]

You are right, I will change the code to make TS emit equivalent.

Just to understand the implications of not emitting these reference directives in transpileDeclaration:

Without these ts will not be able to resolve import of modules that are declared in the declarations of other modules. Such as node having a declare mdoule 'fs' { ... }. (corect e on the terminology here 😅)

For node this is not actually that big an issue from what I can tell since getAutomaticTypeDirectiveNames will automatically add all packages in @types, and node types are defined there.

For types that come from regular imports that resolve to a package in node_modules that contains the appropriate declarations, this again is not an issue.

The only way the absence of these directives becomes a problem is if the types for a module are not in the usual resolution path, and if the module is not in the typeRoots.

[andrewbranch]

I’ve skimmed through most of the remaining changes to declarations.ts, and there are many cases of

if (isolatedDeclarations) {
  // return something different
}

Were these all intended to be if (context.kind === TransformationContextKind.Isolated)?

[andrewbranch]

Also, just to be clear, I don’t think that matching emit here is sufficient; I do think isolatedDeclarations needs to issue an error. I used @types/node as a simple example, and you’re right that in practice it’s very unlikely for the end user not to already have @types/node loaded due to other references. But that’s totally circumstantial. We emit those triple slash references for a reason; we can’t just say they’re universally unnecessary and silently elide them.

[dragomirtitian]

Also, just to be clear, I don’t think that matching emit here is sufficient; I do think isolatedDeclarations needs to issue an error. I used @types/node as a simple example, and you’re right that in practice it’s very unlikely for the end user not to already have @types/node loaded due to other references. But that’s totally circumstantial. We emit those triple slash references for a reason; we can’t just say they’re universally unnecessary and silently elide them.

We can definitely error on it, the problem is I don't know what the workaround would be.

We struggled previously with reference directives. TS will not just add them, but will also remove them if they are not needed for the declaration file. This is the kind of analysis we can't do in isolated declaration emit. We discussed and decided with @DanielRosenwasser that banning all reference directives in isolated declaration mode would be the way to go.

But due to this ban, it means that a user would be stuck. You can't export * from 'fs' without a reference directive, but adding it would be an error because we don't know if we need to keep it in the declaration or not.

I’ve skimmed through most of the remaining changes to declarations.ts, and there are many cases of

if (isolatedDeclarations) {
  // return something different
}

Were these all intended to be if (context.kind === TransformationContextKind.Isolated)?

I will check them again tomorrow, but the if (isolatedDeclarations) should be in cases where the if triggers an error, or if an error would have been triggered already. In such cases declaration emit should not happen anyway.

[andrewbranch]

TS will not just add them, but will also remove them if they are not needed for the declaration file.

I’m not sure I was aware of this. Sorry to jump into a conversation you’ve already had, but did we consider changing normal declaration emit to preserve all manually written reference directives?

[dragomirtitian]

@andrewbranch This example shows the removal of the reference directive.

//index.ts
/// <reference types="node" />
export function x() { }
//index.d.ts
export declare function x(): void;

I am not sure about keeping them in either. Wouldn't that keep implementation details in the declaration file? I am fine with always keeping them. I don't think people are adding these anymore for other reasons.

[andrewbranch]

I think it’s no different from keeping import "./foo" statements in declaration emit. The user wrote it on purpose and it changes the shape of their program; it feels odd to me that we would elide it.

[dragomirtitian]

I will change it to always preserve reference directives with --isolatedDeclarations

[dragomirtitian]

Got through a good bit of the code, but stopped reviewing in order to get myself up to speed on the fairly fundamental question of “why does --isolatedDeclarations actually affect emit, not just issue diagnostics”

The simple answer is that it does not. Running the command line tsc with --isolatedDeclarations should result in the same emit as before.

The slightly more complicated answer is that if you use transpileDeclaration then you will get the types that come from the limited for of inference implemented in local inference. To transpileDeclaration without duplicating the whole declaration transform, I did make changes to the declaration transform to be aware if it's being called with the full type checker or from transpileDeclaration (where the type checker is not available). In this case, there might be some small changes in emit behavior, which are documented in the diffs.

[sandersn]

More comments on emitBinder

[sandersn]

Is it safe to cast to ArrayBindingElement, ignoring [Object]BindingElement?

Suggested change

	(pattern.elements as NodeArray<ArrayBindingElement>).forEach(b => {
	for (const b of pattern.elements as NodeArray<ArrayBindingElement>) {

[sandersn]

these nested functions are not great for performance, since bindVariable will be called a whole lot.

[sandersn]

Suggested change

	statements.forEach(statement => {
	if (!filter(statement)) return;
	bindNode(statement);
	});
	for (const statement of statements) {
	if (filter(statement)) bindNode(statement);
	};

(also this is simple enough I'd be very tempted to inline it.)

[sandersn]

invert this and early-return

[sandersn]

instead of creating a computed property name wrapper for argumentExpression, make getMemberKey handle directly whatever values it might have

Based on my reading of the code (which could be wrong), I'd recommend extracting the computed property part of getMemberKey into a separate function and then calling it directly from here, or calling a small wrapper of it. If this is the only place that getMemberKey is called with a computed property name, then I think the code might even be simplified further.

[sandersn]

same--invert and early-return

[sandersn]

Does typescript do constant folding like this? I don't remember it but I could easily have missed it being added.

[dragomirtitian]

This is similar to what getPropertyNameForPropertyNameNode does. I will change over to use __String and the regular way of escaping names.

[sandersn]

Would you mind looking for an existing utility for this? I thought there was one.

[sandersn]

Suggested change

	if (declaration.type || !isVarConst(declaration)) {
	return false;
	}
	if (!(declaration.initializer && isFunctionExpressionOrArrowFunction(declaration.initializer))) {
	return false;
	}
	return true;
	return !declaration.type
	&& isVarConst(declaration)
	&& declaration.initializer
	&& isFunctionExpressionOrArrowFunction(declaration.initializer);

though it might work even better to return a single predicate for the whole function starting with
return isFunctionDeclaration(declaration) || ...

[sandersn]

The checker should already handle both functions declarations and function expressions assigned to variables. Either this function was previously incorrect, or its usage is intended as the first of a pair of declaration/assignment checks.

Either way it shouldn't need to start handling variable declarations.

[sandersn]

Suggested change

	// Only identifiers of the for A.B.C can be used
	// Only identifiers of the form A.B.C can be used

[sandersn]

I'm less familiar with computed property handling, but this code must also already exist in the checker in order for it to work. So I expected to see some use of it inside the checker instead of just exporting it.

[sandersn]

bikeshed: HasInferrableType ?

[sandersn]

hmm, I wonder why there are new Core* versions of interfaces. Is it Core=public, original=internal? But the new ones are marked internal too.

[sandersn]

My initial reaction is that the checker should be the one issuing the errors, in order to reduce duplication.

[sandersn]

So, a summary from the design meeting:

From the outside, we see the feature as having four parts:

• errors for things that are not allowed in --isolatedDeclarations
• quickfixes for things that are not allowed in --isolatedDeclarations
• Isolated declaration emit via API
• Isolated declaration emit via tsc

Unfortunately, the part we reached consensus on is that the current implementation is too much of a re-implementation. In my opinion, at least, there's too much to change and review before the end of the week, when it would need to be ready for 5.4 beta.

We were split on several other issues and ran out of time to decide on them:

• Should any of this be in the compiler at all? In theory the errors and fixes could be packaged in a typescript-eslint rule, and the reference emitter could be a tool distinct from tsc. Both could depend on the new binder/checker from this PR.
• Should declaration-only emit be in the compiler? Would it make sense to keep our emit the same and have the fast, limited declaration-only emit as separate tool?
• For other tool authors, how important is it for the reference emitter to be part of tsc? (I did not get enough detail about the problems here to understand it, so maybe somebody else can help.) It seems like the question is: do tool authors need examples of limits, examples of inference rules, or something else?

My opinion

My specific complaint is: things that are illegal under isolatedDeclarations should be implemented as errors in the checker, using existing inference code from the checker. Code that's in the compiler should re-use the existing binder, checker, etc. Otherwise we're signing up to maintain two Typescript compilers at full correctness, etc. If that really is necessary, the second compiler should be clearly separate from the first one.

[dragomirtitian]

@sandersn I think this PR would address some of the concerns around code duplication: bloomberg#138

[dragomirtitian]

As discussed in here we are reevaluating our approach.