createProgram corrupts updated SourceFile

[ajafff]

TypeScript Version: 3.1.0-dev.20180801

Search Terms:

Code

For the last two days I'm investigating a crash of my linter when fixing files, updating existing SourceFiles and creating a Program from these SourceFiles: fimbullinter/wotan#361

First you need 2 files that are processed by the repro script:

tsconfig.json

{
  "include": [
    "test.ts"
  ]
}

test.ts

export class Rule {
    private getPropertyInfo() {
        return
    }
}

(name) => b;

Now add the following script (using __dirname in the script assumes it's in the same directory as the above files, that can be changed though):

repro.js

"use strict";

const ts = require("typescript");
const {strictEqual} = require("assert");

const config = ts.parseJsonConfigFileContent(ts.readJsonConfigFile(__dirname + '/tsconfig.json', ts.sys.readFile), ts.sys, __dirname, undefined, __dirname + '/tsconfig.json');
let program = ts.createProgram(config.fileNames, config.options, ts.createCompilerHost(config.options, true));

let sourceFile = program.getSourceFile(__dirname + '/test.ts');
// add a semicolon after `return`
const newCode = sourceFile.text.slice(0, 66) + ';' + sourceFile.text.slice(66);
sourceFile = ts.updateSourceFile(sourceFile, newCode, { newLength: 1, span: { start: 66, length: 0 } });
strictEqual(sourceFile.statements[1].getSourceFile(), sourceFile, 'parent reference should be updated')

const originalHost = ts.createCompilerHost(config.options, true);
const host = {
    ...originalHost,
    getSourceFile(path, version) {
        if (path === sourceFile.fileName)
            return sourceFile;
        return originalHost.getSourceFile(path, version);
    },
};
program = ts.createProgram(program.getRootFileNames(), program.getCompilerOptions(), host, program);
strictEqual(program.getSourceFile(__dirname + '/test.ts'), sourceFile, 'new Program should contain the updated SourceFile');
strictEqual(sourceFile.statements[1].getSourceFile(), sourceFile, 'parent reference should still be the same');

Execute node repro.js

Expected behavior:

All assertions pass

Actual behavior:

Last assertion fails.
There is no error if I use ts.createSourceFile instead of ts.updateSourceFile

[RyanCavanaugh]

The comments on updateSourceFile are instructive:

    // Note: this function mutates nodes from this SourceFile. That means any existing nodes
    // from this SourceFile that are being held onto may change as a result (including
    // becoming detached from any SourceFile).  It is recommended that this SourceFile not
    // be used once 'update' is called on it.
    export function updateSourceFile(sourceFile: SourceFile, newText: string, textChangeRange: TextChangeRange, aggressiveChecks = false): SourceFile {

The default implementation of getSourceFile returns a new object every time and this is generally what you should do on your end unless you have guarantees that the object you recycle hasn't been modified.

[ajafff]

Note that the script no longer uses the old SourceFile object after it's updated.
So this must be a bug in createProgram as it assumes all old sourcefiles are still valid and its Nodes can be mutated at will.

[RyanCavanaugh]

program = ts.createProgram(program.getRootFileNames(), program.getCompilerOptions(), host, program);

createProgram will always attempt to re-use the source files from the last oldProgram parameter - if a program contains source files that have been mutated into invalidness by updateSourceFile, it's not a safe argument to provide to oldProgram

[ajafff]

createProgram will always attempt to re-use the source files from the last oldProgram parameter

I disagree. It always uses the new SourceFiles returned from the host. It only tries to avoid processing files that haven't changed.

I looked through all of the code involved in creating a Program from an oldProgram. Everything is working as expected and nothing could break even with old (now invalid) SourceFiles that were updated. There's just one line that assumes every SourceFile is still valid:
https://github.com/Microsoft/TypeScript/blob/9df88316a2f26a83dd5635e149038f0c10168e63/src/compiler/program.ts#L963
If I add program.getTypeChecker() after creating the initial Program, it works because parent pointers are only updated during creation of the TypeChecker.

[ajafff]

I tried rewriting that function to no longer depend on TypeChecker or Nodes within the old SourceFiles. I don't fully understand why it was implemented that way. So maybe my implementation doesn't do exactly the same.

diff --git a/src/compiler/program.ts b/src/compiler/program.ts
index c70477b4a1..453d465a1e 100644
--- a/src/compiler/program.ts
+++ b/src/compiler/program.ts
@@ -951,8 +951,11 @@ namespace ts {
             // If we change our policy of rechecking failed lookups on each program create,
             // we should adjust the value returned here.
             function moduleNameResolvesToAmbientModuleInNonModifiedFile(moduleName: string, oldProgramState: OldProgramState): boolean {
+                if (!oldProgramState.program) {
+                    return false;
+                }
                 const resolutionToFile = getResolvedModule(oldProgramState.oldSourceFile!, moduleName); // TODO: GH#18217
-                const resolvedFile = resolutionToFile && oldProgramState.program && oldProgramState.program.getSourceFile(resolutionToFile.resolvedFileName);
+                const resolvedFile = resolutionToFile && oldProgramState.program.getSourceFile(resolutionToFile.resolvedFileName);
                 if (resolutionToFile && resolvedFile && !resolvedFile.externalModuleIndicator) {
                     // In the old program, we resolved to an ambient module that was in the same
                     //   place as we expected to find an actual module file.
@@ -960,16 +963,11 @@ namespace ts {
                     //   because the normal module resolution algorithm will find this anyway.
                     return false;
                 }
-                const ambientModule = oldProgramState.program && oldProgramState.program.getTypeChecker().tryFindAmbientModuleWithoutAugmentations(moduleName);
-                if (!(ambientModule && ambientModule.declarations)) {
-                    return false;
-                }

                 // at least one of declarations should come from non-modified source file
-                const firstUnmodifiedFile = forEach(ambientModule.declarations, d => {
-                    const f = getSourceFileOfNode(d);
-                    return !contains(oldProgramState.modifiedFilePaths, f.path) && f;
-                });
+                const firstUnmodifiedFile = oldProgramState.program.getSourceFiles().find(
+                    (f) => !contains(oldProgramState.modifiedFilePaths, f.path) && contains(f.ambientModuleNames, moduleName)
+                );

                 if (!firstUnmodifiedFile) {
                     return false;