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

How to define a parameter as optional, but if specified be ensured as not undefined. #11988

Closed
electricessence opened this issue Nov 2, 2016 · 21 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@electricessence
Copy link

electricessence commented Nov 2, 2016

This is a bit tough to explain but probably similar to: #10014

TypeScript Version: 2.0.6

Code

So if I want to specify (when using strict null checks):

export interface Selector<TSource, TResult>
{
	(source:TSource, index?:number):TResult;
}

Which will allow me to use functions that take 1 or 2 parameters where the second one is a number or undefined. But what I really want is that if a second parameter is defined it will be a number.

Any signature style overrides I tried seemed to just screw things up more.
Is there a workaround for this without having to create separate interfaces like:

export interface Selector<TSource, TResult>
{
	(source:TSource, index?:number):TResult;
}
export interface SelectorIndexed<TSource, TResult>
{
	(source:TSource, index:number):TResult;
}
@RyanCavanaugh RyanCavanaugh changed the title How to define a property as optional, but if specified be ensured as not undefined. How to define a parameter as optional, but if specified be ensured as not undefined. Nov 2, 2016
@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Nov 2, 2016
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Nov 2, 2016

There isn't a way to specify that a parameter is optional but may not accept undefined. Basically, TypeScript's view of the world is that the only legitimate way to resolve overloads is by checking for undefined, rather than checking arguments.length, and that applies symmetrically to callers (you can pass undefined any time you could have omitted a parameter).

@electricessence
Copy link
Author

electricessence commented Nov 3, 2016

There seems to be other ways of solving this but it seems like extra work (like in my sample).

Here's an example of how I ran into this and what I had to do to get around it...
(Obviously with strictNullChecks:true)

// Given the following map function...  And the Selector as described above.
function map<T,TResult>(source:T[], s:Selector<T,TResult>):TResult[] {
  var result:TResult[] = [];
  for(let i = 0; i<source.length; i++) {
     let e = source[i];
     result[i] = s(e,i);
  }
  return result;
}

console.log(map(["a","b","c"], (e,i)=>i)); // [0,1,2]

The problem I encountered is when you use (e,i)=>i strictNullChecks will emit an error that i could be undefined. Now I understand it's as easy as writing: (e,i)=>i! and you're done. But I thought a lot about this and it's just not right... I should be able to say with absolute confidence that i _will_ return a number. The other conundrum happens if I try to enforce it in reverse because what if the lambda I wrote is simply e=>e+"x" then i is unneeded and will break even though it's just extra.

The only solution I came up with (which is okay, but not ideal) is adding an overload to this example map method:

function map<T,TResult>(source:T[], s:(e:T,i:number)=>TResult):TResult[]
function map<T,TResult>(source:T[], s:Selector<T,TResult>):TResult[]
{
///....
}

And to be honest I'm not 100% sure if the above code works. Actual implementation (of what I did) is here:
https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Text/RegularExpressions.ts#L195-L209
Where MatchEvaluator is simply a Selector.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Nov 3, 2016

Callback parameters (e.g. index in your Selector / map example) should not be defined as optional -- that's why you're getting the error about undefined there. Optional in that location means "I may or may not provide this to you", not "You don't necessarily have to look at that parameter".

@electricessence
Copy link
Author

Right... That actually makes sense. I think on a fundamental level you are correct. The issue is the amount of additional code required to get it to work that way. So this comes back to (please help me if I'm doing this wrong):

Why can't I:

export interface Selector<TSource, TResult>
{
    (source:TSource):TResult;
    (source:TSource, index:number):TResult;
}

When I do the above, I encounter a huge amount of breakage.
The question is... Am I writing this overloaded signature correctly?
Is this the correct way?

@electricessence
Copy link
Author

And so more importantly... I'm pretty confident I can get the above to work... BUT...
It means much more complicated signatures for parameters in functions. :(

@electricessence
Copy link
Author

Okay so here's the problem I'm seeing with the above signature...
All my lambdas using it report:

Error:(1022, 13) TS7006: Parameter 'x' implicitly has an 'any' type.

Which they didn't have any problem inferring the type of x before. :(

@electricessence
Copy link
Author

So it keeps falling back to (as you were saying) being more constrained (not optional parameters) but that I have to define multiple signatures for every method that takes a selector.

@electricessence
Copy link
Author

electricessence commented Nov 3, 2016

One last question:
How do signatures decide which are valid? Do you have to always have a root signature?

a(v:Number) {}
a(v:String) {}
a(v:Number | String) {}

// older way:
b(v:Number) {}
b(v:String) {}
b(v:any) {}

But in the case of 'b' you aren't allowed to pass boolean even though the root is 'any'. :/

In the case I'm talking about... Why is this allowed:

a(s:Selector<T,TResult>) {} // where Selector does not contain an index parameter.
a(s:SelectorWithIndex<T,TResult>) {} 

...update...
Okay so it seems that to get this to work the way I want, I need to do this:

a(s:Selector<T,TResult>|SelectorWithIndex<T,TResult>) {}
a(s:SelectorWithIndex<T,TResult>) {} 

@RyanCavanaugh
Copy link
Member

Please take questions to Stack Overflow. I think you also need to refer to this FAQ entry: https://github.com/Microsoft/TypeScript/wiki/FAQ#why-am-i-getting-supplied-parameters-do-not-match-any-signature-error regarding signatures, and also remember not to use Number or String (you almost always want to use number or string)

In general if you have a callback type, it should only have one signature, and you should specify the full arity of that signature.

If you have a function type, write as few signatures as possible (i.e. combine individual types into union types), and place more-specific signatures (those taking more arguments or more-specific types) before less-specific signatures.

@electricessence
Copy link
Author

Sorry if I'm over asking. It still seems like something that needs fixing. I have to write a lot more to get the result I'm expecting.. Including (update) needing to do this:

a(s:Selector<T,TResult>) {}
a(s:SelectorWithIndex<T,TResult>) {} 
a(s:SelectorWithIndex<T,TResult>) {} 

@electricessence
Copy link
Author

I'm guessing that answers my question. It has to have a root signature. :/

@RyanCavanaugh
Copy link
Member

This is very confusing because you're not even posting legal syntax. Self-contained examples, that actually parse, with what you expect to happen, would be really useful.

@electricessence
Copy link
Author

I working on a solution and will post it and close this issue once I do (shortly).
Any subsequent suggestion/comments will be appreciated.

@electricessence
Copy link
Author

electricessence commented Nov 4, 2016

Okay here's the update. And please hang in here with me because I do believe there is a problem that when I try an implement this, it breaks type inference.
Actual code:

export interface SelectorWithoutIndex<TSource, TResult>
{
    (source:TSource):TResult;
}

export interface SelectorWithIndex<TSource, TResult>
{
    (source:TSource, index:number):TResult;
}

export type Selector<TSource, TResult>
    = SelectorWithoutIndex<TSource,TResult>
    | SelectorWithIndex<TSource, TResult>;

export interface ActionWithoutIndex<T> extends SelectorWithoutIndex<T, void>
{
}

export interface ActionWithIndex<T> extends SelectorWithIndex<T, void>
{
}

export type Action<T>
    = ActionWithoutIndex<T>
    | ActionWithIndex<T>;


export interface PredicateWithoutIndex<T> extends SelectorWithoutIndex<T, boolean>
{
}

export interface PredicateWithIndex<T> extends SelectorWithIndex<T, boolean>
{
}

export type Predicate<T>
    = PredicateWithoutIndex<T>
    | PredicateWithIndex<T>;


export interface Comparison<T>
{
    (a:T, b:T, strict?:boolean):number;
}

export interface EqualityComparison<T>
{
    (a:T, b:T, strict?:boolean):boolean;
}


export interface Func<TResult>
{
    ():TResult;
}

export interface Closure
{
    ():void;
}

By using the above code (eliminating optional parameters) I find a lot of serious issues with my own code and THANK YOU for strictNullChecks... AMAZING. Can't say enough about this feature.

But... The above changes break inference all over the place...
I have a special forEach method that used to look like this:

forEach(action:Action<T> | Predicate<T>):void { }

Now to make it work I have to:

    forEach(action:ActionWithIndex<T>):void;
    forEach(action:ActionWithoutIndex<T>):void;
    forEach(action:PredicateWithIndex<T>):void;
    forEach(action:PredicateWithoutIndex<T>):void;
    forEach(action:PredicateWithIndex<T> | ActionWithIndex<T>):void {}

Otherwise I get

TS7006: Parameter 'x' implicitly has an 'any' type.

@electricessence
Copy link
Author

electricessence commented Nov 4, 2016

So I've confirmed that type inference is unnecessarily broken.
Any subsequent usage of any similar method signature like the above forEach, will break type inference. And in my case, in order to get everything working properly. I effectively have to rewrite my entire code base to ensure proper typing.

All methods that appears as such:

a(p:Predicate<T>) {}

Have to be rewritten to look like:

a(p:PredicateWithoutIndex<T>):void;
a(p:PredicateWithIndex<T>):void;
a(p:PredicateWithIndex<T>):void {}

@RyanCavanaugh
Copy link
Member

Again you have a ton of overcomplicated code here but no actual invocations that show what behavior you're trying to accomplish.

@electricessence
Copy link
Author

electricessence commented Nov 4, 2016

Ok. I admit, it's a bit complicated, and unclear. I apologize.

Here is the code before I've migrated away from optional parameters (which I see now as essential for me to do).
https://github.com/electricessence/TypeScript.NET/blob/master/source/System/FunctionTypes.d.ts

And here is an actual use case that if not implemented correctly (using signatures) will end up with the type inference error I described:
https://github.com/electricessence/TypeScript.NET/blob/master/source/System.Linq/Linq.ts#L1021-L1022
https://github.com/electricessence/TypeScript.NET/blob/master/source/System.Linq/Linq.ts#L1024-L1026

I originally spotted this issue here:
https://github.com/electricessence/TypeScript.NET/blob/master/source/System/Text/RegularExpressions.ts#L190-L203
Note that I added the MatchEvaluatorIndexed interface later to eliminate the problem with the type inference error which occured here:
https://github.com/electricessence/TypeScript.NET/blob/master/tests/mocha/System/Text/RegularExpressions.ts#L89 (note the lambda)

I could give you more examples, but the list would be long.

@electricessence
Copy link
Author

electricessence commented Nov 4, 2016

In the end. It is still possible to do what I need to do. But I think there could be an improvement in type inference because it really seems odd that I have to write so much additional code in order to get it to understand my intention. I'm now in the middle of rewriting a significant amount of type-code to get the signatures and constraints right so that:

  1. Internal and external signatures to different methods line up.
    (which I'm writing more overloads than I should need to because no.2).
  2. Type inference works naturally regardless of if I use (x)=>x or (x,i)=>i.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Nov 4, 2016

Before you spend a ton of time on this, I just want to be clear: You should be able to define a signature (x: T, i: number) => T | number and use that both with x => x and (x, i) => i. There's almost always no need to ever have multiple callback signatures like what you have here.

@electricessence
Copy link
Author

electricessence commented Nov 6, 2016

Sure that might solve that particular case. But the problem really boils down to some very strange type inference issue. Where I'm quite happy to have 2 separate signatures. One for just (x) and another for (x,i). But for whatever reason, I have to create a very long list of overloads for each function that receives this in order for type inference to work. Instead of a union type.

You're welcome to watch me work on this: (@RyanCavanaugh I mention your name in the video but I slip and say Dave :P. Sorry.)
https://www.youtube.com/watch?v=8CZH5BLQks0

@electricessence
Copy link
Author

electricessence commented Nov 12, 2016

@RyanCavanaugh just an update. I may have figured this out. Wanted to say thanks again for your help.
I'll know for sure after this morning if when using (x,i) for a function signature works even if you don't want i.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

2 participants