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

Uninitialized class properties with type undefined are allowed, breaking type expectations for class instances. #55195

Closed
RobertSandiford opened this issue Jul 29, 2023 · 12 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@RobertSandiford
Copy link

Bug Report

πŸ”Ž Search Terms

class properties with type undefined properties produce objects that don't match inferred type

πŸ•— Version & Regression Information

Present between 3.3 and 5.2.0 nightly.

⏯ Playground Link

Playground

πŸ’» Code

type O = {
  a: undefined
}

class C { // new C will produce an empty object, because a is not initialized
  a: undefined
}

const c = new C
console.log(c) // c is { }

type TOC = keyof typeof c // "a" - typescript thinks 'c' has a property 'a'

const o: O = c // allowed to assign to type O


function f(o: O) {
  if ('a' in o) {
    console.log('a exists')
  } else {
    const never: never = o
    console.log('This never happens')
  }
}

f(o)

πŸ™ Actual behavior

  1. new C produces an empty object c
  2. TS thinks that c has a property a, based on the class definition
  3. Assignment to o: O is allowed
  4. in function f, TS identifies that the else branch doesn't occur
  5. running f(o) or f(c) results in the else branch running

Output

[LOG]: C: {} 
[LOG]: "This never happens" 

πŸ™‚ Expected behavior

-strict mode was on

  1. strictPropertyInitialization check should catch that a is not initialized, and throw an error

Note: If useDefineForClassFields is true, which occures by default for target >= ES2022, JS is emitted which initializes the property, and there is no problem. Bug exists only for cases of useDefineForClassFields: false (includes the default TS config)

@RobertSandiford RobertSandiford changed the title Class Uninitialized class properties with type undefined are allowed, breaking type expectations for class instances. Jul 29, 2023
@MartinJohns
Copy link
Contributor

MartinJohns commented Jul 29, 2023

Duplicate of #28823 / #12437. As per #12437 the suggested solution is to use useDefineForClassFields.

Also be aware that the in operator has several unsound behaviours that are working as intended. This is one of them (pre useDefineForClassFields). I always strongly advise against using it, it leads to too many problems in a structurally typed language like TypeScript.

@RobertSandiford
Copy link
Author

RobertSandiford commented Jul 29, 2023

I read #12437. It's related, and talks about TS matching the JS spec, but does not report this bug. #28823 duplicates #12437.

There's no reason that TS with default configs should have a bug, and people should have to enable a certain flag to escape it.

@MartinJohns
Copy link
Contributor

MartinJohns commented Jul 29, 2023

That is working as intended. As per #20075:

Strict property initialization checking verifies that each instance property declared in a class either (a) has a type that includes undefined or (b) has an explicit initializer or an assignment to the property in the constructor.

It's not a bug. You're free to disagree of course, but the position of the team is quite clear on this. New features have always been "hidden" behind flags to keep backwards compatibility, even starting with something like strictNullChecks.

Be also aware that TypeScript intentionally does not have a sound type system. There is plenty of unsound type behaviour that is intentional.

https://github.com/Microsoft/TypeScript/wiki/TypeScript-Design-Goals#non-goals

Apply a sound or "provably correct" type system. Instead, strike a balance between correctness and productivity.

@RobertSandiford
Copy link
Author

RobertSandiford commented Jul 29, 2023

That is working as intended. As per #20075:

Strict property initialization checking verifies that each instance property declared in a class either (a) has a type that includes undefined or (b) has an explicit initializer or an assignment to the property in the constructor.

It's not a bug. You're free to disagree of course, but the position of the team is quite clear on this. New features have always been "hidden" behind flags to keep backwards compatibility, even starting with something like strictNullChecks.

Be also aware that TypeScript intentionally does not have a sound type system. There is plenty of unsound type behaviour that is intentional.

https://github.com/Microsoft/TypeScript/wiki/TypeScript-Design-Goals#non-goals

Apply a sound or "provably correct" type system. Instead, strike a balance between correctness and productivity.

I would call that a bug - the type system is giving it a type that it doesn't conform to. The type checker is then telling us things that are not true, based on that typing.

Thanks for the link to #20075, I see that the implementation was by design - but there was no acknowledgement of the issue, so I'm not sure we can say that the bug is by design from that thread.

Apply a sound or "provably correct" type system. Instead, strike a balance between correctness and productivity.

I would hope that the meaning of this, is that developers are allowed to do things like use any and do unsafe typecasts. That's not the same as having the typechecker mistype objects and make false assertions without any developer input. That's not an enhancement to productivity, it's just a trap.

Without useDefineForClassFields this class:

class C { // new C will produce an empty object, because a is not initialized
  a: undefined
  b: boolean | undefined
}

produces objects of type { a?: undefined, b?: boolean }, so I would hope that the TS team agrees that that is what the type checker should infer (dev time permitting), if strictPropertyInitialization is not meant to stop it.

I don't know that the "back compatibility" argument applies when we are talking about TS mistyping objects. I would think bugs should be fixed, in order to alert users to possible bugs in their app, caused by TS mis-typing.

@MartinJohns
Copy link
Contributor

MartinJohns commented Jul 29, 2023

Thanks for the link to #20075, I see that the implementation was by design - but there was no acknowledgement of the issue, so I'm not sure we can say that the bug is by design from that thread.

Here's the acknowledgement: #52259

I would hope that the meaning of this, is that developers are allowed to do things like use any and do unsafe typecasts.

No, it does not mean that. It means there are several aspects of the type system that are intentional wrong.

Here's another example that is intentional (and please don't open an issue about this..):

class Foo { a: string = 'abc' }

const f: Foo = { a: 'abc '}
if (f instanceof Foo) {
  console.log('true');
} else {
  // type is never here, but this branch will execute.
  console.log('false')
}

Related: #55113 (comment)

@RobertSandiford
Copy link
Author

Here's the acknowledgement: #52259

But it still doesn't acknowledge that the objects are mis-typed : )

No, it does not mean that. It means there are several aspects of the type system that are intentional wrong.

Here's another example that is intentional (and please don't open an issue about this..):

class Foo { a: string = 'abc' }

const f: Foo = { a: 'abc '}
if (f instanceof Foo) {
  console.log('true');
} else {
  // type is never here, but this branch will execute.
  console.log('false')
}

But I think again this is a productivity shortcut.

Do you think that the behaviour in this ticket is a productivity shortcut? What is the productivity benefit that justifies type incorrectness here?

@MartinJohns
Copy link
Contributor

But it still doesn't acknowledge that the objects are mis-typed : )

It acknowledges that this is working as intended. Do you want for every issue a wording that matches your preconception?

Anyway, I'm out, this is pointless.

@RobertSandiford
Copy link
Author

Here's another example that is intentional (and please don't open an issue about this..):

By the way do you have a link to where this was discussed? Interested to read about the reasoning.

@fatcerberus
Copy link

fatcerberus commented Jul 29, 2023

Re: Intentional unsoundness - #9825, and in particular #9825 (comment)
Straight from the horse's mouth, so to speak.

@RobertSandiford
Copy link
Author

Re: Intentional unsoundness - #9825, and in particular #9825 (comment) Straight from the horse's mouth, so to speak.

Thanks. Should probably add the 3rd factor to the design doc though.

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Jul 31, 2023
@RyanCavanaugh
Copy link
Member

I don't have anything to add to the above comments. in narrowing isn't sound but you're allowed to do it; I recommend not doing it πŸ˜‰

@typescript-bot
Copy link
Collaborator

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

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

5 participants