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

Union of a nullable and not nullable setter results wrongly into a nullable setter #50142

Closed
MiladSadinam opened this issue Aug 2, 2022 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@MiladSadinam
Copy link

Bug Report

πŸ”Ž Search Terms

setter, union, wrong type

πŸ•— Version & Regression Information

4.7.4
Also checked 3.9.7 with same results.

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about setter___

⏯ Playground Link

https://www.typescriptlang.org/play?#code/MYGwhgzhAEByCuJwCMQFNoG8BQBIADvKgJbDQRoAu0AZgPZ0AUA+gHZoDuAamCPBgC5oreAFtkaAE7QAPsMQgAlNCx4Avtg3ZQkGLDqUESMKgw4CREKXJVaDFu269+0ISPFTlq3Bq3A6rBDUIsam0AC8wpxwCibojIoA3NoBQcIGRijoEVEccBmxpgnJ2AD0pdD+kpJowJQgAJ4ANNAcGMBgrKwG0LrEAOas0ABEISDD0JR0I92GhejD2GNxaAB09NORY8mzmSvrDDnb0OXQACoAFsQw15V01bX1Db13ovjE2VKS96vYZRUAdQuaCG8AgxFY-Re8FYxAC0DoNGgyAMF0mDXwaBgAElWnREAATXpQMQYSgXMDUBr48iUD4gSqdWbE8GDGYKCYSDpgtD-SbA6AwuFDCkwKa0BQ0en89qpSiSMAQmCI5Go9GYiCraAAIXglBawJq0Gp8EZQz6bNGHN+NBhdWFNkkADdSGgALJUC50AmMMCuGKhbJyfRzQNoZRCJ10YhE8xgA6beRIRInCqXW63Zl+-xvD4YL73FrIPXQXHkiEAa2gxGoEC9hORa00QA

πŸ’» Code

class Nullable {
	public set foo(_newValue : number | null)  {
	}
}

class NotNullable {
	public set foo(_newValue : number)  {
	}
}

const nullable = new Nullable();
const notNullable = new NotNullable();

// correctly, we cannot assign "null" to "notNullable"
nullable.foo = null;
notNullable.foo = null; // This is correctly a compile error.

// When using a union of both types I would assume that you still cannot assign "null" because
// the union has to fullfill the constrains of both types. But, here you can assign "null".
function serviceMethod(a : Nullable | NotNullable) : void {
	a.foo = null; // This is not a compile error, but I think it should be.
}

πŸ™ Actual behavior

When using a union of two types with a setter where one is nullable and another one is not, the resulting setter is also nullable.

πŸ™‚ Expected behavior

I believe this is wrong, as the union must ensure that the constraints of both types are fulfilled. Which mean that null should be prohibited as one of the types does not allow it.

@MartinJohns
Copy link
Contributor

Related: #45376

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Aug 2, 2022
@RyanCavanaugh
Copy link
Member

Properties are always measured covariantly, and this is unsound under supertype aliasing.

@fatcerberus
Copy link

fatcerberus commented Aug 2, 2022

Properties are always measured covariantly

I'm kind of surprised this isn't caught under the rules of #30769. Even a['foo'] = null is accepted. Probably subtype reduction kicking in.

@MiladSadinam
Copy link
Author

MiladSadinam commented Aug 2, 2022

Properties are always measured covariantly, and this is unsound under supertype aliasing.

I am not sure if I understand this completely. If this is unsound, I cannot follow why it should be the expected behavior. If there is somewhere a more detailed reading about the reasoning, I would be glad to get a pointing.

@fatcerberus
Copy link

fatcerberus commented Aug 2, 2022

For the same reason that { foo: number } is assignable to { foo: number | undefined } and Cat[] is assignable to Animal[]: it's unsound, but a ton of real-world code would be broken if it weren't allowed. The unsoundness in your case is a consequence of subtype reduction: Nullable | NonNullable is treated as a Nullable because the former is considered a supertype of the latter.

@MiladSadinam
Copy link
Author

Thank you for the explanation. I can follow the reasoning.

Though, concerning the backwards compatibility: Typescript has introduced many optional "strict" flags to solve this problem.

@Jamesernator
Copy link

N

@typescript-bot
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants