Prefer higher-priority inferences from generic signatures over low-priority inferences from the first pass #56939
Conversation
typescript-bot
added
the
For Uncommitted Bug
| target[i] = source[i]; | ||
| target[i].priority = InferencePriority.None; |
There was a problem hiding this comment.
TODO for myself:
this line might not be needed at all or the assigned priority should be the source priority without InferencePriority.ReturnType (the latter sounds like a good idea if those inferences should stay "upgradable"/mergeable). IIUC, no new inferences without InferencePriority.ReturnType can be made here anyway - all of those were already collected before the ones from generic signatures start to be included.
There was a problem hiding this comment.
Hm, yeah, None definitely doesn't feel correct, since None is actually the highest priority inference possible - and I don't think simply adopting source's inference for target when they overlap should guarantee this is the best inference possible across all potential inference sites.
But also - this is mutating the inference object, which you should avoid, since it means if that inference is reused in multiple contexts, you'll be mangling the priority in all of them (...we probably shouldn't be reusing an inference in multiple contexts, but there are probably some edge cases we decided it was OK in that this might break the invariants of). If we can't reuse the source inference wholesale (because its priority already doesn't contain ReturnType), then we should duplicate the source inference object with a new priority.
There was a problem hiding this comment.
I initially picked None here because it's the most conservative thing to do. If I'm reasoning about it correctly. Right now (on main) we iterate over those generic signatures (conceptually at least):
- if the inferences are overlapping then we don't try to use any of the new ones
- if the inferences are not overlapping then we use the new ones but that means that when we overlap again we won't pick any new ones
So my conservative change adjusts this only slightly - it only allows a single "upgrade" when inferences are overlapping (and in such a case, we only update the overlapping one, this is important - we can't assign inferences that are not overlapping).
I thought that this, both, is just the smallest change that could be done here if this direction is promising at all and that there might be an argument to be made here that in this scenario the inferences made from earlier generic signatures should be preferred (left ones having the higher priority over the right ones). The latter doesn't match how "regular" inferences are gathered but it is how it implicitly works on main (when overlap happens the new inferences are ignored after all, so they can't be replaced/updated).
I could certainly experiment with relaxing this behavior and allow those upgrades continuously.
I pushed out a change using cloneInferenceInfo but I'm not sure if you meant that or cloneInferenceContext. And perhaps it should also be applied conditionally? but then that would imply that cloneInferenceInfo is the right choice here since ReturnType is relevant to the info and not to the whole context.
If we can't reuse the source inference wholesale (because its priority already doesn't contain ReturnType)
Does it mean that when the inference info of the source contains ReturnType then it's OK to reuse it and avoid cloning?
There was a problem hiding this comment.
Does it mean that when the inference info of the source contains ReturnType then it's OK to reuse it and avoid cloning?
If you're not going to mutate it, you can reuse it. Probably.
I pushed out a change using cloneInferenceInfo
Yeah, that's the one.
I could certainly experiment with relaxing this behavior and allow those upgrades continuously.
Yeah, I just don't think an inference site like this is the end-all inference source within a signature context (eg, a direct inference w/o return type inference or any other priority modifiers) - there may be a better one elsewhere, and for that one to be preferred, this priority needs to not be None.
This is spitballing, but it may be better to think of this, rather than as "merging" the inference infos, instead as using information from the contextual signature inference process to make new inferences in the original context - and that inference should have an appropriate priority that might not be the highest available. Honestly, in that regard, all this cloning may be a bit misleading - if we're allowing overlapping inference results in the two lists of inferences, mergeInferences is only used here, and it may not be the clearest approach. It may be better to instead create a new inference context to do these return type/contextual parameter inferences within, and then do a inferTypes(context.inferences, getInferredType(newContext, i), context.inferences[i].typeParameter, newContext.inferences[i].priority & ~InferencePriority.ReturnType) for each type parameter at index i, which should better handle merging inferences of differing priorities together, without needing bespoke or duplicated priority merging logic. I dunno though, there may be more complexity with this approach.
There was a problem hiding this comment.
I need to chew on this for a bit. As an additional data point - I think this issue is quite related to this code and perhaps could help guiding us to the best solution: #57072 (comment) .
One extra problem with allowing further inferences here is that context.inferredTypeParameters would get polluted more and more with the type parameters that might not get used or something. I still don't quite understand all of the implications of this algorithm though so maybe I'm missing something. I already have seen this happening today when experimenting with some stuff:
declare function compose<A, B, C>(f: (a: A) => B, g: (b: B) => C): (a: A) => C
declare function f<A, B>(a: A): B
declare function g<B, C>(a: B): C
const result = compose(f, g)
// ^? const result: <A, B>(a: A) => unknownSince those inferredTypeParameters are simply concatenated there is no good way of even rejecting the unused ones~ at some point.
There was a problem hiding this comment.
Since those inferredTypeParameters are simply concatenated there is no good way of even rejecting the unused ones~ at some point.
Yeah, at some point a filtering of that list to remove type parameters that definitely aren't used and logic for figuring out if the type parameter was actually used is probably needed for usability. It'll be a bit of a challenge, though, with all the deferred inferences we can make "maybe it's used later 🤷♂️" is probably a somewhat common case.
…iority inferences from the first pass
Andarist
force-pushed
the
fix/generic-signature-inference-over-low-priority
branch
from
c50d57b to
31d0e8b
Compare
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the parallelized Definitely Typed test suite on this PR at 31d0e8b. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite (tsserver) on this PR at 31d0e8b. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite on this PR at 31d0e8b. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the regular perf test suite on this PR at 31d0e8b. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite (tsserver) on this PR at 31d0e8b. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite on this PR at 31d0e8b. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at 31d0e8b. You can monitor the build here. |
|
Hey @RyanCavanaugh, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@RyanCavanaugh Here are the results of running the user test suite comparing Everything looks good! |
|
@RyanCavanaugh Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@RyanCavanaugh Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @RyanCavanaugh, the results of running the DT tests are ready. |
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Everything looks good! |
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. DetailsServer exited prematurely with code unknown and signal SIGABRTAffected reposbackstage/backstageRaw error text:RepoResults8/backstage.backstage.rawError.txt in the artifact folder
Last few requests{"seq":3379,"type":"request","command":"definitionAndBoundSpan","arguments":{"file":"@PROJECT_ROOT@/packages/integration/src/azure/AzureUrl.ts","line":41,"offset":35}}
{"seq":3380,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/packages/integration/src/azure/AzureUrl.ts","line":46,"offset":18,"includeExternalModuleExports":false,"triggerKind":1}}
{"seq":3381,"type":"request","command":"completionEntryDetails","arguments":{"file":"@PROJECT_ROOT@/packages/integration/src/azure/AzureUrl.ts","line":46,"offset":18,"entryNames":["arguments"]}}
{"seq":3382,"type":"request","command":"references","arguments":{"file":"@PROJECT_ROOT@/packages/integration/src/azure/AzureUrl.ts","line":75,"offset":14}}
Repro steps
|
| target[i] = source[i]; | ||
| target[i].priority = InferencePriority.None; |
There was a problem hiding this comment.
Hm, yeah, None definitely doesn't feel correct, since None is actually the highest priority inference possible - and I don't think simply adopting source's inference for target when they overlap should guarantee this is the best inference possible across all potential inference sites.
But also - this is mutating the inference object, which you should avoid, since it means if that inference is reused in multiple contexts, you'll be mangling the priority in all of them (...we probably shouldn't be reusing an inference in multiple contexts, but there are probably some edge cases we decided it was OK in that this might break the invariants of). If we can't reuse the source inference wholesale (because its priority already doesn't contain ReturnType), then we should duplicate the source inference object with a new priority.
typescript-bot
added
For Backlog Bug
🎶 Everything you know is wrong |
fixes #56931
At the moment, I'm treating this as an experiment and my primary goal is to get some potential feedback from the test suite run (cc @jakebailey). It's not meant to be reviewed yet - unless somebody feels like it 😉 At the very least, I still have to adjust code comments.