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

What do we do about object literals that make use of structural subtyping in 1.6? #4234

Closed
DanielRosenwasser opened this issue Aug 8, 2015 · 21 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

Consider the following hierarchy:

interface Base {
    kind: string;
}

interface Derived1 extends Base {
    prop1: number;
}

interface Derived2 extends Base {
    prop2: string;
}

interface Container {
    a: Base;
}

let x: Container = {
    a: {
        kind: "Derived1",
        prop1: 1000,
    }
}

Humor the idea that when Container says that it has a property a, there was an implicit setof types that will be accounted for throughout the program - Derived1 and Derived2. Really, a should have type Derived1 | Derived2, but there's is an intuition of doing things with a common supertype from languages like C#, Java, etc. The above code is perfectly valid, however, in 1.6 we will report:

foo.ts(17,5): error TS2322: Type '{ a: { kind: string; prop1: number; }; }' is not assignable to type 'Container'.
  Types of property 'a' are incompatible.
    Type '{ kind: string; prop1: number; }' is not assignable to type 'Base'.
      Object literal may only specify known properties, and 'prop1' does not exist in type 'Base'.

I think the consequences of this were recognized, but underestimated. For instance, as part of my work for #4081, I've run into a test case where I have 177,000 lines of tests that utilized this pattern - see fhir/fhir-tests.d.ts.

Do we feel we can think of a better way for users to deal with this? There's no way users are willing to comb through that much code and add type assertions to fix that test. I know the definitions could be amended to use string index signatures, but that gets us back to square 1 (plus, there's 10K lines in the definition file as well.

@RyanCavanaugh
Copy link
Member

The fhir case is simply insane. It's clearly auto-generated, so whatever is generating either the file or the definitions can be appropriately adjusted. We had the same thing with an internal team when we were messing around with BCT - ginormous object literals being consumed through the type system, which was just a waste of computer cycles.

@meirgottlieb
Copy link

This was auto-generated based on the FHIR specification. However, it does represent a common pattern in JavaScript. Why not allow properties on object literals when there exists an interface that contains the additional properties and extends the expected interface? As noted by @DanielRosenwasser, this is consistent with how other languages handle interfaces. In the example above, it seems counter intuitive to have to use a type assertion for property a when Derived1 explicitly extends Base.

@danquirk danquirk added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Aug 10, 2015
@benliddicott
Copy link

@DanielRosenwasser , I am pretty sure this is a new message introduced to help catch typos in object literals, which TBH are a big problem for many devs.

Note that you can also resolve your example problem with a cast either on the assignment or within the literal, at the cost of disabling typo checking:

let x: Container = {
  a: <Base>{
    kind: "Derived1",
    prop1: 1000,
  }
}

@DanielRosenwasser
Copy link
Member Author

@benliddicott I understand why it was introduced, and I understand the workaround. As I've stated above, the problem is that for some examples you'll have deeply nested structures of objects which are typed as base-types containing other base-types. which means you'll need not one, but several type assertions.

I mentioned offline with @danquirk and @CyrusNajmabadi that this would not be quite as bad of a problem if we actually pointed out where the excess property was in our errors.

@benliddicott
Copy link

@DanielRosenwasser, Sorry to be so slow on the uptake.

OK so since this works:

let x: Container = {
    a: <Derived1>{
        kind: "Derived1",
        prop1: 1000,
    }
}

That means the compiler is perfectly happy to accept a Derived1 in that position, so it just needs to figure out that it has one without us telling it explicitly. The workaround above goes against the grain of the spellchecker feature though as it kills the spellchecker for the Derived1 portion.

Thinks to self: A union type is like a notional base class which exposes all the members which are common to the types of which it is a union. But: The difference between a union type and a base class, is that a union type places restrictions on the additional members which the type must (or can) have (over the members in the base class).

interface A { a;}
interface B extends A {b;}
interface C extends A {c;}
interface D extends A {d;}

Type B|C is not equivalent to A for the following reasons:

  1. B|C must have either member b or member c, in addition to member a from A. Therefore D is not of type B|C, even though it has all the common members.
  2. Initialising from an object literal, B|C cannot have member d from D. Normally this is OK, but not when initialising from an object literal, because spellchecker.

So I think you are saying in the spellchecking case here, either:

  1. a base class could/should be treated as a union type consisting of the base and every known derived type, OR
  2. A better error message could be given, identifying the location and nature of the error more precisely (and perhaps suggesting the use of a union type)

@DanielRosenwasser
Copy link
Member Author

Right, the issue is there's no way for to say with certainty that it was one or the other; we can't just say that the union of the derived and the base are equivalent.

The drawbacks of specifying your types in a union type is that it's tedious; however, that's just part of the expression problem anyway, and if that's the way the code was structured, we're doing the right thing by reporting an error.

On the other hand, we want to reduce friction and we don't want to make users slog through and re-type everything.

I think we should investigate working on the error message. If we can improve the experience their with little complexity, I would consider that a big win.

@benliddicott
Copy link

On the topic of improving the error messages, It seems like we might be being stricter than really necessary here?

Am I right in saying, the main use-case for this feature is if

  • the user has supplied an additional member which is not expected AND ALSO
  • the user has not supplied at least one optional member
  • so we warn the programmer in case the additional member is a misspelt optional member

If the user has supplied all optional members, is it reasonable for the compiler to assume the additional member is intended, and not generate an error?

In other words does this make sense:

interface A { a?: any; b?: any;}

// We assume c here may be a spelling error for b
// error TS2322: Type '{ a: string; c: string; }' is not assignable to type 'A'.
//  Object literal may only specify known properties, and 'c' does not exist in type 'A'.
var a1: A = {a: "a", c: "c"}

var a2: A = {a: "a", b: "b", c: "c"}// Here we assume c is intended.

// So if we want b to be undefined we can say:
var a3: A = {a: "a", b: undefined, c: "c" }

So maybe the error should be:
Object literal assignment check failed. Property 'c' does not exist in 'A', and optional property 'b' in 'A' is absent.

@RyanCavanaugh
Copy link
Member

If the user has supplied all optional members

One of the (many) bugs Dan found while updating DefinitelyTyped was something like this:

interface PromptOptions {
  message: string;
  modal?: boolean;
}
interface DialogOptions {
  message: string;
  title?: string;
}
declare function prompt(opts: PromptOptions): void;
declare function dialog(opts: DialogOptions): void;

dialog( { message: 'Hello', title, 'World', modal: true });
// Later: tester files a bug saying dialogs aren't modal when they should be

My question is, when someone writes this line of code:

var a2: A = {a: "a", b: "b", c: "c"};

Why did they put a type annotation on a2 ?

If the next use of a2 is in a position where it's going to be assignability-checked vs A (e.g. as a function argument), the type annotation adds very little value (just contextual typing) over letting the type of a2 be inferred.

If they're returning a2 as a function result for a function with an inferred return type, what the heck is c doing in there? The property will never be accessed again in a soundly-typed manner.

If they wrote the type annotation because they want to make sure that they're specifying members of A in the literal, then we ought to reject c.

@benliddicott
Copy link

TL;DR: There are additional use-cases which work in 1.5, therefore you won't have bug reports for them, but I think you will get bug reports in 1.6 if they suddenly don't work

@DanielRosenwasser, maybe you'll like the partial workaround I suggest at the bottom.
@RyanCavanaugh,

Why did they put a type annotation on a2 ?

Good question. In the more usual case:

// Arthur writes:
declare function dialog(opts: DialogOptions): void;
//====
// Brian writes:
dialog( { message: 'Hello', title, 'World', modal: true });

Given that Arthur wrote the annotation, what does Brian want to happen now?

Here's some possible answers, and I stress these are related but separate features - any of them could in theory be provided without the others. Brian wants:

  1. IntelliSense to remind him of the available options (in theory JSDoc could do this even without typescript, in any case works in 1.4)
  2. required options to be enforced (This works in 1.4 already)
  3. misspelt optional parameters to be warned of (My suggestion would this)
  4. meaningless extra parameters to be warned of. (your example above).

Here's some additional cases I think Brian (and TypeScript) would also benefit from accommodating. (These work in 1.5, therefore you won't have bug reports for them, but I think you will get bug reports in 1.6 if they suddenly don't work):

  1. Incomplete typings: Boris Yankov is very industrious but cannot possibly keep up with every JS library out there. If express/trimble@1.9.3344 adds a new option maxEigenstate, it may take some time for it to appear in tsd update trimble. What to do?
    • Must the developer fork DefinitelyTyped and keep trimble.d.ts up to date? That seems like a lot of work just to use a minor option.
    • They could use a cast, but then they lose benefits 1, 2 and 3 above.
  2. extensible configuration. Suppose express/trimble has an extension mechanism, spinners:
import trimble= require("trimble");
import spinup = require("trimble-spinup");
import spindown = require("trimble-spindown");

// Error: spinUpEigenstate is not a member of Trimble.Options
server.add(trimble.spin("up", {spinFaster: false; spinUpEigenstate: 42;})};

There is no practical way to write a typing which will accommodate that.

  • You could write the typing with a union type, but when somebody writes there own express\trimble-boson library, they have to put the options into express\trimble. This seems impractical.
  • You could use a cast, but then you lose the benefit of the object-literal-spellcheck feature - that is benefits 1, 2 and 3 above.
  • You could put the options into a temporary:
var spinUpOptions: SpinUp.Options = {spinFaster: false; spinUpEigenstate: 42;};
server.add(trimble.spin("up", spinUpOptions));
  • you could use this workaround below.

Workarounds
There's a workaround for problem 2 : check the literal for a different type than the parameter expects:

/** use to trigger object literal type checking for a particular type */
function check_literal<T>(t:T): T {return t;}

// If `SpinUp.Options` implements `Trimble.Options` no cast is necessary. 
// We still get the benefit of strict object literal checking
server.add(trimble.spin("up", 
    check_literal<SpinUp.Options>(
        {spinFaster: false; spinupEigenstate: 42; }
    ));

The workaround for incomplete typings would be similar but so much more long winded as to be impractical:

// This typing uses an anonymous type, because this is the only place it is used.
declare function doSomething(options: {
option1: number; option2: number;
dodadOptions: {
   dodadType: string;
   message: string;
   widgetInfo: {
      widgetSize: {
         w:number;
         h: number;
      }
   }
}
});

Now imagine a new option depth for options.doodadOptions.widgetInfo.widgetSize is added, but hasn't made it into DefinitelyTyped yet. Now imagine it is a required option.

@RyanCavanaugh
Copy link
Member

@benliddicott

If express/trimble@1.9.3344 adds a new option maxEigenstate, it may take some time for it to appear in tsd update trimble. What to do?

I agree this is a problem, but it is not a new problem. If someone adds a new overload, or adds a member to a type that you want to access via x.newProp, or adds a new function, or makes a required property optional, etc, etc, etc, then TypeScript isn't going to let you get away with it without updating your definition file.

A major forcing function for .d.ts files to become correct is to make them cause undesired errors when they are wrong!

extensible configuration.

In the case of extensible configuration, the config object should have a string indexer. That's new guidance, but is fairly simple to explain and understand (if you pass your config to someone else, have an indexer, otherwise don't).

@jbondc
Just scanning through your error list, I don't think the first error visible there is related to this change. I'd like to to see an isolated repro if you're able to provide it. The second error is a bug in jquery.d.ts, I believe -- it should have declared the processData member! Third error, I have no idea what that is. The fourth error is, again, looks like a bug in a .d.ts file (the onClick member presumably should exist on ModalButton) ?

@JsonFreeman
Copy link
Contributor

You can infer the string index for an untyped object literal by looking at assignments.

It is not feasible to allow the type of a variable to change based on subsequent mutations of that variable. In your example, you could assign opt to something else, and we would be unable to track what happens to it.

