Skip to content

ECMAScript spread param rules causing loss of type-safety (and slower compile time?) #7347

Closed
@benlesh

Description

@benlesh

I think we're hitting a similar situation to #1336 with RxJS 5, which is causing some crazy workarounds for some minimal type-safety, which in turn is causing slower build times.

Ideally what we'd have is this:

// Observable.prototype.concat
concat<R>(...observables: ObservableInput<any>[], scheduler?: Scheduler): Observable<R>

But since that's illegal, we're forced to do this:

concat<R>(...args: Array<ObservableInput<any> | Scheduler>): Observable<R>

... that clearly doesn't give us what we want for type safety, so we end up defining a crazy list of common type signatures like:

/* tslint:disable:max-line-length */
export interface ConcatSignature<T> {
  (scheduler?: Scheduler): Observable<T>;
  <T2>(v2: ObservableInput<T2>, scheduler?: Scheduler): Observable<T | T2>;
  <T2, T3>(v2: ObservableInput<T2>, v3: ObservableInput<T3>, scheduler?: Scheduler): Observable<T | T2 | T3>;
  <T2, T3, T4>(v2: ObservableInput<T2>, v3: ObservableInput<T3>, v4: ObservableInput<T4>, scheduler?: Scheduler): Observable<T | T2 | T3 | T4>;
  <T2, T3, T4, T5>(v2: ObservableInput<T2>, v3: ObservableInput<T3>, v4: ObservableInput<T4>, v5: ObservableInput<T5>, scheduler?: Scheduler): Observable<T | T2 | T3 | T4 | T5>;
  <T2, T3, T4, T5, T6>(v2: ObservableInput<T2>, v3: ObservableInput<T3>, v4: ObservableInput<T4>, v5: ObservableInput<T5>, v6: ObservableInput<T6>, scheduler?: Scheduler): Observable<T | T2 | T3 | T4 | T5 | T6>;
  (...observables: Array<ObservableInput<T> | Scheduler>): Observable<T>;
  <R>(...observables: Array<ObservableInput<any> | Scheduler>): Observable<R>;
}
/* tslint:enable:max-line-length */

I believe this is directly related to compilation speed issues that @mhegazy wanted to discuss with me.

cc/ @kwonoj @david-driscoll @jayphelps

Activity

benlesh

benlesh commented on Mar 2, 2016

@benlesh
Author

Since this relates to compile-times relevant to Angular 2, which uses and/or is commonly paired with RxJS 5, also cc/ @IgorMinar @mhevery

RyanCavanaugh

RyanCavanaugh commented on Mar 2, 2016

@RyanCavanaugh
Member

Is the request here simply to allow rest args in non-final positions?

benlesh

benlesh commented on Mar 2, 2016

@benlesh
Author

@RyanCavanaugh, I think so yes.

But there would need to be rules around it. For example, you couldn't have optional params before rest params, but you could have them after.

benlesh

benlesh commented on Mar 2, 2016

@benlesh
Author

Now in ECMAScript, I think they could loosen the rules, but not as much as you could in a typed language. For example, without types: foo(...bar, baz, ...qux) would be impossible to suss out. But with types, it's actually doable at compile time: foo(...bar:number[], baz:string, ...qux:number[]).

We don't have to go all-in with this change, but I would be happy with just allowing them in places that are doable in ECMAScript

mhegazy

mhegazy commented on Mar 2, 2016

@mhegazy
Contributor

I am not sure i see how you can figure something like foo(...bar:number[], baz:string, ...qux:number[]).?

i see allowing named arguments to come at the end, and the generated code would do arguments[arguments.length-position] but do not see how you can do the multiple rest args case.

david-driscoll

david-driscoll commented on Mar 2, 2016

@david-driscoll

In theory you could do multiple rest arguments, but you would have to code gen some metadata into it (which hurts my head when thinking about JS <-> TS).

Keep in mind this particular problem isn't new to RxJS5, this also applies to RxJS4. I can't think of any other big names where I've seen this pattern though. In the Rx world it makes sense, and once you're used to Rx concepts, it's honestly fairly intuitive.

Variadic Types may very well fix this problem.

combineLatest is a pretty popular (at least for me 😄)

combineLatestStatic<T, T2, T3, T4, T5, T6>(v1: ObservableInput<T>, v2: ObservableInput<T2>, v3: ObservableInput<T3>, v4: ObservableInput<T4>, v5: ObservableInput<T5>, v6: ObservableInput<T6>, scheduler?: Scheduler): Observable<[T, T2, T3, T4, T5, T6]>;
combineLatestStatic<T, T2, T3, T4, T5, T6, R>(v1: ObservableInput<T>, v2: ObservableInput<T2>, v3: ObservableInput<T3>, v4: ObservableInput<T4>, v5: ObservableInput<T5>, v6: ObservableInput<T6>, project: (v1: T, v2: T2, v3: T3, v4: T4, v5: T5, v6: T6) => R, scheduler?: Scheduler): Observable<R>;

Possible Variadic version

combineLatestStatic<T...>(inputs: T..., scheduler?: Scheduler): Observable<[T...]>;
combineLatestStatic<T..., R>(inputs: T..., project: (inputs: T...) => R, scheduler?: Scheduler): Observable<R>;
  • Notes
    • the scheduler is the last optional argument, but the projection method can also be the last item
    • the overload without the projection does return a Tuple, which is extremely useful, because you can just destructure the tuple later on in your chain.

ie

Observable.combineLatest(a, b, c)
    .filter(([, something]) => something.isValid())
    .map(([a, b, c]) => ({ /* now I want to project the values */ })

Merge is another fun one...

mergeStatic<T, T2, T3, T4, T5, T6>(v1: ObservableInput<T>, v2: ObservableInput<T2>, v3: ObservableInput<T3>, v4: ObservableInput<T4>, v5: ObservableInput<T5>, v6: ObservableInput<T6>, scheduler?: Scheduler): Observable<T | T2 | T3 | T4 | T5 | T6>;
mergeStatic<T, T2, T3, T4, T5, T6>(v1: ObservableInput<T>, v2: ObservableInput<T2>, v3: ObservableInput<T3>, v4: ObservableInput<T4>, v5: ObservableInput<T5>, v6: ObservableInput<T6>, concurrent?: number, scheduler?: Scheduler): Observable<T | T2 | T3 | T4 | T5 | T6>;

Possible varidaic version

mergeStatic<...T>(inputs: ...T, scheduler?: Scheduler): Observable<...T>;
mergeStatic<...T>(inputs: ...T, concurrent?: number, scheduler?: Scheduler): Observable<...T>;
  • Notes:
    • generally speaking when you merge, you will always merge the same type T but this isn't always the case, so the result is a union of all Ts
    • concurrency comes in here, again scheduler is always the last argument, but is optional.
david-driscoll

david-driscoll commented on Mar 2, 2016

@david-driscoll

Also how would we handle constraints on variadic types?

I forgot to model it in my last comment, but something like...

mergeStatic<...T extends ObservableInput<...T>>(inputs: ...T, concurrent?: number, scheduler?: Scheduler): Observable<...T>;
RyanCavanaugh

RyanCavanaugh commented on Mar 2, 2016

@RyanCavanaugh
Member

There's already #1773 tracking variadic type parameters so let's not complicate the discussion here with that.

I agree that more than one rest parameter should not be allowed in the event that we do support this.

I would be happy with just allowing them in places that are doable in ECMAScript

To be clear, this is already the case. The ES spec (https://tc39.github.io/ecma262/#sec-function-definitions) only allows rest args in the last parameter position. As far as I know, there isn't even a Stage 0 proposal for supporting them elsewhere.

benlesh

benlesh commented on Mar 2, 2016

@benlesh
Author

Here's a pertinent discussion on ESDiscuss: https://esdiscuss.org/topic/rest-parameters

I think a simple set of rules can enable up front rest params in ECMAScript.

  • NO rest params anywhere after other rest params
  • NO rest params anywhere after defaulted or optional arguments
  • NO defaulted or optional params anywhere after rest params

In TypeScript, the last rule isn't as necessary, because of type checking I think.

And I think TypeScript, at least, could throw compilation errors if a function is called with a shorter than necessary number of arguments, and the ECMAScript implementation itself could throw a run time error in that case... for example:

These should be errors, IMO:

foo(a, ...b, c) { };  foo(1, 2);
bar(...a, b, c) { };  bar(1);
RyanCavanaugh

RyanCavanaugh commented on Mar 2, 2016

@RyanCavanaugh
Member

The thread there generally points to this not ever happening in ECMAScript. I don't see why any of their arguments are less correct in TypeScript.

benlesh

benlesh commented on Mar 3, 2016

@benlesh
Author

I don't see why any of their arguments are less correct in TypeScript.

The "arguments" are mostly just "what ifs" without answers.

If a solid set of rules was made around it, I don't see why it couldn't be implemented. If TypeScript supported plugins I'd add it myself. The real trick, though, is the type-safety. That's an actual problem.

@jayphelps actuall pointed out that Swift actually supports this feature, but that seems to be enabled by the fact that swift has named arguments after the first argument like concat(a, b, c, scheduler: myScheduler) would match concat(observables: Observable..., scheduler: Scheduler)

benlesh

benlesh commented on Mar 3, 2016

@benlesh
Author

rest feature or no feature, I need a way to reliably and efficiently support type-safety with this method signature. That's the real issue at hand.

jayphelps

jayphelps commented on Mar 4, 2016

@jayphelps

One solution that wouldn't impact ECMAScript spec is to only allow first param spread in abstract/overload signatures but not in the actual implementation/concrete signature.

function concat(...observables: Array<Observable>, scheduler: Scheduler);
function concat(...args: Array<Observable | Scheduler>) {
  console.log(args.length);
  // 3
}

concat(new Observable, new Observable, new Scheduler);
function concat(...observables: Array<Observable>, scheduler: Scheduler) {
  // do stuff
}
// error: A reset parameter must be last in a parameter list, except in overload signatures.

That means that it's a purely compile type-related feature, never a runtime one. This is obviously a bit of a quirk, but would solve the ECMA argument AFAIK.

benlesh

benlesh commented on Mar 4, 2016

@benlesh
Author

@jayphelps the contention there will be roughly the same as the contention around foo(a, ...b, c) though:

function foo(...a: TypeA[], b: TypeB, c: TypeC) { }
foo(1);  // what's what?

// or worse
function bar(...a: TypeA[], b?: TypeB, c?: TypeC) { }
bar(2); // what now?

What I propose is the above call to foo(1) should be compilation errors in TypeScript, it should probably be a runtime error as well. But the call to bar(2) should probably be fine. If you look at how someone would have implement the above methods it'll be more apparent:

function foo(...args: Array<TypeA|TypeB|TypeC>) {
  const a = args.slice(0, args.length - 3);
  const b = args[args.length - 2];
  const c = args[args.length - 1];
}

// or even
function foo(...args: Array<TypeA|TypeB|TypeC>) {
  const c = args.pop();
  const b = args.pop();
  const a = args;
}


// bar gets weirder
function bar(...args: Array<TypeA|TypeB|TypeC>) {
  let c: TypeC;
  let b: TypeB;
  if (args[args.length - 1] instanceof TypeC) {
    c = <TypeC>args.pop();
  }
  if (args[args.length - 1] instanceof TypeB) {
    b = <TypeB>args.pop();
  }
  const a: TypeA[] = <TypeA[]>args;
  // do stuff
}

Things to notice:

  1. None of them are type-safe, for the desired signature which is really foo(...a, b, c) and bar(...a, b?, c?).
  2. both impls of foo will throw if you pass an invalid number of arguments. We could guard against that, sure, but most likely developers wouldn't do that.
  3. The bar impl is mostly fine and will make "smarter" decisions about how to handle calls to it like bar(new TypeA()), bar(new TypeC()) and bar(new TypeB())... but won't really know what to do about out of order calls like bar(new TypeC(), new TypeA()).

What I'd like is to support the type signatures above with rest params at the front or even in the middle and generate accompanying code that adheres to some rules and enforces those rules. Front will do for now.

15 remaining items

david-driscoll

david-driscoll commented on Mar 8, 2016

@david-driscoll

the generic types on concat, merge are useful because they let us resolve something like...

let obs1: Observable<number>;
let obs2: Observable<number>;
const result: Observable<number> = obs1.concat(obs2);

to something like....

let obs1: Observable<number>;
let obs2: Observable<number>;
const result = obs1.concat(obs2);

The less manual inference I as the consumer has to do, the easier my life is. In addition if you make a small incompatible change at the top of a set of operations, it cascades gracefully down letting you see how badly you failed.

let obs1: Observable<{item: number}>;
let obs2: Observable<string>;
const result = obs1
    .concat(obs2)
    .map(x => x.item * 2) // would fail saying `x` doesn't have a property of `item`

I'm fine with ignoring variadics for the moment to deal with this specific scenario (rest args), but variadics will be useful going forward beyond just combineLatest and zip.

david-driscoll

david-driscoll commented on Mar 8, 2016

@david-driscoll

Now to note, for concat/merge we could drop the requirement of variadics and just assume we're getting a named type T, and then returning Observable<T>. That covers most situations, but then some operations, where you want to have a stream of Apples or Oranges would have to be explicitly typed.

added
Too ComplexAn issue which adding support for may be too complex for the value it adds
RevisitAn issue worth coming back to
and removed on Mar 15, 2016
RyanCavanaugh

RyanCavanaugh commented on Mar 15, 2016

@RyanCavanaugh
Member

This pattern seems to be relatively rare and would be quite complex to handle during overload resolution, which makes tons of assumptions about parameter ordering. If we see other libraries putting things at the end we can reconsider (i.e. if you encounter other functions with this pattern, please leave a comment with a link).

jayphelps

jayphelps commented on Mar 15, 2016

@jayphelps

@RyanCavanaugh understandable.

It's definitely not a super common problem. I imagine it's usually APIs dealing with a callback, since they're typically the last arg but even then, not something I see often.

benlesh

benlesh commented on Mar 21, 2016

@benlesh
Author

This pattern seems to be relatively rare

It seems like Angular 1 had this pattern everywhere for their DI, or at least something very similar.

benlesh

benlesh commented on Mar 21, 2016

@benlesh
Author

Either way, I'm pleased that you at least considered our needs. 👍

TheLarkInn

TheLarkInn commented on Sep 21, 2016

@TheLarkInn
Member

Since this is relevant to my current endeavors I'm going to comment here in the case it gets revisited.

In attempting to convert one of our webpack core libraries, I too, came across the same issue when trying to implement the following: https://github.com/webpack/tapable#applypluginsasyncseries

This is the current JavaScript implementation:

Tapable.prototype.applyPluginsAsyncSeries = Tapable.prototype.applyPluginsAsync = function applyPluginsAsync(name) {
    var args = Array.prototype.slice.call(arguments, 1);
    var callback = args.pop();
    if(!this._plugins[name] || this._plugins[name].length === 0) return callback();
    var plugins = this._plugins[name];
    var i = 0;
    var _this = this;
    args.push(copyProperties(callback, function next(err) {
        if(err) return callback(err);
        i++;
        if(i >= plugins.length) {
            return callback();
        }
        plugins[i].apply(_this, args);
    }));
    plugins[0].apply(this, args);
};

And the documentation for that function specifying the following:

applyPluginsAsyncSeries

applyPluginsAsyncSeries(
    name: string,
    args: any...,
    callback: (err: Error, result: any) -> void
)

Asynchronously applies all registered handers for name. The hander functions are called with all args and a callback function with the signature (err: Error) -> void. The handers are called in series, one at a time. After all handlers are applied, callback is called. If any handler passes a value for err, the callback is called with this error and no more handlers are called.

I will take the same approach that @Blesh et al are taking for RxJS. Only problem is that the number of arguments can reach up to 50+ (because in webpack core, almost every bit of functionality uses this plugin system).

Thanks for tracking this!

UPDATE:

50+ (because in webpack core, almost every bit of functionality uses this plugin system).

I mispoke on this, the arguments list can be pretty large still but plugins registration happens at a different point etc.

locked and limited conversation to collaborators on Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    RevisitAn issue worth coming back toSuggestionAn idea for TypeScriptToo ComplexAn issue which adding support for may be too complex for the value it adds

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @jayphelps@david-driscoll@benlesh@TheLarkInn@RyanCavanaugh

        Issue actions

          ECMAScript spread param rules causing loss of type-safety (and slower compile time?) · Issue #7347 · microsoft/TypeScript