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

All Optional Object Interface Converts to any #7485

Closed
kitsonk opened this issue Mar 11, 2016 · 24 comments · Fixed by #16047
Closed

All Optional Object Interface Converts to any #7485

kitsonk opened this issue Mar 11, 2016 · 24 comments · Fixed by #16047
Assignees
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Mar 11, 2016

I couldn't find a relevant issue, but this does seem rather fundamental and against the principle of:

  1. Statically identify constructs that are likely to be errors.

TypeScript Version:

1.8.7

Code

interface Optional {
    foo?: string;
}

function foo(o: Optional): void {}

foo('bar'); // should throw
foo(1); // should throw
foo({ bar: 'baz' }); // should (and does) throw

Expected behavior:

Expected behaviour is that you should be able to type guard against other non-objects and only accept Objects with no properties or declared properties.

Actual behavior:

Passing anything other than an object literal that contains extra properties is acceptable.

@danquirk
Copy link
Member

#3842 seems relevant

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Mar 28, 2016
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Apr 13, 2016
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Apr 13, 2016
@RyanCavanaugh
Copy link
Member

We talked about this briefly and think the idea in #3842 is worth trying out. I ported the PR forward but the code was written incorrectly around this specific case:

interface NotAllOptional {
  x: string;
}
interface AllOptional {
 y?: number;
}
type mixed= NotAllOptional & AllOptional;
let z: mixed = { x: 'ok' };

The code I had failed because we check intersection assignability by seeing if the source is assignable to each constituent in sequence; the first type succeeds and the second type fails (even though it shouldn't). A more complex check is needed here but I don't have time to implement it right now.

If someone wants to figure it out and send an updated PR that we could try against our internal suite of partner code, that'd be great. No guarantees it's going to be worth taking a breaking change over, but seeing what kind of issues (and non-issues) it finds might help us down the path to a more complete solution if needed.

@JabX
Copy link

JabX commented Apr 15, 2016

I'm a bit confused, you just said that when you check assignability in an intersection you check the assignability of the source to both constituents, but if I have something like that:

interface A { a: string }
interface B { b: string }

const test: A & B
test = {a: 'a', b: 'b'}

It works fine (as expected) even though test isn't assignable to either A or B?

@JabX
Copy link

JabX commented Apr 29, 2016

What's the status on this? In this something we can reasonably expect to be part of the 2.0 release?

I've been continously porting my app to Typescript over the past month or two and this is the one feature I desperately miss, as I'm using a lot of weak-typed objects to represent the state of a React component or for DTOs.

@kitsonk
Copy link
Contributor Author

kitsonk commented Apr 29, 2016

What's the status on this? In this something we can reasonably expect to be part of the 2.0 release?

The labels indicate that it a potentially good idea (Suggestion), but that the TypeScript team have limited resources and do not feel it is important enough to address (Accepting PRs) and are currently leaving it up to the community to address (Milestone = Community).

@DomenicD
Copy link

DomenicD commented Oct 7, 2016

I believe this is a bigger issue because of TypeScript named parameters.

class Foo {
  bar({a = <string|undefined>void 0, b = <number|undefined>void 0} = {}) {
    // Do something
  }
}
let foo = new Foo();
/*
This explicitly violates the method signature,
which states that it only takes optional named parameters 
"a" and "b".
*/
foo.bar("hello"); 

The named parameters feature for methods and functions is broken until the type system can support it.

@saschanaz
Copy link
Contributor

Is this issue solved by the new object type?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 7, 2017

Is this issue solved by the new object type?

No. a type with only optional properties is treated as {} from assignment-compatibility perspective, i.e. you can pass in almost any thing to it.

@realyze
Copy link

realyze commented Feb 28, 2017

This is kind of a big deal when using the "optional options" pattern:

interface IOptions {
  upsert?: boolean;
  debug?: boolean;
}

function questionableBehaviour(id: string, options?: IOptions, callback?: Function) {
  // code
}

questionableBehaviour('testId', 'invalid string', (err) => console.log(err));

This IMHO should definitely not compile.

@RyanCavanaugh
Copy link
Member

Excluding the primitives and incorrect object literals is nice but doesn't go far enough to be "solved" IMO. You can still e.g. use a class constructor when you meant to use a class instance.

@bcherny
Copy link

bcherny commented Apr 19, 2017

e.g. use a class constructor when you meant to use a class instance.

Does that happen for anything other than the pathological case of an empty class? Supporting extends object seems like a cheaper way to the cover a lot of cases.

@bijoutrouvaille
Copy link

I just asked a related question on SO and was directed to this thread. Here is an example where the object & intersection doesn't solve the problem:

type Right = { right?: boolean } & object
type Wrong = { wrong: {fail: number} }

function returnWrongX(p: Right, callback: (param: Right) => Right) { 
    return callback(p).right
}
// throws a runtime error
returnWrongX({}, (param: Wrong) => param.wrong.fail ? {} : {})

@bcherny
Copy link

bcherny commented Apr 20, 2017

@bijoutrouvaille I see what you mean, in that the typing is more fragile than it should be. On the last line you're explicitly broadening param's type to Wrong, which compiles.

If you don't type param on the last line and let TS infer it as Right, then you get a compile time error as expected.

@3n-mb
Copy link

3n-mb commented Apr 30, 2017

If the fix breaks some corner cases, can we still have the fix under a new compiler flag?

Example, strictNullChecks, it found tones of things that were missed before this strict check was available. Can we have the same here. Please ...

@bcherny
Copy link

bcherny commented Apr 30, 2017

As usual, @RyanCavanaugh is right :) A few cases that compile, but shouldn't:

type A = object & { foo?: number }

let a: A = {}
let b: A = [1]
let c: A = new class { bar = 42 }
let d: A = () => 'foo'

@kitsonk
Copy link
Contributor Author

kitsonk commented May 1, 2017

And a few more:

let e: A = null;
let f: A = /foo/;
let g: A = class A { };

It has to at least work somewhat correctly for it to be considered @3n-mb.

@rcollette
Copy link

It would be nice if you could specify that the object have at least one of the optional members. This way if you specify something that doesn't match at least one of the expected properties, it will get caught at compile time.

@kitsonk
Copy link
Contributor Author

kitsonk commented May 18, 2017

It would be nice if you could specify that the object have at least one of the optional members.

That can already be modelled by an intersection type:

type AtLeastOne = { foo: string; bar?: string } | { foo?: string; bar: string};
const a: AtLeastOne = { foo: 'bar' }; // ok
const b: AtLeastOne = { bar: 'baz' }; // ok
const c: AtLeastOne = {}; // error on `c`

@rcollette
Copy link

While it can be be done with an intersection, it quickly becomes unwieldy for something even relatively small like a mailing address.

@kitsonk
Copy link
Contributor Author

kitsonk commented May 24, 2017

While it can be be done with an intersection, it quickly becomes unwieldy for something even relatively small like a mailing address.

Even so, I suspect that would be a different request than what this issue is about.

@sandersn sandersn added the Fixed A PR has been merged for this issue label May 30, 2017
@sandersn sandersn moved this from In Progress to Done in Rolling Work Tracking May 30, 2017
@mhegazy mhegazy modified the milestones: TypeScript 2.4, Community Jun 3, 2017
@sandersn sandersn removed this from Done in Rolling Work Tracking Jun 6, 2017
@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
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.