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

Object spread and enumerable properties #13148

Closed
Conaclos opened this issue Dec 23, 2016 · 8 comments · Fixed by #13365
Closed

Object spread and enumerable properties #13148

Conaclos opened this issue Dec 23, 2016 · 8 comments · Fixed by #13365
Assignees
Labels
Fixed A PR has been merged for this issue In Discussion Not yet reached consensus

Comments

@Conaclos
Copy link

Conaclos commented Dec 23, 2016

Hi :)

EDIT:
As noted by @aluanhaddad, the bug is related to the combination of the shorthand syntax in a literal object and the object spread operator on this object.

TypeScript Version: 2.1.4

Code

First code (compilation success):

interface I <T> {
    f (): void
    g (): void
}
const a = {
    f <T> (this: I<T>): void {}
}
const b: I<any> = Object.assign({
    g (): void {}
}, a)

Replacement for last 3 lines (compilation failing):

const b: I<any> = {
    ...a,
    g (): void {}
}

Expected behavior:

Replace Object.assign with the object spread operator should not be harmful.

Actual behavior:

The first code makes happy the compiler.
However if I I replace Object.assign with the object spread operator (See replacement code), the compiler raises the next error:

error TS2322: Type '{ g(): void; }' is not assignable to type 'I<any>'.
  Property 'f' is missing in type '{ g(): void; }'.
@aluanhaddad
Copy link
Contributor

aluanhaddad commented Dec 25, 2016

This is unrelated to generics. It has to do with the difference between properties and methods.
It is also a very subtle behavior.
The issue here is that the value

{
  f() {}
}

is transpiled into the value

{
  f: function() {}
}

which correctly, as per the ECMAScript specification, has an enumerable own property f whose value is a function.

However, consider this expression which is created via a class

new class {
  f() {}
}()

this doesn't have an enumerable own property f. Because f is placed on the prototype and not the object itself, it will be ignored by the spread operator. The ... operator correctly ignores what it sees as methods but the problem is that it is not recognizing that the object literal has a property defined using method syntax.
That said, I think this is a bug because the type of the object literal does have an enumerable own property f but the compiler is behaving as if the concise method notation translates to a method definition when it in fact translates to a property definition.
Changing this would probably break a lot of builds...

@Conaclos Conaclos changed the title Object spread and generic functions Object spread and enumerable properties Dec 26, 2016
@mhegazy mhegazy modified the milestones: sand, TypeScript 2.2 Dec 26, 2016
@mhegazy mhegazy added the In Discussion Not yet reached consensus label Dec 26, 2016
@tomdale
Copy link

tomdale commented Jan 2, 2017

I'm hitting this when trying to use object spread to initialize default values.

This works:

interface RunOptions {
  log: (str: string) => void;
}

const DEFAULT_OPTIONS: RunOptions = {
  log: function(str: string): void {
    console.log(str);
  }
};

function run(command: string, opts: Partial<RunOptions> = {}) {
  let options: RunOptions = { ...DEFAULT_OPTIONS, ...opts }
}

This doesn't, even though it's semantically identical (the only difference is changing function properties to use concise method syntax):

interface RunOptions {
  log(str: string): void;
}

const DEFAULT_OPTIONS: RunOptions = {
  log(str: string): void {
    debug(str);
    logOnFailure(str);
  }
}

function run(command: string, opts: Partial<RunOptions> = {}) {
  let options: RunOptions = { ...DEFAULT_OPTIONS, ...opts }
}

It seems important for POJOs to be treated as a "data bag," and concise methods in this context should be treated as an enumerable property that happens to contain a function.

@wycats
Copy link

wycats commented Jan 2, 2017

fwiw, we intentionally made { foo() { } } semantically identical to { foo: function() { } } (enumerable own property in both cases), even though methods are non-enumerable in classes.

As @tomdale says, object literals are intended to be data bags, with concise methods serving as shorthands, while classes have a clear data/behavior split, which is reflected through enumerability.

@aluanhaddad
Copy link
Contributor

@wycats thank you! I remember having read something to this effect at one point and I tried to find it and link it to this issue but I couldn't find it again. It's great to have the intent and reasoning behind the runtime behavior clarified.

@sandersn
Copy link
Member

sandersn commented Jan 3, 2017

I had a lot of discussions about this when working on the original proposal at #10727 that got condensed to a couple of comments there. I'll try to pull them together in one place here.

First, a summary based on @Conaclos' example:

  1. Object.assign's type is wrong right now (T & U). It will change to match the spread expression's type in 2.2 ({ ...T, ...U }).
  2. The compiler doesn't track own-ness or enumerability. So we decided to fake it: methods are non-own-enumerable unless they are specified in the object literal that contains the spread.

