Skip to content

Undo 'any' inference propagation#22736

Merged
sandersn merged 3 commits into
masterfrom
undo-any-inference-propagation
Mar 21, 2018
Merged

Undo 'any' inference propagation#22736
sandersn merged 3 commits into
masterfrom
undo-any-inference-propagation

Conversation

@sandersn
Copy link
Copy Markdown
Member

Fixes #22362

#22197 introduced a change to inference where any as the source caused an inference from any to be added for all type parameters in the target. It's not clear that this "deep-any" behaviour is correct or useful.

Specifically, this change breaks inference in JQuery while changing only one other test; an inferred property deep inside mappedTypeRecursiveInference. There, when inferring from XMLHttpRequest to PartialDeep, one property has type any which would otherwise have type {}.

In this PR, I just undid the change and added a test that @Andy-MS simplified from the real jquery.d.ts on DefinitelyTyped.

Removing this only changes one test slightly, and fixes JQuery types,
which rely on the old method of inference.
@sandersn
Copy link
Copy Markdown
Member Author

Note that inference pre-#22197 was not right; for p1: MyPromise<number, U>, we would always infer p2: MyPromise<number, {}> for all U. But at least we didn't infer p2: MyPromise<any, any> when starting with p1: MyPromise<number, any>.

@sandersn sandersn requested a review from ahejlsberg March 20, 2018 21:38
Copy link
Copy Markdown
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The any propagation code was added primarily to propagate wildcards (one of our sentinel any types). It appears that other changes in the conditional type logic has shrouded tests that previously depended on it, but I wouldn't want to completely get rid of it. I suggest you change to only propagate wildcards.

Comment thread src/compiler/checker.ts
if (!couldContainTypeVariables(target)) {
return;
}
if (source.flags & TypeFlags.Any) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to source === wildcardType and keep everything else unchanged.

@sandersn
Copy link
Copy Markdown
Member Author

Sounds good. From discussion with @weswigham, it sounds like we need to revisit this after 2.8 to handle variance better.

@sandersn sandersn merged commit c930895 into master Mar 21, 2018
sandersn added a commit that referenced this pull request Mar 21, 2018
* Undo 'any' inference propagation

Removing this only changes one test slightly, and fixes JQuery types,
which rely on the old method of inference.

* Add jquery regression test and update baselines

* Restore any inference propagation to wildcard only
@sandersn
Copy link
Copy Markdown
Member Author

This is now in 2.8 as well.

@sandersn sandersn deleted the undo-any-inference-propagation branch March 21, 2018 18:17
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants