tsc --build can pass even when there is a problem introduced by a dependency

[samreid]

Follow these steps:

(1) in GravityAndOrbitsScene.ts, add a line like:

this.tandem.runSomething();

(2) run tsc -b in gravity-and-orbits, it should fail
(3) Then in PhetioObject.js, comment out this.tandem=DEFAULTS.tandem
(4) run tsc -b in gravity and orbits, it should succeed since tandem now has any type.
(5) restore PhetioObject.js
(6) run tsc -b in gravity and orbits, it should fail like it did in step (2) since all of the code is exactly the same, but it succeeds for unknown reasons. Inspecting chipper/dist/PhetioObject.d.ts, it does have the correct type. So it seems that GravityAndOrbitsScene is not getting triggered to recompile. Also, surprisingly, tsc succeeds even as tsc -b fails.

~/apache-document-root/main/gravity-and-orbits$ tsc
js/common/GravityAndOrbitsScene.ts:119:17 - error TS2339: Property 'runSomething' does not exist on type 'Tandem'.

119     this.tandem.runSomething();
                    ~~~~~~~~~~~~


Found 1 error.

~/apache-document-root/main/gravity-and-orbits$ tsc -b
~/apache-document-root/main/gravity-and-orbits$ 

Running tsc -b --clean && tsc -b for each of the 3 compiles correctly shows the missing method. Therefore, it seems there is stale/bad intermediate data from the incremental or project build.

[samreid]

For now, perhaps we should use --clean for builds, since it is important to catch all the errors. The code output is the same, but it can be important to error out if there is a type error. But maybe we would catch it in the IDE in advance, but it seems flaky for the builds to not catch this. Also, if we run --clean all the time, it will significantly hamper our workflow.

[samreid]

I wonder if this overlaps with the caveat in https://www.typescriptlang.org/docs/handbook/project-references.html#caveats. Do we need to make sure noEmitOnError is truly enabled?

[samreid]

@jonathanolson and I discussed briefly. The next steps for this issue are:

• Reproduce the problem above. DONE: Behavior is the same buggy behavior.
• Create a self-contained reproducible case and see if it also has the problem
• Try more likely scenarios, such as introducing a type error in scenery or another dependency, and make sure it is picked up in the build
• Based on the above, search online to see if others had similar experience. If not (but we do have a small reproducible example), then report a bug to TypeScript.

[samreid]

Surprisingly, the self-contained example does not exhibit this problem. (Note that it is easier to switch between the tandem being a string vs any). I'm not sure if I have the tsconfigs matching our style exactly, or where the essential difference might be. Multiple layers to get to the dependency?

Details

Index: test/typescript/test-dependencies/gravity-and-orbits/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/gravity-and-orbits/tsconfig.json b/test/typescript/test-dependencies/gravity-and-orbits/tsconfig.json
new file mode 100644
--- /dev/null	(date 1631312913052)
+++ b/test/typescript/test-dependencies/gravity-and-orbits/tsconfig.json	(date 1631312913052)
@@ -0,0 +1,22 @@
+{
+  "references": [
+    {
+      "path": "../tandem"
+    }
+  ],
+  "compilerOptions": {
+    "module": "commonjs",
+    "target": "es5",
+    "sourceMap": true,
+    "composite": true,
+    "outDir": "./dist/",
+    "incremental": true,
+    "allowJs": true
+  },
+  "include": [
+    "GravityAndOrbitsScene.ts"
+  ],
+  "exclude": [
+    "node_modules"
+  ]
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/gravity-and-orbits/GravityAndOrbitsScene.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/gravity-and-orbits/GravityAndOrbitsScene.ts b/test/typescript/test-dependencies/gravity-and-orbits/GravityAndOrbitsScene.ts
new file mode 100644
--- /dev/null	(date 1631313092547)
+++ b/test/typescript/test-dependencies/gravity-and-orbits/GravityAndOrbitsScene.ts	(date 1631313092547)
@@ -0,0 +1,9 @@
+// Copyright 2021, University of Colorado Boulder
+import PhetioObject from '../tandem/PhetioObject';
+
+class GravityAndOrbitsScene extends PhetioObject {
+  constructor() {
+    super( {} );
+    this.tandem.runSomething();
+  }
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/tandem/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/tandem/tsconfig.json b/test/typescript/test-dependencies/tandem/tsconfig.json
new file mode 100644
--- /dev/null	(date 1631312555500)
+++ b/test/typescript/test-dependencies/tandem/tsconfig.json	(date 1631312555500)
@@ -0,0 +1,15 @@
+{
+  "compilerOptions": {
+    "module": "commonjs",
+    "target": "es5",
+    "sourceMap": true,
+    "composite": true,
+    "allowJs": true,
+    "outDir": "./dist/",
+    "incremental": true
+  },
+  "include": ["PhetioObject.js"],
+  "exclude": [
+    "node_modules"
+  ]
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/tandem/PhetioObject.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/tandem/PhetioObject.js b/test/typescript/test-dependencies/tandem/PhetioObject.js
new file mode 100644
--- /dev/null	(date 1631313103943)
+++ b/test/typescript/test-dependencies/tandem/PhetioObject.js	(date 1631313103943)
@@ -0,0 +1,16 @@
+// Copyright 2021, University of Colorado Boulder
+const DEFAULTS = {
+
+  tandem: 'optional'
+};
+
+class PhetioObject {
+  constructor( options ) {
+    this.tandem = 'its a tandem';
+
+    // other code
+    // this.tandem = options.tandem;
+  }
+}
+
+export default PhetioObject;
\ No newline at end of file

[samreid]

Multiple layers to get to the dependency?

That did the trick. I introduced an intermediate transitive dependency and now can reproduce the problem in an isolated case. I'll show a patch for this failing example, perhaps we can rename it to something sensible for sharing in a TypeScript issue.

Details

Index: test/typescript/test-dependencies/gravity-and-orbits/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/gravity-and-orbits/tsconfig.json b/test/typescript/test-dependencies/gravity-and-orbits/tsconfig.json
new file mode 100644
--- /dev/null	(date 1631332362009)
+++ b/test/typescript/test-dependencies/gravity-and-orbits/tsconfig.json	(date 1631332362009)
@@ -0,0 +1,22 @@
+{
+  "references": [
+    {
+      "path": "../joist"
+    }
+  ],
+  "compilerOptions": {
+    "module": "commonjs",
+    "target": "es5",
+    "sourceMap": true,
+    "composite": true,
+    "outDir": "./dist/",
+    "incremental": true,
+    "allowJs": true
+  },
+  "include": [
+    "GravityAndOrbitsScene.ts"
+  ],
+  "exclude": [
+    "node_modules"
+  ]
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/gravity-and-orbits/GravityAndOrbitsScene.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/gravity-and-orbits/GravityAndOrbitsScene.ts b/test/typescript/test-dependencies/gravity-and-orbits/GravityAndOrbitsScene.ts
new file mode 100644
--- /dev/null	(date 1631332680316)
+++ b/test/typescript/test-dependencies/gravity-and-orbits/GravityAndOrbitsScene.ts	(date 1631332680316)
@@ -0,0 +1,9 @@
+// Copyright 2021, University of Colorado Boulder
+import PhetioObject from '../tandem/PhetioObject';
+
+class GravityAndOrbitsScene extends PhetioObject {
+  constructor() {
+    super( {} );
+    this.tandem.runSomething();
+  }
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/tandem/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/tandem/tsconfig.json b/test/typescript/test-dependencies/tandem/tsconfig.json
new file mode 100644
--- /dev/null	(date 1631312555500)
+++ b/test/typescript/test-dependencies/tandem/tsconfig.json	(date 1631312555500)
@@ -0,0 +1,15 @@
+{
+  "compilerOptions": {
+    "module": "commonjs",
+    "target": "es5",
+    "sourceMap": true,
+    "composite": true,
+    "allowJs": true,
+    "outDir": "./dist/",
+    "incremental": true
+  },
+  "include": ["PhetioObject.js"],
+  "exclude": [
+    "node_modules"
+  ]
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/joist/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/joist/tsconfig.json b/test/typescript/test-dependencies/joist/tsconfig.json
new file mode 100644
--- /dev/null	(date 1631332655878)
+++ b/test/typescript/test-dependencies/joist/tsconfig.json	(date 1631332655878)
@@ -0,0 +1,19 @@
+{
+  "references": [
+    {
+      "path": "../tandem"
+    }
+  ],
+  "compilerOptions": {
+    "module": "commonjs",
+    "target": "es5",
+    "sourceMap": true,
+    "composite": true,
+    "allowJs": true,
+    "outDir": "./dist/",
+    "incremental": true
+  },
+  "exclude": [
+    "node_modules"
+  ]
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/tandem/PhetioObject.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/tandem/PhetioObject.js b/test/typescript/test-dependencies/tandem/PhetioObject.js
new file mode 100644
--- /dev/null	(date 1631332909359)
+++ b/test/typescript/test-dependencies/tandem/PhetioObject.js	(date 1631332909359)
@@ -0,0 +1,9 @@
+// Copyright 2021, University of Colorado Boulder
+class PhetioObject {
+  constructor( options ) {
+    // this.tandem = 'its a tandem';
+    this.tandem = options.tandem;
+  }
+}
+
+export default PhetioObject;
\ No newline at end of file

Instructions:

1. Run tsc -b --clean && tsc -b in gravity-and-orbits. This succeeds because tandem has type any
2. Swap the lines in PhetioObject, so tandem has type string.
3. Run tsc -b in gravity and orbits. It should fail, but it succeeds.

Note in the last step that tsc fails. And also tsc -b correctly fails if you don't have the intermediate transitive dependency (in this case, joist).

Here's a renamed version that uses A,B,C:

Details

Index: test/typescript/test-dependencies/projectA/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/projectA/tsconfig.json b/test/typescript/test-dependencies/projectA/tsconfig.json
new file mode 100644
--- /dev/null	(date 1631333534010)
+++ b/test/typescript/test-dependencies/projectA/tsconfig.json	(date 1631333534010)
@@ -0,0 +1,18 @@
+{
+  "references": [
+    {
+      "path": "../projectB"
+    }
+  ],
+  "compilerOptions": {
+    "module": "commonjs",
+    "target": "es5",
+    "composite": true,
+    "outDir": "./dist/",
+    "incremental": true,
+    "allowJs": true
+  },
+  "include": [
+    "A.ts"
+  ]
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/projectA/A.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/projectA/A.ts b/test/typescript/test-dependencies/projectA/A.ts
new file mode 100644
--- /dev/null	(date 1631333343040)
+++ b/test/typescript/test-dependencies/projectA/A.ts	(date 1631333343040)
@@ -0,0 +1,9 @@
+// Copyright 2021, University of Colorado Boulder
+import C from '../projectC/C';
+
+class A extends C {
+  constructor() {
+    super( {} );
+    this.name.arbitraryFakeMethod();
+  }
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/projectC/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/projectC/tsconfig.json b/test/typescript/test-dependencies/projectC/tsconfig.json
new file mode 100644
--- /dev/null	(date 1631333534012)
+++ b/test/typescript/test-dependencies/projectC/tsconfig.json	(date 1631333534012)
@@ -0,0 +1,11 @@
+{
+  "compilerOptions": {
+    "module": "commonjs",
+    "target": "es5",
+    "composite": true,
+    "allowJs": true,
+    "outDir": "./dist/",
+    "incremental": true
+  },
+  "include": ["C.js"]
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/projectB/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/projectB/tsconfig.json b/test/typescript/test-dependencies/projectB/tsconfig.json
new file mode 100644
--- /dev/null	(date 1631333534011)
+++ b/test/typescript/test-dependencies/projectB/tsconfig.json	(date 1631333534011)
@@ -0,0 +1,15 @@
+{
+  "references": [
+    {
+      "path": "../projectC"
+    }
+  ],
+  "compilerOptions": {
+    "module": "commonjs",
+    "target": "es5",
+    "composite": true,
+    "allowJs": true,
+    "outDir": "./dist/",
+    "incremental": true
+  }
+}
\ No newline at end of file
Index: test/typescript/test-dependencies/projectC/C.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/typescript/test-dependencies/projectC/C.js b/test/typescript/test-dependencies/projectC/C.js
new file mode 100644
--- /dev/null	(date 1631333461798)
+++ b/test/typescript/test-dependencies/projectC/C.js	(date 1631333461798)
@@ -0,0 +1,9 @@
+// Copyright 2021, University of Colorado Boulder
+class C {
+  constructor( options ) {
+    this.name = 'test name';   // takes type string
+    // this.name = options.name;     // infers type any
+  }
+}
+
+export default C;
\ No newline at end of file

Instructions:

1. Run tsc -b --clean && tsc -b in projectA. This correctly fails because name has type string and hence has no arbitraryFakeMethod.
2. Swap the lines in C.js, so name has type any
3. Run tsc -b in projectA. It succeeds.
4. Swap the lines in C.js back so we are back where we started.
5. Run tsc -b in projectA. It succeeds but should fail.

Note that switching projectA's tsconfig references to depend on projectC directly instead of through projectB correctly identifies the problem. The same problem occurs if projectC is a *.ts file C.ts instead of C.js (adding the field declaration as appropriate for each test).

Next steps:

• What if we turn incremental off? Could it be a bad interplay between incremental and project references? UPDATE: incremental cannot be disabled for project references.
• Try a practical test like changing something in scenery and see if we see the same effect in a sim.
• If this remains a problem, could a workaround to be tsc -b && tsc? How would that interplay with incremental?

[samreid]

Draft write up before creating a TypeScript issue:

Issue title: # tsc -b incorrectly succeeds when there is a problem introduced by a transitive dependency

Bug Report

project references transitive dependency stale

🕗 Version & Regression Information

• This changed between versions 3.6.0-dev.20190621 (good) and 3.6.0-dev.20190622 (bad)
  The incorrect behavior was observed in many versions since then, including:
  • 4.4.2
  • 4.4.3
  • 4.5.0-dev.20210928

⏯ Playground Link

Reproducing the problem requires transitive project references, and running tsc -b from the command line, so I don't believe it can be done in the TypeScript playground.

💻 Code

My team observed that tsc -b can incorrectly succeed when a problem is introduced by a transitive dependency. In fact, tsc -b can give two different results (type check passes or type check fails) for the same codebase. We created a self-contained reproducible example to demonstrate the problem. This creates 3 projects using project references, where project A depends on project B and that, in turn, depends on project C. There is one TypeScript file in project A, none in B and one in C.

Step 1: Create the following 5 files, distributed into directories projectA/, projectB/ and projectC/.

// projectA/A.ts
import C from '../projectC/C';

class A extends C {
  constructor() {
    super( {} );
    this.name.arbitraryFakeMethod();
  }
}

// projectA/tsconfig.json
{
  "references": [
    {
      "path": "../projectB"
    }
  ],
  "compilerOptions": {
    "module": "commonjs",
    "target": "es5",
    "composite": true,
    "outDir": "./dist/",
    "incremental": true,
    "allowJs": true
  },
  "include": [
    "A.ts"
  ]
}

// projectB/tsconfig.json
{
  "references": [
    {
      "path": "../projectC"
    }
  ],
  "compilerOptions": {
    "module": "commonjs",
    "target": "es5",
    "composite": true,
    "allowJs": true,
    "outDir": "./dist/",
    "incremental": true
  }
}

// projectC/C.ts
class C {
  name: string;
  constructor( options ) {
  }
}

export default C;

// projectC/tsconfig.json
{
  "compilerOptions": {
    "module": "commonjs",
    "target": "es5",
    "composite": true,
    "allowJs": true,
    "outDir": "./dist/",
    "incremental": true
  },
  "include": [
    "C.ts"
  ]
}

Step 2: clean and build project A

The clean step is not necessary on your first run, but we include it here in case you want to run through the steps again.

cd projectA
tsc -b --clean
tsc -b

In this case, the expected result is the same as the actual result, which is a build failure with this message:

A.ts:7:15 - error TS2339: Property 'arbitraryFakeMethod' does not exist on type 'string'.

7     this.name.arbitraryFakeMethod();
                ~~~~~~~~~~~~~~~~~~~


Found 1 error.

Step 3: Fix the error by updating C.ts.

In C.ts, change name: string; to name: any;

Step 4: build project A

tsc -b

Again, the expected result matches the actual result. tsc succeeds with no output.

Step 5. Reintroduce the problem in C.ts

In C.ts, change name: any; back to name: string;

Step 6. Build Project A

tsc -b

🙁 Actual behavior

There is no output from tsc because the type check and build passes successfully. This is incorrect because there is
a type error because string does not have a method arbitraryFakeMethod. Note that Step 6 is building the same code as in Step 2. However, in Step 2, the type error is correctly identified, but in Step 6 the type error is missed.

🙂 Expected behavior

Step 6 should produce the following error (as it correctly did in Step 2):

A.ts:7:15 - error TS2339: Property 'arbitraryFakeMethod' does not exist on type 'string'.

7     this.name.arbitraryFakeMethod();
                ~~~~~~~~~~~~~~~~~~~


Found 1 error.

Note that changing project A to depend on C directly (not through B) correctly identifies the problem when running tsc -b in project A, so this bug does require the intermediate transitive dependency. TypeScript correctly caught this error through Version 3.6.0-dev.20190621 (good), but started missing it in 3.6.0-dev.20190622 (bad). For completeness, we will mention that we first detected the problem when projectC was *.js code, so it seems to affect both *.ts and *.js dependencies.

[samreid]

@jonathanolson and/or @zepumph can one of you please volunteer to review the prior comment, which is a draft issue to report to TypeScript? You should follow the steps, see if you get the same result. Are the instructions clear? Do you have any ideas about the problem, or can you find any paper trail on it in https://github.com/microsoft/TypeScript/issues ? You can check out different versions of typescript like so: npm install typescript@3.6.0-dev.20190622. You can also see the bug template here: https://github.com/microsoft/TypeScript/issues/new/choose

Also, let me know if you prefer to discuss and work through this synchronously.

[samreid]

The bug report template recommended testing against the nightly build. I ran npm install typescript@next which gave Version 4.5.0-dev.20210928. Since I did not install it globally, I used node node_modules/typescript/bin/tsc instead of just tsc. But it appeared to have the same problem.

3.0.3 has this problem: 'rootDir' is expected to contain all source files. and some casing problems.
3.4.5 (version that introduced incremental builds) correctly catches the problem.
3.5.3 correctly catches the problem.
3.6.0-beta fails to catch the problem.
3.6.2 fails to catch the problem.
3.6.4 fails to catch the problem.

So this bug was introduced between 3.5.3 and 3.6.0-beta.

All releases are listed here: https://www.npmjs.com/package/typescript, so we can binary search:

3.6.0-dev.20190530 (first release in that series) - GOOD
3.6.0-dev.20190620 - GOOD
3.6.0-dev.20190704 - BAD
3.6.0-dev.20190627 - BAD
3.6.0-dev.20190623 - BAD
3.6.0-dev.20190621 - GOOD
3.6.0-dev.20190622 - BAD

So the problem was introduced in 3.6.0-dev.20190622. Looking at the TypeScript commits from that day, there are 3-6 commits that look like they relate to project references. There are no referenced issues in the commit messages.

[jonathanolson]

Verified #1067 (comment), reproduced the buggy case, directions were quite clear!