-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix relations for instantiations of same generic signature #31029
Conversation
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 78f54c5. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 78f54c5. You can monitor the build here. It should now contribute to this PR's status checks. |
RWC summary One error elaboration change (possibly unrelated?) A somewhat interesting example interface Alpha<S> {
foo<T extends S>(s: T): any;
}
interface Beta extends Alpha<"b"> { }
interface Gamma extends Alpha<"c"> { }
// Previously not an error
// Now an error because of conflicting definitions of 'foo'
interface Delta extends Beta, Gamma { } This one is from TSLint: export function ancestorWhere<T extends ts.Node>(node: ts.Node, predicate: (n: ts.Node) => n is T): T | undefined;
export function ancestorWhere(node: ts.Node, predicate: (n: ts.Node) => boolean): ts.Node | undefined;
~~~~~~~~~~~~~
!!! error TS2394: This overload signature is not compatible with its implementation signature.
!!! related TS2750 language/utils.ts:98:17: The implementation signature is declared here.
export function ancestorWhere<T extends ts.Node>(node: ts.Node, predicate: (n: ts.Node) => n is T): T | undefined { |
Does this also fix #30988? EDIT: No |
@typescript-bot run dt |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at df50477. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at df50477. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 39c263b. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot run dt slower |
Heya @ahejlsberg, I've started to run the Definitely Typed test suite on this PR at 39c263b. You can monitor the build here. It should now contribute to this PR's status checks. |
@weswigham Looks like the DT test runner has an issue. The last three runs above all failed with OOM errors towards the end, but not in a consistent spot. |
Even the "slower" runner is still running DT in multiple threads in parallel on a single container, so the output log order isn't strictly deterministic. Since in the container-sharded run only one shard runs OOM I'd wager a specific package is running OOM - @sandersn do you know if we can get better logging so we know which? |
The RWC tests now look good. One insignificant change in an error elaboration and one new error related to multiple inherited methods. Since we've always had a rule that multiple inherited methods must be identical I think the new error is fine. The methods aren't identical, we just failed to detect it before. There are two issues in Ember that are uncovered by the stricter checking. One is in Even with fixes, I did notice that Also, we need to figure out what the OOM error in the DT runs is about. |
@weswigham are you starting dtslint-runner with 4GB of max old space? The nightly run does that, so that may already be true. @rbuckton improved types-publisher’s parallel runner to report oom failures correctly on Friday. Probably the right thing to do is port that code to dtslint-runner. @ahejlsberg I believe you can run dtslint-runner with a local typescript and only one thread, so you can that as a workaround for now. |
This comment has been minimized.
This comment has been minimized.
@typescript-bot run dt since now it should log what's running OOM. |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at c2b79df. You can monitor the build here. It should now contribute to this PR's status checks. |
strophe is still running out of memory |
I think this looks good as long as it no longer breaks DT. I'll merge from master and re-run. |
Unfortunately when I merge from master, another test is now broken, bestChoiceType. I may have made a mistake trying to convert the |
# Conflicts: # src/compiler/checker.ts # tests/baselines/reference/keyofAndIndexedAccess2.errors.txt # tests/baselines/reference/keyofAndIndexedAccess2.symbols
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at e262f49. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at e262f49. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at e262f49. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here they are:Comparison Report - master..31029
System
Hosts
Scenarios
|
@ahejlsberg unfortunately, this now needs another merge from master |
In the design meeting, we decided to see whether it makes sense to put this under strictFunctionTypes, or some other strict flag -- possibly a new one. |
@ahejlsberg Is it worthwhile to keep working on this fix? It's been inactive for 6 months now. |
This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. |
Fixes #31006.