microsoft
Fixed crash in import fixes for augmented modules
Starting jobs; this comment will be updated as builds start and complete.
| Command | Status | Results |
|---|---|---|
test top400 |
✅ Started | 👀 Results |
user test this |
✅ Started | ✅ Results |
run dt |
✅ Started | ✅ Results |
perf test this faster |
✅ Started | 👀 Results |
Hey @iisaduan, the results of running the DT tests are ready.
Everything looks the same!
@iisaduan Here are the results of running the user tests with tsc comparing main and refs/pull/58965/merge:
Everything looks good!
@iisaduan The results of the perf run you requested are in!
Here they are:
tsc
Comparison Report - baseline..pr| Metric | baseline | pr | Delta | Best | Worst | p-value |
|---|---|---|---|---|---|---|
| Compiler-Unions - node (v18.15.0, x64) | ||||||
| Errors | 30 | 30 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 62,153 | 62,153 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 50,242 | 50,242 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 192,745k (± 0.77%) | 192,743k (± 0.75%) | ~ | 192,116k | 195,687k | p=0.575 n=6 |
| Parse Time | 1.31s (± 0.62%) | 1.30s (± 0.63%) | ~ | 1.29s | 1.31s | p=0.389 n=6 |
| Bind Time | 0.71s | 0.71s | ~ | ~ | ~ | p=1.000 n=6 |
| Check Time | 9.47s (± 0.39%) | 9.47s (± 0.31%) | ~ | 9.44s | 9.52s | p=1.000 n=6 |
| Emit Time | 2.75s (± 0.59%) | 2.75s (± 0.59%) | ~ | 2.73s | 2.77s | p=1.000 n=6 |
| Total Time | 14.23s (± 0.27%) | 14.23s (± 0.13%) | ~ | 14.21s | 14.26s | p=0.809 n=6 |
| angular-1 - node (v18.15.0, x64) | ||||||
| Errors | 5 | 5 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 944,114 | 944,114 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 407,051 | 407,051 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 1,218,342k (± 0.00%) | 1,218,341k (± 0.00%) | ~ | 1,218,317k | 1,218,401k | p=0.689 n=6 |
| Parse Time | 6.65s (± 1.13%) | 6.66s (± 0.61%) | ~ | 6.62s | 6.71s | p=1.000 n=6 |
| Bind Time | 1.86s (± 0.53%) | 1.86s (± 0.65%) | ~ | 1.85s | 1.88s | p=0.865 n=6 |
| Check Time | 30.61s (± 0.28%) | 30.61s (± 0.48%) | ~ | 30.46s | 30.86s | p=0.685 n=6 |
| Emit Time | 13.59s (± 0.38%) | 13.57s (± 0.79%) | ~ | 13.45s | 13.77s | p=0.376 n=6 |
| Total Time | 52.72s (± 0.30%) | 52.70s (± 0.34%) | ~ | 52.47s | 52.94s | p=1.000 n=6 |
| mui-docs - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 2,135,096 | 2,135,096 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 927,167 | 927,167 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 2,117,330k (± 0.00%) | 2,117,407k (± 0.01%) | ~ | 2,117,292k | 2,117,571k | p=0.298 n=6 |
| Parse Time | 6.64s (± 0.28%) | 6.64s (± 0.54%) | ~ | 6.60s | 6.70s | p=0.414 n=6 |
| Bind Time | 2.34s (± 0.63%) | 2.34s (± 1.06%) | ~ | 2.29s | 2.36s | p=1.000 n=6 |
| Check Time | 70.97s (± 0.30%) | 70.67s (± 1.33%) | ~ | 68.85s | 71.40s | p=0.810 n=6 |
| Emit Time | 0.13s (± 3.87%) | 0.14s (± 6.19%) | ~ | 0.13s | 0.15s | p=0.923 n=6 |
| Total Time | 80.09s (± 0.27%) | 79.78s (± 1.13%) | ~ | 78.04s | 80.45s | p=0.936 n=6 |
| self-build-src - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 1,230,971 | 1,230,974 | +3 (+ 0.00%) | ~ | ~ | p=0.001 n=6 |
| Types | 261,250 | 261,250 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 2,345,659k (± 0.03%) | 2,347,086k (± 0.01%) | +1,426k (+ 0.06%) | 2,346,715k | 2,347,632k | p=0.013 n=6 |
| Parse Time | 5.01s (± 0.91%) | 5.01s (± 0.60%) | ~ | 4.98s | 5.04s | p=0.872 n=6 |
| Bind Time | 1.91s (± 0.54%) | 1.91s (± 0.70%) | ~ | 1.89s | 1.93s | p=1.000 n=6 |
| Check Time | 33.87s (± 0.30%) | 33.81s (± 0.23%) | ~ | 33.72s | 33.92s | p=0.298 n=6 |
| Emit Time | 2.70s (± 2.75%) | 2.59s (± 0.68%) | 🟩-0.11s (- 4.01%) | 2.58s | 2.62s | p=0.020 n=6 |
| Total Time | 43.51s (± 0.38%) | 43.35s (± 0.19%) | ~ | 43.25s | 43.45s | p=0.066 n=6 |
| self-build-src-public-api - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 1,230,971 | 1,230,974 | +3 (+ 0.00%) | ~ | ~ | p=0.001 n=6 |
| Types | 261,250 | 261,250 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 2,422,721k (± 0.03%) | 2,422,910k (± 0.02%) | ~ | 2,422,396k | 2,423,797k | p=0.471 n=6 |
| Parse Time | 7.70s (± 0.88%) | 7.69s (± 0.68%) | ~ | 7.61s | 7.74s | p=0.470 n=6 |
| Bind Time | 2.50s (± 1.17%) | 2.48s (± 0.69%) | ~ | 2.46s | 2.51s | p=0.258 n=6 |
| Check Time | 49.52s (± 0.21%) | 49.49s (± 0.49%) | ~ | 49.08s | 49.81s | p=0.936 n=6 |
| Emit Time | 3.95s (± 2.01%) | 3.92s (± 1.53%) | ~ | 3.86s | 4.02s | p=0.688 n=6 |
| Total Time | 63.67s (± 0.16%) | 63.60s (± 0.42%) | ~ | 63.14s | 63.98s | p=0.575 n=6 |
| self-compiler - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 258,575 | 258,578 | +3 (+ 0.00%) | ~ | ~ | p=0.001 n=6 |
| Types | 104,826 | 104,826 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 428,175k (± 0.01%) | 428,257k (± 0.01%) | +82k (+ 0.02%) | 428,201k | 428,372k | p=0.008 n=6 |
| Parse Time | 3.31s (± 0.35%) | 3.33s (± 0.85%) | ~ | 3.30s | 3.37s | p=0.288 n=6 |
| Bind Time | 1.32s (± 1.31%) | 1.32s (± 1.30%) | ~ | 1.30s | 1.34s | p=0.682 n=6 |
| Check Time | 17.77s (± 0.40%) | 17.76s (± 0.20%) | ~ | 17.72s | 17.81s | p=0.748 n=6 |
| Emit Time | 1.36s (± 1.23%) | 1.35s (± 1.12%) | ~ | 1.34s | 1.38s | p=0.621 n=6 |
| Total Time | 23.76s (± 0.37%) | 23.77s (± 0.24%) | ~ | 23.70s | 23.86s | p=0.936 n=6 |
| ts-pre-modules - node (v18.15.0, x64) | ||||||
| Errors | 35 | 35 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 224,565 | 224,565 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 93,734 | 93,734 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 369,463k (± 0.02%) | 369,510k (± 0.04%) | ~ | 369,400k | 369,745k | p=0.630 n=6 |
| Parse Time | 2.76s (± 0.89%) | 2.76s (± 1.24%) | ~ | 2.71s | 2.80s | p=0.936 n=6 |
| Bind Time | 1.58s (± 0.98%) | 1.60s (± 1.41%) | ~ | 1.57s | 1.62s | p=0.157 n=6 |
| Check Time | 15.44s (± 0.25%) | 15.46s (± 0.26%) | ~ | 15.41s | 15.51s | p=0.422 n=6 |
| Emit Time | 0.00s | 0.00s | ~ | ~ | ~ | p=1.000 n=6 |
| Total Time | 19.79s (± 0.18%) | 19.82s (± 0.30%) | ~ | 19.76s | 19.92s | p=0.469 n=6 |
| vscode - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 2,881,942 | 2,881,942 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 975,887 | 975,887 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 3,043,273k (± 0.00%) | 3,043,246k (± 0.00%) | ~ | 3,043,198k | 3,043,296k | p=0.230 n=6 |
| Parse Time | 13.52s (± 0.13%) | 13.53s (± 0.31%) | ~ | 13.47s | 13.58s | p=0.747 n=6 |
| Bind Time | 4.18s (± 0.23%) | 4.18s (± 0.44%) | ~ | 4.15s | 4.20s | p=0.869 n=6 |
| Check Time | 73.40s (± 0.37%) | 73.63s (± 0.38%) | ~ | 73.33s | 74.06s | p=0.199 n=6 |
| Emit Time | 24.01s (± 0.51%) | 23.99s (± 1.31%) | ~ | 23.37s | 24.23s | p=0.521 n=6 |
| Total Time | 115.12s (± 0.24%) | 115.32s (± 0.23%) | ~ | 115.08s | 115.76s | p=0.173 n=6 |
| webpack - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 267,117 | 267,117 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 108,775 | 108,775 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 411,529k (± 0.01%) | 411,549k (± 0.02%) | ~ | 411,463k | 411,654k | p=0.689 n=6 |
| Parse Time | 3.82s (± 0.51%) | 3.81s (± 0.68%) | ~ | 3.79s | 3.85s | p=0.625 n=6 |
| Bind Time | 1.69s (± 0.32%) | 1.70s (± 0.80%) | ~ | 1.68s | 1.72s | p=1.000 n=6 |
| Check Time | 16.74s (± 0.31%) | 16.70s (± 0.36%) | ~ | 16.63s | 16.78s | p=0.290 n=6 |
| Emit Time | 0.00s | 0.00s | ~ | ~ | ~ | p=1.000 n=6 |
| Total Time | 22.26s (± 0.30%) | 22.21s (± 0.17%) | ~ | 22.16s | 22.25s | p=0.291 n=6 |
| xstate-main - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 525,251 | 525,251 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 178,574 | 178,574 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 462,886k (± 0.06%) | 462,552k (± 0.07%) | ~ | 462,323k | 463,070k | p=0.066 n=6 |
| Parse Time | 3.17s (± 0.55%) | 3.18s (± 0.88%) | ~ | 3.14s | 3.21s | p=0.936 n=6 |
| Bind Time | 1.17s (± 1.37%) | 1.17s (± 0.47%) | ~ | 1.16s | 1.17s | p=0.855 n=6 |
| Check Time | 17.91s (± 0.32%) | 17.93s (± 0.44%) | ~ | 17.86s | 18.07s | p=0.936 n=6 |
| Emit Time | 0.00s | 0.00s | ~ | ~ | ~ | p=1.000 n=6 |
| Total Time | 22.26s (± 0.18%) | 22.27s (± 0.32%) | ~ | 22.18s | 22.39s | p=1.000 n=6 |
- node (v18.15.0, x64)
- Compiler-Unions - node (v18.15.0, x64)
- angular-1 - node (v18.15.0, x64)
- mui-docs - node (v18.15.0, x64)
- self-build-src - node (v18.15.0, x64)
- self-build-src-public-api - node (v18.15.0, x64)
- self-compiler - node (v18.15.0, x64)
- ts-pre-modules - node (v18.15.0, x64)
- vscode - node (v18.15.0, x64)
- webpack - node (v18.15.0, x64)
- xstate-main - node (v18.15.0, x64)
| Benchmark | Name | Iterations |
|---|---|---|
| Current | pr | 6 |
| Baseline | baseline | 6 |
Developer Information:
@iisaduan Here are the results of running the top 400 repos with tsc comparing main and refs/pull/58965/merge:
Something interesting changed - please have a look.
Details
dotansimha/graphql-code-generator
8 of 23 projects failed to build with the old tsc and were ignored
examples/react/urql/tsconfig.json
error TS2383: Overload signatures must all be exported or non-exported.
The new error might be a positive change but I'm not entirely sure. Module augmentation is a weird beast.
Test case:
// @strict: true
// @module: nodenext
// @moduleResolution: nodenext
// @noEmit: true
// @skipLibCheck: true
// @filename: node_modules/urql/index.d.ts
type AnyVariables =
| {
[prop: string]: any;
}
| void
| undefined;
declare function useQuery<Data = any, Variables extends AnyVariables = AnyVariables>(
args: Variables
): Data;
export { AnyVariables, useQuery };
// @filename: src/index.ts
import { AnyVariables, useQuery } from 'urql';
declare module 'urql' {
export function useQuery<Data = any, Variables extends AnyVariables = AnyVariables>(
name: string,
args: Variables
): Data;
}
The important ingredient here is skipLibCheck: true. The reported error gets reported with the current TS version only with skipLibCheck: false. With this branch it errors "consistently" with skipLibCheck and without it. Since the error is reported in both scenario in a .ts file... that's perhaps an improvement?
That said, I'm probably somewhat surprised that this is an error at all. I guess it's different to export a declaration using a modifier or using a separate export declaration but... it really feels like a detail that the user shouldn't be concerned with.
I'm also not sure how to even augment this correctly since this is not allowed
That said, I'm probably somewhat surprised that this is an error at all. I guess it's different to export a declaration using a modifier or using a separate export declaration but... it really feels like a detail that the user shouldn't be concerned with.
I agree that it seems like a detail that users shouldn't need to worry about, since without the module augmentation, they should be the same, and it seems we allow export function useQuery<.... in the module augmentation as a declaration. I haven't had time to fully review the PR yet, but I was able to strip your original test case even further (below). It's interesting that the completion works if in index.d.ts, you remove Container out of the export list, and instead export the declaration: export interface Container
// @module: nodenext
// @Filename: ./node_modules/@sapphire/pieces/index.d.ts
//// interface Container {
//// stores: unknown;
//// }
////
//// declare class Piece {
//// container: Container;
//// }
////
//// export { Piece, type Container };
// @FileName: ./augmentation.ts
//// declare module "@sapphire/pieces" {
//// interface Container {
//// client: unknown;
//// }
//// }
// @Filename: ./index.ts
//// import { Piece } from "@sapphire/pieces";
//// class FullPiece extends Piece {
//// /*1*/
//// }
I haven't had time to fully review the PR yet, but I was able to strip your original test case even further (below).
Thanks. I just stopped reducing at the point that test could be easily debugged and proceeded to fixing it 😅 I added this now as an extra test case here.
It's interesting that the completion works if in index.d.ts, you remove Container out of the export list, and instead export the declaration: export interface Container
That's likely related to the directly exported Container having .exportSymbol. The local symbol that gets exported through a separate export declaration doesn't have it.
Is there any progress on this? Like, a dev-release that can be installed through NPM with a fix, or maybe a workaround?
Is there any progress on this? Like, a dev-release that can be installed through NPM with a fix, or maybe a workaround?
Once this or another bug fix PR is merged, the nightly build of that day will contain the fix.
The new error might be a positive change but I'm not entirely sure. Module augmentation is a weird beast.
Test case:
// @strict: true // @module: nodenext // @moduleResolution: nodenext // @noEmit: true // @skipLibCheck: true // @filename: node_modules/urql/index.d.ts type AnyVariables = | { [prop: string]: any; } | void | undefined; declare function useQuery<Data = any, Variables extends AnyVariables = AnyVariables>( args: Variables ): Data; export { AnyVariables, useQuery }; // @filename: src/index.ts import { AnyVariables, useQuery } from 'urql'; declare module 'urql' { export function useQuery<Data = any, Variables extends AnyVariables = AnyVariables>( name: string, args: Variables ): Data; }The important ingredient here is
skipLibCheck: true. The reported error gets reported with the current TS version only withskipLibCheck: false. With this branch it errors "consistently" withskipLibCheckand without it. Since the error is reported in both scenario in a.tsfile... that's perhaps an improvement?That said, I'm probably somewhat surprised that this is an error at all. I guess it's different to export a declaration using a modifier or using a separate export declaration but... it really feels like a detail that the user shouldn't be concerned with.
I'm also not sure how to even augment this correctly since this is not allowed
The new error might be a positive change but I'm not entirely sure. Module augmentation is a weird beast.
Test case:
// @strict: true // @module: nodenext // @moduleResolution: nodenext // @noEmit: true // @skipLibCheck: true // @filename: node_modules/urql/index.d.ts type AnyVariables = | { [prop: string]: any; } | void | undefined; declare function useQuery<Data = any, Variables extends AnyVariables = AnyVariables>( args: Variables ): Data; export { AnyVariables, useQuery }; // @filename: src/index.ts import { AnyVariables, useQuery } from 'urql'; declare module 'urql' { export function useQuery<Data = any, Variables extends AnyVariables = AnyVariables>( name: string, args: Variables ): Data; }The important ingredient here is
skipLibCheck: true. The reported error gets reported with the current TS version only withskipLibCheck: false. With this branch it errors "consistently" withskipLibCheckand without it. Since the error is reported in both scenario in a.tsfile... that's perhaps an improvement?That said, I'm probably somewhat surprised that this is an error at all. I guess it's different to export a declaration using a modifier or using a separate export declaration but... it really feels like a detail that the user shouldn't be concerned with.
I'm also not sure how to even augment this correctly since this is not allowed
This new error seems "correct" in that those two useQuery declarations should be considered overloads of the same function, but the error itself is of course wrong, because they are both exported, even though TypeScript doesn't think so. This last part is a bug tracked by #58756.
having the same issue, is this ready to merge? @Andarist @gabritto @iisaduan
