microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.17k stars 12.38k forks source link

ESNext emits optional class members #47350

Open jods4 opened 2 years ago

jods4 commented 2 years ago

Bug Report

πŸ”Ž Search Terms

optional class members

πŸ•— Version & Regression Information

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

// Consider this class
class C { 
  x?: number; 
}

πŸ™ Actual behavior

// This is the emitted JS if you target ESNext and have --useDefineForClassFields
class C {
  x;
}

x is emitted as a class member, even though it was optional. This feels wrong, but to make a stronger point consider --exactOptionalPropertyTypes. With this compiler option, we should have the typing invariant !("x" in c) || typeof c.x == "number"; but with this emit, a new instance has the opposite "x" in c && typeof c.x == "undefined".

πŸ™‚ Expected behavior

Optional members should probably not be emitted:

class C { }

I am not sure / have not thought about what the implications with respect to useDefineForClassFields semantics are.

DanielRosenwasser commented 2 years ago

I believe that this is intended. If you want to signal that this was truly not present (i.e. "this thing will potentially not be set"), then you'll need to use the declare modifier.

class C { 
  declare x?: number; 
}
jods4 commented 2 years ago

@DanielRosenwasser I think that's a good enough workaround in practice.

That said, it means x?: number (without declare) is actually an impossible contract with the compiler options mentionned in issue. Should it be an error or at least warning?

DanielRosenwasser commented 2 years ago

Sounds like this is possibly a bug in --exactOptionalPropertyTypes.

DanielRosenwasser commented 2 years ago

Specifically, under both --strictPropertyInitialization and --exactOptionalProperties, I think either we must require that x is initialized or given | undefined in its annotation, or we avoid narrowing obj.x to number.

class C {
  x?: number;
  y = 3;
}

function foo(obj: C) {
  if ("x" in obj) {
    obj.x.valueOf();
  }
}
fatcerberus commented 2 years ago

Isn’t this for consistency w.r.t. [[Define]] vs. [[Set]] semantics for fields? [[Define]] semantics require the field to be declared in the class body, or defineProperty’d in the constructor. Otherwise you get [[Set]] semantics, which can, e.g. call parent class setters.

It’d be confusing, IMO, to have some properties using [[Define]] and some using [[Set]] depending on whether they were optional or not.

DanielRosenwasser commented 2 years ago

It's more about the fact that property declarations unconditionally add a property even if there's no initializer. If you want the type system to correctly encode whether something is possibly-missing vs. possibly-undefined, then --strictPropertyInitialization is slightly diverged in its expectations from --exactOptionalProperties.

fatcerberus commented 2 years ago

Right, but my point was that if foo?: string; doesn’t emit the property because it’s optional and uninitialized (as suggested in the OP), then that also changes the runtime semantics for that property ([[Set]] vs. [[Define]]), which would be confusing. useDefineForClassFields and exactOptionalPropertyTypes are thus fundamentally at odds with each other.

pushkine commented 2 years ago

The discussion is so cluttered for such a clear cut bug 😭

class A { foo?: 0; }

// target: "ES2020" | new A() is { foo?: 0 } βœ…
class A {}

// target: "ESNext" | new A() is { foo: 0 | undefined } ❌
class A { foo; }
darlanalves commented 2 years ago

Bumped into this one today.

Extending @pushkine example, if I have a decorator to define a property with a getter, that won't work in esnext target.

Example:

class Foo {}
class C {
  @dependency() foo: Foo;
}

function dependency() {
  return function(target, property) {
    Object.defineProperty(target, property, {
        get() {
          return new Foo();
        }
    });
  }
}

// target: "ES2020" βœ…
class C {}
new C().foo instanceof Foo === true;

// target: "ESNext" ❌
//foo is `undefined` because it is a property in every instance of C, overriding the getter from C.prototype
class C { foo; }
new C().foo instanceof Foo === false;