redo #28564 #36665
redo #28564 #36665
Conversation
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 4e3c2c5. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..36665
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
No impact to check time, but it doesn't mean much without knowing how many overload errors are in the perf test suite. That's the next thing I need to look at. |
|
Hey, it at least means that the hypothesis of "this should only affect compilations with errors" is probably true~ |
|
So, uh, here are the numbers of getCandidateForOverloadFailure calls. They are small. The number in parenthesis is the total number of resolveErrorCalls; that function is called a number of places.
I'm still leaning toward yes since it appears (1) to be pay-to-use (2) angular, the project that uses it a little, on the whole checks at the same speed. @weswigham if you agree, sign off and you or I can merge this. |
|
Actually, I want to look over the baseline changes before I sign off. So far the look benign, but there are a lot of them. |
There was a problem hiding this comment.
I'm pretty much always fine with slight degredation to error-case performance in exchange for either better performance elsewhere or better errors, since "constant and extensive errors" is not the steady state for a codebase, and this should allow us to have much better API and real-world services performance (since we'll be able to reuse the hot diagnostics-producing checker from the constantly made err event for quick info). As for the baselines, it looked to me like a lot were pretty expected - many type baselines got return types when calls had errors, many error baselines got another entry or two as a result of downstream errors exposed by this.
|
Note: breaks jest types in the following call: const spy3Mock = spy3
.mockImplementation(() => '')
.mockImplementation()
// $ExpectError
.mockImplementation((arg: {}) => arg)
.mockImplementation((...args: string[]) => args.join(''))
.mockImplementationOnce(() => '')
.mockName('name')
.mockReturnThis()
.mockReturnValue('value')
.mockReturnValueOnce('value')
.mockResolvedValue('value')
.mockResolvedValueOnce('value')
.mockRejectedValue('value')
.mockRejectedValueOnce('value');The last four calls expect an argument of type |
|
Oh, the tests were ALWAYS an error, it's just the we now report downstream errors after the expected error on The test should be making those calls on a mock of a promise, so that's probably the right fix. |
|
@sandersn Somehow this has caused some concerning regressions in The immutable case is something like //@filename: Traversable.ts
export interface ITraversable<T> {}
export function isTraversable(a: any): boolean {
return isList(a) || isOption(a);
}
export function isList(a: any) {
return (a instanceof _list.Cons) || (a instanceof _list.Nil);
}
export function isOption(a: any) {
return (a instanceof _option.Some) || (a instanceof _option.None);
}
// @filename: List.ts
import _tr = require('./Traversable');
export class Cons<T> extends IList<T> {
flatten<U>(): IList<U> {
return this.foldLeft<IList<U>>(new Nil<U>(), (acc, t) => {
if(_tr.isList(t)) { // usage here
var l = <IList<U>><any> t;
return acc.append(l);
} else if(_tr.isOption(t)) { // and here
var o = <_option.IOption<U>><any> t;
if(o.isDefined()) {
return acc.appendOne(o.get());
} else return acc;
} else {
return acc.appendOne(<U><any> t);
}
});
}
}I think we're failing to check a child function body or something? |
|
Probably when there is an error in the parent call expression. I observed this in the user tests too. |
|
Makes sense that we wouldn't have noticed this since our test coverage of the language service is so much worse, and getCandidateForOverloadFailure was only called from there. |
|
After discussion with @weswigham, the fix is to make getCandidateForOverloadFailure call resolveUntypedCall the way resolveErrorCall does. resolveUntypedCall is just a barebones set of |
Redo #28564
I haven't looked at the baseline changes; maybe they're horrible.