Reuse Module Resolutions from Unchanged Files #15261

aozgaa · 2017-04-18T23:33:13Z

This fix is intended to speedup typing in import statements.
Fixes the following VS community issues:
https://developercommunity.visualstudio.com/content/problem/29920/vs-2017-is-hanging-for-a-a-few-seconds-when-i-add.html
https://developercommunity.visualstudio.com/content/problem/3317/vs-typescript-editor-hanging-while-im-typing-the-p.html

… it specified Fixes microsoft#13910

Previously it assumed valueDeclaration was present, which is not true for type aliases.

Previously it crashed when it assumed valueDeclaration was always defined.

Remove unused file

…rst-decl-not-valueDecl mergeSymbol uses first declaration, not valueDeclaration

Use base tsconfig's compileOnSave option if tsconfig.json doesnt have it specified

RyanCavanaugh · 2017-04-19T18:06:50Z

src/compiler/program.ts

+        None = 0,
+        NoOldProgram = 1,
+        ModuleResolutionOptions = 2,
+        RootNames = 3,


1 << 3, etc

…eReuse

billti · 2017-04-19T21:55:56Z

src/compiler/program.ts


            for (const oldSourceFile of oldProgram.getSourceFiles()) {
                let newSourceFile = host.getSourceFileByPath
                    ? host.getSourceFileByPath(oldSourceFile.fileName, oldSourceFile.path, options.target)
                    : host.getSourceFile(oldSourceFile.fileName, options.target);

                if (!newSourceFile) {
-                    return false;
+                    structuralChanges |= StructuralChangesFromOldProgram.SourceFileRemoved;


SourceFileRemoved is in your CannotReuseResolution set, so why collect flags and continue here rather than just return?

billti · 2017-04-19T22:00:02Z

src/compiler/program.ts

-                        return false;
+                    else {
+                        // file has no changes - use it as is
+                        newSourceFile = oldSourceFile;


It's a bit hard to tell in this diff... but is this in the else clause to if (oldSourceFile !== newSourceFile)? If so, this statement seems like a no-op.

mhegazy · 2017-04-21T21:29:27Z

src/compiler/types.ts

+        /* @internal */ structureIsReused?: StructureIsReused;
+    }
+
+    export const enum StructureIsReused {


/* @internal */

mhegazy · 2017-04-21T21:35:17Z

src/compiler/program.ts

-            if (!oldProgramState && !file.ambientModuleNames.length) {
-                // if old program state is not supplied and file does not contain locally defined ambient modules
-                // then the best we can do is fallback to the default logic
+        function createOldProgramState(


just inline that. no need to call a function that just wraps the arguments in a literal.

mhegazy · 2017-04-21T21:43:12Z

src/compiler/program.ts

@@ -6,6 +6,23 @@ namespace ts {
    const emptyArray: any[] = [];
    const ignoreDiagnosticCommentRegEx = /(^\s*$)|(^\s*\/\/\/?\s*(@ts-ignore)?)/;

+
+    const enum StructuralChangesFromOldProgram {
+        None                        = 0,


let's keep this as a tri-state enum

mhegazy · 2017-04-21T22:02:30Z

src/compiler/program.ts

                    // ensure that module resolution results are still correct
                    const resolutionsChanged = hasChangesInResolutions(moduleNames, resolutions, oldSourceFile.resolvedModules, moduleResolutionIsEqualTo);
                    if (resolutionsChanged) {
-                        return false;
+                        return StructuralChangesFromOldProgram.Imports | StructuralChangesFromOldProgram.ModuleAugmentations;


you want to continue here instead of return to allow for copying the state of unchanged files.

mhegazy · 2017-05-01T19:51:23Z

src/compiler/program.ts

-                // - in the old program module name was resolved to ambient module whose declaration is in non-modified file
+                // TODO: if we want to reuse resolutions more aggressively, refine this to check for whether the
+                // text of the corresponding modulenames has changed.
+                if (file === oldSourceFile) {


can you add a test for the npm install case:

file1.ts => import * as a from "a";

semantic errors

Add new file node_modules\a\index.d.ts

make an edit in file2.ts

no semantic errors

mhegazy · 2017-05-01T19:53:11Z

src/compiler/program.ts


            // there is an old program, check if we can reuse its structure
            const oldRootNames = oldProgram.getRootFileNames();
            if (!arrayIsEqualTo(oldRootNames, rootNames)) {
-                return false;
+                oldProgram.structureIsReused = StructureIsReused.Not;
+                return StructureIsReused.Not;


return oldProgram.structureIsReused = StructureIsReused.Not;

mhegazy · 2017-05-01T19:53:24Z

src/compiler/program.ts

            }

            if (!arrayIsEqualTo(options.types, oldOptions.types)) {
-                return false;
+                oldProgram.structureIsReused = StructureIsReused.Not;


return oldProgram.structureIsReused = StructureIsReused.Not;

RyanCavanaugh · 2017-05-01T22:23:03Z

src/compiler/types.ts

+
+    /* @internal */
+    export const enum StructureIsReused {
+        Completely,


Start at 1 or make Not === 0

RyanCavanaugh · 2017-05-01T22:27:39Z

src/compiler/utilities.ts

@@ -123,7 +123,7 @@ namespace ts {
    /* @internal */
    export function hasChangesInResolutions<T>(names: string[], newResolutions: T[], oldResolutions: Map<T>, comparer: (oldResolution: T, newResolution: T) => boolean): boolean {
        if (names.length !== newResolutions.length) {
-            return false;
+            return true;


Figure out how to hit this in a test

(or change to an assert)

hasChangesInResolutions is called on resolutions obtained from resolveModuleNamesReusingOldState and resolveTypeReferenceDirectiveNamesWorker.
Both guarantee in their implementations that the array they return is the same length as names.

Changing to an assert.

billti · 2017-05-02T00:23:00Z

src/compiler/diagnosticMessages.json

+    },
+    "Reusing resolution of module '{0}' to file '{1}' from old program.": {
+        "category": "Message",
+        "code": 618


Did you mean 618 here? Seems kind low ;-)

billti · 2017-05-02T00:31:06Z

src/compiler/program.ts

-                if (!oldProgramState) {
-                    return false;
-                }
+            // TODO: if we change our policy of rechecking failed lookups on each program create,


Minor nit: But some of the TODO comments aren't really TODOs (in the sense that work is incomplete), but just a note for any future changes. Just remove the "TODO: " from these if there is no work still to do.

sheetalkamat and others added 6 commits April 18, 2017 14:10

Use base tsconfig's compileOnSave option if tsconfig.json doesnt have…

3f7b53e

… it specified Fixes microsoft#13910

Reuse module resolutions in unmodified files

d5f246f

Remove WIP comment

77f9099

mergeSymbol error uses first decl not valueDecl

effa032

Previously it assumed valueDeclaration was present, which is not true for type aliases.

Test:type alias merged w/interface-namespace errors

eba15f4

Previously it crashed when it assumed valueDeclaration was always defined.

cleanup

c3582d2

aozgaa requested review from billti and mhegazy April 18, 2017 23:33

msftclas added the cla-not-required label Apr 18, 2017

Andy Hanson and others added 4 commits April 19, 2017 07:07

Remove unused file

0e06793

Merge pull request microsoft#15268 from Microsoft/rm_goToImplementation

b8b801b

Remove unused file

Merge pull request microsoft#15258 from Microsoft/mergeSymbol-uses-fi…

a859399

…rst-decl-not-valueDecl mergeSymbol uses first declaration, not valueDeclaration

Merge pull request microsoft#15255 from Microsoft/extendCompileOnSave

5819402

Use base tsconfig's compileOnSave option if tsconfig.json doesnt have it specified

RyanCavanaugh reviewed Apr 19, 2017

View reviewed changes

src/compiler/program.ts Outdated

None = 0,

NoOldProgram = 1,

ModuleResolutionOptions = 2,

RootNames = 3,

Copy link

Member

RyanCavanaugh Apr 19, 2017

Choose a reason for hiding this comment

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

1 << 3, etc

Merge branch 'moduleReuse' of github.com:aozgaa/TypeScript into modul…

044152e

…eReuse

billti reviewed Apr 19, 2017

View reviewed changes

Arthur Ozga and others added 3 commits April 20, 2017 11:02

make enum bitflag and add trace message

7966c08

test module reuse

0ea1b82

consolidate program-structure reuse flags

ac20fc2

mhegazy suggested changes Apr 24, 2017

View reviewed changes

aozgaa added 5 commits April 27, 2017 20:27

check ambient modules before reusing old state

6547c18

inline createOldProgramState

86d7031

Avoid double-resolving modified files

c63d6d1

More thorough module reuse test

293a04c

clarifying comments

ef1cd50

mhegazy approved these changes May 1, 2017

View reviewed changes

aozgaa added 2 commits May 1, 2017 15:16

Simplify returns

2118de0

npm install test

28843c4

RyanCavanaugh reviewed May 1, 2017

View reviewed changes

src/compiler/types.ts Outdated

/* @internal */

export const enum StructureIsReused {

Completely,

Copy link

Member

RyanCavanaugh May 1, 2017

Choose a reason for hiding this comment

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

Start at 1 or make Not === 0

RyanCavanaugh reviewed May 1, 2017

View reviewed changes

RyanCavanaugh approved these changes May 1, 2017

View reviewed changes

aozgaa added 2 commits May 1, 2017 16:37

reorder enum

df3630b

assert length

7d1d22c

billti reviewed May 2, 2017

View reviewed changes

respond to comments

6082514

aozgaa merged commit 2150a77 into microsoft:master May 2, 2017

microsoft locked and limited conversation to collaborators Jun 14, 2018

Reuse Module Resolutions from Unchanged Files #15261

Reuse Module Resolutions from Unchanged Files #15261

Uh oh!

Conversation

aozgaa commented Apr 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aozgaa May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aozgaa May 1, 2017 •

edited

Loading