It is also not desirable, as there is nothing that checks whether future assignments are even correct.

just not on by default

The feature cannot be user configurable because it is an assignability rule. Any assignability rule can affect overload resolution, and the user should not have fine grained control of overload resolution semantics.

@benliddicott
Copy link

TL;DR: It would be nice to have some way for the caller to relax strict type checking at the call site, so pain points can be reduced on a case by case basis. I suggest a clumsy workaround below. I would also like a broader feature of loose and strict type assertions.

@RyanCavanaugh

A major forcing function for .d.ts files to become correct is to make them cause undesired errors

The problem with applying pressure on library authors in this way is that you first have to hurt the library users, and hope that they pass the pressure on. Library users are the people who decide whether or not to use TypeScript vs. JavaScript.

@JsonFreeman the user already has control over overload resolution because they can use a cast. What they can't do is use a cast and also get the benefit of object literal spellchecking.

Here's the problem: How to a) get the benefits of intellisense and required member checking and also b) avoid the drawbacks of generating errors for additional members where this is desired (which is not all of the time). This can be done using the workaround below, but it is clumsy.

Workaround

Here's a workaround for library users, which uses intersection types.

What's good about it:

  • you get to keep intellisense,
  • keep checking for required members,
  • while also allowing additional members to be specified.

What's bad about it:

  • Clumsy hack, clumsy syntax,
  • generates a do-nothing function call in output js.
    /** Triggers typescript@1.6 to check an object literal with strict rules 
     * (no additional members) 
     */
    function check_literal<T>(t: T): T { return t; }

     /** Triggers typescript@1.6 to check an object literal with loose rules 
      * (additional members permitted) 
      */
   function check_loose<T>(t: (T&{ [name: string]: any })): T {return t;}

    // Usage:
    interface A { a?: any, b?: any};
    interface B { a: any, b: any};
   var a1 = check_loose<A>({a: "", c: ""}); // No error
   var a2: B = check_loose<typeof a2>({a: "", c: ""}); // Error, missing member b.
   var a3: B = check_literal<typeof a2>({a: "", b: "", c: ""}); // Error, additional member c

Type Assertion

Better would be a "type assertion" specifically intended to have these effects. E.g. to trigger loose type checking where strict would normally be used. A loose type assertion would generate an error if value is not of type A, but would not generate an error if it had additional members.

/** Loose type assertion. Must implement typeof(a) 
More or less equivalent to var a: A = check_loose<typeof a>(value)
*/
var a: A = <+>value;

/** Loose type assertion. Must implement A */
var a = <+A>value;
/** Loose type assertion. Must implement A or B*/
var a = <+A|B>value;

Strict type assertion. Performs strict type checking of the RHS. The RHS need not be an object literal.

/** Strict type assertion: Must implement A, and must not have additional members
<=T> might be a better mnemonic, but more confusing to read.
*/
var a = <^A>value;

@JsonFreeman
Copy link
Contributor

@jbondc I'm not sure what rule you have in mind, but it does not seem sensible to special case assignability for overload resolution.

@benliddicott If I understand you correctly, you are saying that the purpose of a type assertion expression has nothing to do with the checking of excess properties, so why mix the two concepts? You want the ability to control whether an excess property check is required, independently of whether a type assertion is present.

But I disagree, I think that the essence of a type assertion does have to do with omitting this check. Specifically, a type assertion can be viewed as an expression level operation that changes the type of an expression. It disrupts the normal flow of types. Specifically, when you actively change the type of an expression, you are likely to add properties not present in the operand (downcast), or forget properties present in the operand (upcast). An upcast would be absolutely useless if it checked for excess properties because forgetting these properties is the sole purpose of the upcast.

I think allowing the user to explicitly opt into / out of the check is excessive control. To make an analogy, I view this as similar to allowing users to toggle whether assignabilty or subtype is used in certain situations. Users are not generally aware of the rationales / implications of which relation is chosen, and that is why they are never allowed to toggle it. In the same way, I think this check should be embedded into the language in a way that makes sense with the rest of the language.

@benliddicott
Copy link

@JsonFreeman

allowing the user to explicitly opt into / out of the check is excessive control. ... similar to allowing users to toggle whether assignabilty or subtype is used in certain situations. ... [Users] are never allowed to toggle it.

_Every word of that is untrue._ We've shown multiple examples where this control is needed and not excessive. The user can already opt out and toggle of the check at will: <MyType><any>value. You know this!!!! And I just showed you how to opt back in and choose assignability over subtype or vice versa. You seem to be talking about a completely different language. Maybe you are thinking of ADA?

My point is to give the user additional options to allow them to work within the actually existing type system, where types as declared don't always correspond to the actualité. The user does not care to be used as an instrument to apply pressure on library authors. They just want to say "I don't want a strict type check here godammit!!!" They need a way to say that. They have a way - a cast - which throws the baby out with the bathwater. They would like a better way.

Here's an anecdote: In the .Net 1.1 world, the PrintFileName member of PrinterSettings wasn't exposed. There may have been a reason for this related to the security model, but I rather think it was just because the library authors thought it wasn't necessary. So we had to use reflection to access it. (This was in the days when the way to generate a PDF was to use the Microsoft Postscript Driver and then GhostScript PS2PDF).

Point: You always, always need an escape hatch. Users need options. Don't try to fit them into a type-correct straitjacket because you will never understand all of their requirements - how could you after all?: They outnumber you a thousand to one (if you fail! if you succeed it will be a million to one.) They fixed it in 2.0 - there were a bunch of things we needed which were fixed such as the drop-highlight on a treeview. But we had an escape hatch: We could always work around it with P/Invoke and reflection.

Options: Some things should be easy, some should be hard, some should be in between. But nothing should be impossible.

@danquirk
Copy link
Member

You always, always need an escape hatch.

Not disputing other points but to be clear here there are escape hatches in the current proposal/implementation. Adding an indexer to the extensible type or using an appropriate cast is your escape hatch. That may be too fine grained for some (ie it may require many code changes at each offending location) but it is precisely analogous to fixing all your broken .NET calls to manually use Reflection/PInvoke at all call sites that require it.

@JsonFreeman
Copy link
Contributor

Yes, I agree that an escape hatch is needed, but I don't see why a type assertion is an insufficient escape hatch. You mention throwing the baby out with the bathwater, but what is the baby here? Type assertions are specifically designed to be an escape hatch for various things.

@RyanCavanaugh
Copy link
Member

But nothing should be impossible.

Using examples, what specifically is impossible right now?

@benliddicott
Copy link

@danquirk adding an indexer to an extensible type is something the library author can do, it isn't an escape hatch for the library user. These are two different people. The baby-with-bathwater escape hatch for the user is a cast where:

  • Baby = would like to keep = ability to type-check and spell-check object literals.
  • Bathwater = would sometimes like to lose = false-positive errors on additional members

I'm not saying there's no workaround, I'm suggesting this is going to be a common need so why not make it nicer.

@RyanCavanaugh The only thing I know of that's impossible is you can't implement as an interface any class which has a private member, #471. There are workarounds for pretty much everything else but with varying degrees of ugliness. #4278, #3854. Point: Workarounds are a commonplace need in JavaScript and shouldn't be so horrid.

Finally: I like TypeScript - a lot - or I wouldn't be doing this at all!!

@danquirk
Copy link
Member

The library user would cast as necessary. The library author (or library user depending on the circumstances) could augment types with indexers as necessary.

@JsonFreeman
Copy link
Contributor

@benliddicott Maybe I'm missing some context, but what is the spell-check that you mentioned? The check for excess properties behaves in a sense like the spell check, but that is the only known way of spell checking your properties.

Also, type assertions are indeed type checked (using assignability in both directions), so you do not sacrifice any static type checking by including a type assertion.

@DanielRosenwasser
Copy link
Member Author

From what I've seen in updating DefinitelyTyped, the impact was mainly positive. I'm going to close this issue for now.

@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
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants