tsc -b incorrectly succeeds when there is a problem introduced by a transitive dependency

[samreid]

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. This problem was discovered in phetsims/chipper#1067

[andrewbranch]

This does look like a bug, but it also sounds like there may be a misconception that projectC is somehow being included as a project reference of projectA through projectB. It looks like that’s not the case here—I think you are directly including projectC/C.ts as a source of projectA/tsconfig.json.

[samreid]

This does look like a bug, but it also sounds like there may be a misconception that projectC is somehow being included as a project reference of projectA through projectB. It looks like that’s not the case here—I think you are directly including projectC/C.ts as a source of projectA/tsconfig.json.

projectA/tsconfig.json only specifies A.ts in its include section:

  "include": [
    "A.ts"
  ]

And if we remove { "path": "../projectB" } from projectA/tsconfig.json, and run tsc -b in projectA, then we get this error:

A.ts:2:15 - error TS6059: File '/path/test-dependencies/projectC/C.ts' is not under 'rootDir' '/path/test-dependencies/projectA'. 'rootDir' is expected to contain all source files.

2 import C from '../projectC/C';
                ~~~~~~~~~~~~~~~

To me, this suggests that the import statement import C from '../projectC/C'; does not establish a dependency on C.ts (at least from a build standpoint). Also, I should mention I'm new to TypeScript, so my apologies if I'm misunderstanding something simple.

[andrewbranch]

And if we remove { "path": "../projectB" } from projectA/tsconfig.json, and run tsc -b in projectA, then we get this error

This is... surprising to me 😅 Could be a clue as to what’s going on.

[andrewbranch]

Your hypothesis sounds plausible—it certainly sounds like the transitive project reference connecting A to C is being treated as a proper project reference (by the lack of the rootDir error), but not invalidating A’s status when it changes. I think my expectation was that this kind of transitive project reference would not be a project reference at all, but rather result in a direct inclusion of files from C into A (include is basically irrelevant here; anything imported either needs to be part of a referenced project or will get sucked into the project doing the importing). It sounds like your expectation was that the transitive project reference would “just work” and properly invalidate A when C changes. I’m honestly not sure which of these expectations is correct, but either would be better than the halfway state that we seem to be in.

[samreid]

Multiple members of my team have reported incorrect results when running tsc -b, it seems possible that it may be caused by (or related to) this issue. The latest occurrence was documented in phetsims/chipper#1144 in case it is helpful. Our only remedy at this point is to run tsc -b --clean frequently, or when we suspect trouble. This takes much longer and hampers our development efforts. It's difficult for me to believe that mine is the only team that suffers from this problem. If that problem is truly caused by (or related to) this issue, and we know this problem was introduced between versions 3.6.0-dev.20190621 (good) and 3.6.0-dev.20190622 (bad), then hopefully we can get to the root cause.

[sandersn]

There are a lot of commits on 21 Jun 2019. I think this was before we started squashing PRs into a single commit. The PRs that are likely relevant are
#32027 (merged on the 21st) and #32028 (lots of commits on the 21st, but not merged until 24 Sep 2019).

#32027 is a really simple bug fix. In getUpToDateStatusWorker, the loop over a project's references previously skipped over references to projects whose status was ComputingUpstream with the comment "it's a circular reference". The PR now also skips references with status ContainerOnly with the comment "ignore container-only projects".

I'll need to repro this myself, but from the commentary above on previous investigation, it sounds like maybe B or C has status ContainerOnly when it shouldn't.

[zepumph]

This issue is quickly becoming detrimental to productivity in the project that I work on. Are there any updates on timeline here?

When making changes in a project C, I have to run type checking as tsc -b --clean && tsc -b in order for files in project A to be type checked. This comes at the cost of upwards of four minutes for each type check to complete.

[kathy-phet]

@RyanCavanaugh @sandersn - Anyway we elevate the priority of the bug, and the timing of the fix. It's really causing major problems for our project. Thank you!

[sheetalkamat]

I think this is working as intended. projectA does not directly reference projectC so there is no way to know that change to projectA should affect projectC directly. The references are not transitive and that's what determines what gets built again.
Relevant discussion in #30608

[andrewbranch]

If the reference is not transitive (which was my original understanding), why does import C from '../projectC/C'; inside projectA not issue a rootDir/all-files-must-be-listed error?

[samreid]

Thanks, I was unaware of the design decision in #30608 (comment)

Not having references be transitive was an intentional design decision when we wrote project references

Does this mean we are supposed to list all transitive dependencies explicitly at each level in order to achieve accurate type-checking behavior? For instance, in the example at the top of the page, projectA would change from:

  "references": [
    {
      "path": "../projectB"
    }
  ]

to

  "references": [
    {
      "path": "../projectB",
      "path": "../projectC"
    }
  ]

In practice, our project has transitive dependencies around 10 layers deep, so each level should list everything?

If the reference is not transitive (which was my original understanding), why does import C from '../projectC/C'; inside projectA not issue a rootDir/all-files-must-be-listed error?

That would certainly help us ensure we didn't forget to list a transitive dependency as a direct dependency.

[sheetalkamat]

By having project reference, you are telling that any change to these project can affect the project. So it depends on your granularity of the project and what you want the distiction be

[andrewbranch]

@sheetalkamat I still think #46153 (comment) either sounds like a bug or (more likely) I’m still not understanding something.

@samreid the way I have always set up project references is if I import directly from another project (as you imported projectC from projectA in your example), I list projectC as a reference of projectA. On the other hand, if I use exports from projectC only indirectly through projectB, I only list projectB as a reference of projectA.

[sheetalkamat]

If the reference is not transitive (which was my original understanding), why does import C from '../projectC/C'; inside projectA not issue a rootDir/all-files-must-be-listed error?

Even if you are not directly importing projectC, projectB's types could depend on projectA(and you might not be using those types) in that case your program still needs projectC
When we added project references we didnt have these clever understanding of when and why files are included baked in. Now we have but i think its too late to formulate error as it could affect configurations in wild to report error if you are directly importing projectC but not referencing it in the config,
Note you dont get error because we use d.ts files transitively that is program will have c.d.ts and not .ts

[andrewbranch]

Ah, right, the rootDir restriction only applies to .ts files, since it’s about the output directory structure. That makes sense.

[typescript-bot]

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

[jwmerrill]

I think its too late to formulate error as it could affect configurations in wild to report error if you are directly importing projectC but not referencing it in the config

It would be really helpful to have more support from the compiler for making sure project references are set up in a way that will result in correct caching behavior from tsc --build.

We recently migrated to project references, and we're now running into some issues with incorrect caching.

I was not previously aware of the requirement to list transitive dependencies in project references in order for build caching to be correctly invalidated. I assumed that if my project type checked, then the references must be set up correctly.

If it's too disruptive to make it an error to fail to list transitive dependencies in project references, perhaps the build caching system should be updated to invalidate its cache correctly without having references to transitive dependencies explicitly listed.