redo #28564#36665
redo #28564 #36665
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sandersn commented Feb 7, 2020
@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! |
typescript-bot commented Feb 7, 2020
@sandersn Here they are:Comparison Report - master..36665
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sandersn commented Feb 7, 2020
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. |
weswigham commented Feb 7, 2020
Hey, it at least means that the hypothesis of "this should only affect compilations with errors" is probably true~ |
sandersn commented Feb 7, 2020
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. |
sandersn commented Feb 7, 2020
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. |
weswigham left a comment
weswigham left a comment 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.
sandersn commented Feb 10, 2020
Note: breaks jest types in the following call: constspy3Mock=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. |
@sandersnSomehow this has caused some concerning regressions in The immutable case is something like //@filename: Traversable.tsexportinterfaceITraversable<T>{}exportfunctionisTraversable(a: any): boolean{returnisList(a)||isOption(a);}exportfunctionisList(a: any){return(ainstanceof_list.Cons)||(ainstanceof_list.Nil);}exportfunctionisOption(a: any){return(ainstanceof_option.Some)||(ainstanceof_option.None);}// @filename: List.tsimport_tr= require('./Traversable');exportclassCons<T>extendsIList<T>{flatten<U>(): IList<U>{returnthis.foldLeft<IList<U>>(newNil<U>(),(acc,t)=>{if(_tr.isList(t)){// usage herevarl=<IList<U>><any>t;returnacc.append(l);}elseif(_tr.isOption(t)){// and herevaro=<_option.IOption<U>><any>t;if(o.isDefined()){returnacc.appendOne(o.get());}elsereturnacc;}else{returnacc.appendOne(<U><any>t);}});}}I think we're failing to check a child function body or something? |
sandersn commented Feb 11, 2020
Probably when there is an error in the parent call expression. I observed this in the user tests too. |
sandersn commented Feb 11, 2020
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. |
sandersn commented Feb 12, 2020
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.