Skip to content
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

Can't assign type though it is on both branches of conditional type #51488

Closed
wbt opened this issue Nov 11, 2022 · 12 comments
Closed

Can't assign type though it is on both branches of conditional type #51488

wbt opened this issue Nov 11, 2022 · 12 comments
Labels
Duplicate An existing issue was already created

Comments

@wbt
Copy link

wbt commented Nov 11, 2022

Bug Report

🔎 Search Terms

both sides conditional type assignable

🕗 Version & Regression Information

  • This changed for the better between versions 4.2.3 and 4.3.5 and back between 4.4.4 and 4.5.5
  • Nightly version at time of testing: v5.0.0-dev20221111

⏯ Playground Link

Playground link with relevant code

💻 Code

const demo = function<B extends boolean>(
	myBool: B,
	definedValue?: string
) {
	//Error ts(2322): Type 'undefined' is not assignable to type
	//'B extends false ? undefined : string | undefined'.
	let keyValue : (B extends false ? undefined : (string | undefined)) = undefined;
	if(myBool) {
		keyValue = definedValue;
	}
}

const tinyDemo = function<B extends boolean>(myBool: B) {
	//Error ts(2322): Type 'number' is not assignable to type 'B extends true ? number : number'.
	let myVar : (B extends true ? number : number) = 5;
}

🙁 Actual behavior

Errors as noted, plus one on the conditional assignment in the first example.

🙂 Expected behavior

If a variable has a conditional type but type T is on both sides of the conditional, I should be able to assign type T to that variable regardless of which branch of the conditional applies.

Cross-links

#30639 is an older PR which claims to fix an issue that sounds a lot like this, and that was merged 3/11/21 - it looks like that was for 4.3 which changed things for the better. Restoring that fix might be a step in the right direction. (There is still an error in the conditional assignment, where the conditional type should be narrowable to allow that assignment, but that might be separable as a different issue.)

#46429 is another PR merged over a year ago, just ahead of version 4.5. The issue reappeared somewhere in the 4.5 range, so it's possible that this PR was the cause of the regression. The issue still exists in the latest version.

In some cases, where the types on each side of the conditional are complex constructed types, it might be harder to figure out if T is in fact assignable to both sides of the conditional. However, these examples use primitives, so that's clearly not the source of the issue.

@fatcerberus
Copy link

fatcerberus commented Nov 11, 2022

Reading through the linked issues, it appears that most of the logic added in #30639 was deliberately reverted in #46429 due to problems with performance/excessive type instantiations.

@fatcerberus
Copy link

fatcerberus commented Nov 11, 2022

Actually it looks like this is fully intentional and documented:

https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#restrictions-on-assignability-to-conditional-types

TypeScript no longer allows types to be assignable to [deferred] conditional types that use infer, or that are distributive. Doing so previously often ended up causing major performance issues. For more information, see the specific change on GitHub.

B extends false ? undefined : (string | undefined) is distributive because it operates on a naked type parameter B.

@wbt
Copy link
Author

wbt commented Nov 11, 2022

The top of #46429 states:

With this PR, a source type S is assignable to a target type A extends B ? C : D when

  • The conditional type is not distributive, or is distributive but A is never referenced in C or D, and
  • B has no infer positions, and
  • S is assignable to C (except if A never assignable to B for any instantiation), and
  • S is assignable to D (except if A is always assignable to B for any instantiation).

All four of those conditions appear to be met in the examples. A is never referenced in C or D since C and D are primitive, B (from that PR) has no infer keyword (being the primitive false), and S is assignable to both C and D. Thus, it seems like the intention was to have this working even after that PR.

@wbt
Copy link
Author

wbt commented Nov 11, 2022

(and for whatever it's worth, I am definitely not in favor of the TS practice of rolling out breaking changes on semver-feature version number bumps.)

@fatcerberus
Copy link

(and for whatever it's worth, I am definitely not in favor of the TS practice of rolling out breaking changes on semver-feature version number bumps.)

TS doesn’t follow semver.

@wbt
Copy link
Author

wbt commented Nov 11, 2022

TS doesn’t follow semver.

It clearly does not, but it just looks like it does, leading people to be confused.
Documenting the breaking change with a pointer to more information led by a description suggesting the use case at hand should still work, if the intent was for that not to be the case, leads to more confusion!

Otherwise, it seems to me that the intent was to have this working even after that PR, and if the execution doesn't match then that's a bug which should be fixed.

@MartinJohns
Copy link
Contributor

but it just looks like it does, leading people to be confused.

Where does it look like that?

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Nov 11, 2022
@wbt
Copy link
Author

wbt commented Nov 11, 2022

Where does it look like that?

The TS version number of three dot-separated numeric components, in a context surrounded by dependencies and repositories that do (at least claim to) use semver, combined with the overlap in the reasons why you would use semver/TypeScript, lead folks who have not invested tons of time reading the obscure parts of TS documentation to think that TypeScript likely uses semver. It is an incorrect conclusion, but a common one, and leads to folks unintentionally making breaking-change upgrades more often than they otherwise would.

Which Issue is this one tagged as a duplicate of? Tagging it as a duplicate of an already-closed-as-fixed issue, where the fix was undone by a subsequent PR that stated an intention to have something like this still work, would be a pretty disappointing outcome of ensuring it's never fixed.

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@wbt
Copy link
Author

wbt commented Nov 14, 2022

It's still not clear what this is a duplicate of.

@RyanCavanaugh
Copy link
Member

Which Issue is this one tagged as a duplicate of? Tagging it as a duplicate of an already-closed-as-fixed issue, where the fix was undone by a subsequent PR that stated an intention to have something like this still work, would be a pretty disappointing outcome of ensuring it's never fixed.

What are we supposed to do with issues that don't seem to be fixable? Leaving them open forever means that the open issue count grows without bound, commingling impossible work with approachable work and confusing the status of what is available to be done. The process is here to manage our workload and track things that need doing, not to be an ever-growing list of sources of incompleteness in the type system, of which there will always be an infinite number of.

@wbt
Copy link
Author

wbt commented Nov 19, 2022

The issue was fixed before and I think it can be fixed again, and the regression does not appear to be intentional based on the description in the PR which leads one to believe this case should still work after that PR. I'd categorize this as a fixable regression bug introduced by the second PR and put it on the backlog for a change that can lead to it working again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants