Repository navigation
tsc -b incorrectly succeeds when there is a problem introduced by a transitive dependency #46153
Description
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
Activity
andrewbranch commentedon Oct 4, 2021
This does look like a bug, but it also sounds like there may be a misconception that
projectCis somehow being included as a project reference ofprojectAthroughprojectB. It looks like that’s not the case here—I think you are directly includingprojectC/C.tsas a source ofprojectA/tsconfig.json.samreid commentedon Oct 4, 2021
projectA/tsconfig.jsononly specifies A.ts in its include section:And if we remove
{ "path": "../projectB" }fromprojectA/tsconfig.json, and runtsc -binprojectA, then we get this error: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 commentedon Oct 5, 2021
This is... surprising to me 😅 Could be a clue as to what’s going on.
andrewbranch commentedon Oct 5, 2021
Your hypothesis sounds plausible—it certainly sounds like the transitive project reference connecting
AtoCis being treated as a proper project reference (by the lack of therootDirerror), but not invalidatingA’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 fromCintoA(includeis 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 invalidateAwhenCchanges. 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 commentedon Nov 10, 2021
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 runtsc -b --cleanfrequently, 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 commentedon Nov 13, 2021
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
BorChas status ContainerOnly when it shouldn't.zepumph commentedon Mar 17, 2022
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 astsc -b --clean && tsc -bin order for files in projectAto be type checked. This comes at the cost of upwards of four minutes for each type check to complete.kathy-phet commentedon May 5, 2022
Ryan Cavanaugh (@RyanCavanaugh) Nathan Shively-Sanders (@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 commentedon May 6, 2022
I think this is working as intended.
projectAdoes not directly referenceprojectCso there is no way to know that change toprojectAshould affectprojectCdirectly. The references are not transitive and that's what determines what gets built again.Relevant discussion in #30608
andrewbranch commentedon May 6, 2022
If the reference is not transitive (which was my original understanding), why does
import C from '../projectC/C';insideprojectAnot issue a rootDir/all-files-must-be-listed error?samreid commentedon May 6, 2022
Thanks, I was unaware of the design decision in #30608 (comment)
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:
to
In practice, our project has transitive dependencies around 10 layers deep, so each level should list everything?
That would certainly help us ensure we didn't forget to list a transitive dependency as a direct dependency.
sheetalkamat commentedon May 6, 2022
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 commentedon May 6, 2022
Sheetal Nandi (@sheetalkamat) I still think #46153 (comment) either sounds like a bug or (more likely) I’m still not understanding something.
Sam Reid (@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 commentedon May 6, 2022
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 commentedon May 6, 2022
Ah, right, the rootDir restriction only applies to .ts files, since it’s about the output directory structure. That makes sense.
typescript-bot commentedon May 9, 2022
This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.
jwmerrill commentedon Jun 1, 2023
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.