@Conaclos' example just exposes the boundary where the compiler decides that "this method is not owned". We could extend the fakery and treat methods declared in any object literal as own-enumerable. It would take a little more work but not much.

However, @tomdale's example exposes the place where fakery will never get us anywhere.
The underlying reason that this bug appears is that the compiler tracks neither own nor enumerable. So it can't tell the difference between an own-enumerable property like { f() { } } from @Conaclos' example and a non-own-enumerable new class { f() { } } from @aluanhaddad's example. (I'm ignoring the less common cases where own-ness and enumerability are different for this discussion.)

In fact, as @tomdale's example shows, as soon as you decouple types and values, the compiler doesn't have a good way to guess which properties of a type are owned:

interface I {
  a(): void; // own-enumerable?
  b(): void; // or not?
}
class C implements I {
  a = () => { }; // own-enumerable
  b() { }        // non-own-enumerable
}

Currently we use syntax to guess:

interface I {
  a: () => void; // probably own-enumerable
  b(): void;     // probably not
}

This is not great. The best thing you can say about it is that it reflects the syntax you will probably write to create an instance of the type.

By the way, the reason that this is important is that in 2.2 you will be able to write generic spread types like <T>(t: T) => { ...T, ...I }. Given the current semantics, this return type will have a property a but not a property b (unless of course T happens to have a b).

Unfortunately, the only way I can think of to really fix this is to make own-ness part of the language. For example, we could change interfaces to only have own properties, and require everything else to come from some special extends clause:

interface A {
  // only A's own properties here!
  a: number;
}
interface B extends__proto A {
  // only B's own properties here!
  b: (x: string) => void;
}

Or we could add own annotations:

interface B {
  proto a;
  b;
}

Although those should really be stackable so the compiler could track how many levels up the prototype chain it should expect to see the property. For example: proto proto proto proto fourLevelsUp: number.

I don't really think this is a good solution. I actually think fakery and subtle syntax is preferable. If we do start tracking all methods back to their declaration we could have a complex set of heuristics that differ for object literals, classes and interfaces, something like:

const o =  {
  a: () => { } // own-enumerable
  b() { }      // own-enumerable
}
class C {
  a: () => { } // own-enumerable
  b() { }      // not
}
interface I (
  a: () => { } // own-enumerable
  b() { }      // own-enumerable (Note [1])
}

But any rule you make here is (1) guaranteed to be wrong sometimes (2) trivially easy to circumvent. And you have to be able to explain the rules in a reasonable amount of time.

[1] Probably people who write an interface that they expect to be used in a spread will also expect only object literals to be spread, not instances of classes.

@sandersn
Copy link
Member

sandersn commented Jan 3, 2017

@tomdale I don't think I called it out explicitly enough, but in 2.1, you can control own-enumerable in interfaces with a syntactic distinction:

interface I {
  a: () => { } // own-enumerable
  b() { } // not
}

I intended it to work that way so in that sense, it is By Design. But it might be better to assume that people who are writing interfaces for use in a spread context want users to spread only object literals:

interface I {
  a: () => { } // own-enumerable
  b() { }      // own-enumerable
}
const o: I = { a() { }; b() { } };
const s = { ...o }; // still has `a` and `b`.

const c = new class { a() { }; b() { } };
const s2 = { ...c }; // incorrectly has `a` and `b`, but why did you spread a class instance?

And it's more honest because the compiler can't possibly track own-enumerable through an interface, so allowing control of its approximation through a "secret" feature is kind of shady.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 6, 2017

As per the discussion in #13331; properties should be filtered iff they originate from method declarations on a class.

@aluanhaddad
Copy link
Contributor

@sandersn Thank you for the detailed explanation. I was only considering the direct use of an inferred object literal type in a spread expression but you point out how quickly that assumption breaks down - as soon as the types and values are are dissociated. @mhegazy Even if the method originates in a class declaration, the class might be extended by an interface and then implemented through an object literal...

declare class A {
  m(): void;
}
interface B extends A {
  n(): void;
}
const x: B = { m() { }, n() { } };
const y = { ...x };

It comes down to how the value is created, which could be at odds with how it is annotated and seems impossible to track in a meaningful way.

Also, given that it is not unheard of for .d.ts files to be updated
from

interface A {
  m(): string;
}
declare const A: AStatic;
declare interface AStatic {
  new (x: string): A;
}

to

declare class A {
  constructor(x: string);
  m(): string;
}

this could get a lot trickier because what was once a straightforward refactoring becomes a major change.

By the way, it is great to hear that spread types are going to be in the next version!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue In Discussion Not yet reached consensus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants