Add tags for the family of Observable* types to simplify typechecking

[vladima]

Currently shape of Observable class and its derived classes is quite complicated. As a consequence it takes a long time for a TypeScript compiler to determine relations between them based on only its structure. This PR adds a unique tag property for every class, type of property reflects the set of type parameters of the class. Presence of these properties has following effects:

• compiler can quickly determine if type A is not a subtype of B if A does not have tag for B
• in case if A is a subtype of B use types of tags to check relations between type arguments.

I've used this example to measure the actual impact of this change.

# rxjs-5.0.0-beta-2
Files:              60
Lines:           20139
Nodes:           96348
Identifiers:     37452
Symbols:       2052011
Types:          330447
Memory used:   270717K
I/O read:        0.03s
I/O write:       0.02s
Parse time:      0.19s
Bind time:       0.11s
Check time:      4.73s
Emit time:       0.03s
Total time:      5.06s

#rxjs-taggedTypes
Files:             231
Lines:           22169
Nodes:          101982
Identifiers:     39340
Symbols:       6061714
Types:            9638
Memory used:    39202K
I/O read:        0.03s
I/O write:       0.00s
Parse time:      0.31s
Bind time:       0.17s
Check time:      0.48s
Emit time:       0.02s
Total time:      0.98s

Notes:

• tag fields are preserved in .d.ts files but don't have any runtime impact since they are not initialized.
• this pattern is broken

class A {
    f: EventEmitter<number> = new EventEmitter(); // was ok, now error since `{}` is not assignable to `number`
}

fix is fairly trivial

class A {
    f = new EventEmitter<number>(); // or f: EventEmitter<number> = new EventEmitter<number>();
}

Reason: since type parameter appear only in function parameters they were treated as bivariant (A<T1> is assignable toA<T2> if either T1 is assignable to T2 or T2 is assignable to T1 ) which is technically unsafe.

class A<T> {
    f: ( fn: (x: T) => void ) => void;
    set(a: T): A<T> {
        this.f = f => f(a);
        return this;
    }

    run(f) {
        this.f(f)
    }
}

let a: A<number> = new A().set("123");
a.run(x => x.toFixed()); // 0 compiler errors but will fail in runtime since actual type of 'x' is string

Tags keep only covariant step (A<T1> is assignable toA<T2> if T1 is assignable to T2) so now these code will be rejected by the compiler.

[vladima]

// cc @mhegazy

[david-driscoll]

@Blesh and @kwonoj will probably want you to squash to one commit 😄

With this change things like the following will work as expected?

let o: Observable<number> = new Subject<number>();

It's only scenarios where you do not define the input type as part of the constructor, for example:

let o: Observable<number> = new Subject();

That makes sense, because we're "pretending" T is a property.

Only question I have is would there be any benefit / negative of using the interface syntax here instead? (Besides it being a little more verbose)

export interface FromEventObservable<T, R> {
    ' tag_class_FromEventObservable': [T, R];
}
export class FromEventObservable<T, R> extends Observable<T> {
    ...

[vladima]

1. yes, let o: Observable<number> = new Subject<number>(); should work
2. with interfaces this become much more invasive - this property needs to be repeated in all classes that implement the interface. This includes user classes, so this will be yet another breaking change (and I guess more difficult to explain to end user)

[david-driscoll]

Fair enough, I just wasn't sure if it would be a plus or a minus (sounds like a minus!)

[benlesh]

This includes user classes, so this will be yet another breaking change (and I guess more difficult to explain to end user)

... for users that happen to be using TypeScript. Everyone else will be fine. It sounds okay.

Two things I can see:

• I think we should add comments everywhere these exist that explain what they're for
• We can flatten this to one commit. The commit message needs to outline the breaking change with a separate line like BREAKING CHANGE: blah blah blah.

Other than that, it looks good. Thanks for generous contribution @vladima

[jayphelps]

@vladima just to clarify, this speeds up compilation because the key starts with a space and when structurally comparing the shapes, TypeScript does so by comparing the properties sorted alpha-numerically + spaces, so including a space as the first characters of these tag properties, " tag_class_Observable" the tag is compared first so the checker can abort quickly?

[jayphelps]

In pseudo code: (I realize the typesystem works on a meta level, so it's not runtime like this)

function typesAreEqual(a, b) {
  const keys = keys(a)
    .concat(
      keys(b)
    )
    .removeDuplicates()
    .sort();

  keys.forEach(key => {
    if (typeof a[key] === typeof b[key]) {
      return false;
    }
  });

  return true;
}

class A {
  ' first': number;
  second() {}
  third() {}
}

class B {
  ' first': boolean;
  second() {}
  third() {}
}

let a = new A();
let b = new B();

typesAreEqual(a, b);
// false

[vladima]

compiler processes properties in declaration order. Space in the name is a (kind of hacky) way to hide this property in the completion list - currently we don't include members with such names in the completion list.

[benlesh]

@vladima and @mhegazy ... would it help if instead of doing this fix, we actually had all methods that would normally accept an Observable to accept an ISubscribable instead?

[vladima]

what is the expected shape of ISubscribable?

[benlesh]

@vladima something like:

interface ISubscribable<T> {
  subscribe: (observer: Observer<T>) => Subscription;
}

[vladima]

so ObservableInput<T> will look like export type ObservableInput<T> = ISubscribable<T> | Promise<T> | ArrayOrIterator<T>; ?

[vladima]

yes, it works. I've made this change and tried repro cases that we have - results are just marginally worse than with tagged types.

[vladima]

#1475 introduces Subscribable interface that is used instead of Observable in ObservableInput.

[robwormald]

cc @IgorMinar as this will be a breaking change on our end if i'm reading @vladima's OP properly

[vladima]

I think that this PR will be closed in favor of #1475 that does not introduce any breaks

[benlesh]

Yup, closed in favor of #1475

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.