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

Error when 'extra' properties appear in object literals #3755

Closed
RyanCavanaugh opened this issue Jul 6, 2015 · 34 comments
Closed

Error when 'extra' properties appear in object literals #3755

RyanCavanaugh opened this issue Jul 6, 2015 · 34 comments
Labels
Breaking Change Would introduce errors in existing code Fixed A PR has been merged for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@RyanCavanaugh
Copy link
Member

What's the Change?

Old Behavior

Prior to 1.6, TypeScript didn't do a good job detecting problems in object literals, especially when the property you tried to specify was optional:

interface TextOptions {
    alignment?: string;
    color?: string;
    padding?: number;
}
function drawText(opts: TextOptions) { ... }

// None of these were errors before
drawText({ align: 'center' }); // Oops
drawText({ colour: 'grey' }); // Oops
drawText({ pading: 32}); // Oops

New Behavior

As of 1.6, properties in object literals that do not have a corresponding property in the type they're being assigned to are flagged as errors:

drawText({ align: 'center' }); // Error, no property 'align' in 'TextOptions'

But I meant to do that

There are a few cases where you may have intended to have extra properties in your object. Depending on what you're doing, there are several appropriate fixes

Type-checking only some properties

Sometimes you want to make sure a few things are present and of the correct type, but intend to have extra properties for whatever reason. Type assertions (<T>v or v as T) do not check for extra properties, so you can use them in place of a type annotation:

interface Options {
    x?: string;
    y?: number;
}

// Error, no property 'z' in 'Options'
let q1: Options = { x: 'foo', y: 32, z: 100 };
// OK
let q2 = <Options>{ x: 'foo', y: 32, z: 100 };
// Still an error (good):
let q3 = <Options>{ x: 100, y: 32, z: 100 };

These properties and maybe more

Some APIs take an object and dynamically iterate over its keys, but have 'special' keys that need to be of a certain type. Adding a string indexer to the type will disable extra property checking

Before

interface Model {
  name: string;
}
function createModel(x: Model) { ... }

// Error
createModel({name: 'hello', length: 100});

After

interface Model {
  name: string;
  [others: string]: any;
}
function createModel(x: Model) { ... }

// OK
createModel({name: 'hello', length: 100});

This is a dog or a cat or a horse, not sure yet

interface Animal { move; }
interface Dog extends Animal { woof; }
interface Cat extends Animal { meow; }
interface Horse extends Animal { neigh; }

let x: Animal;
if(...) {
  x = { move: 'doggy paddle', woof: 'bark' };
} else if(...) {
  x = { move: 'catwalk', meow: 'mrar' };
} else {
  x = { move: 'gallop', neigh: 'wilbur' };
}

Two good solutions come to mind here

Specify a closed set for x

// Removes all errors
let x: Dog|Cat|Horse;

or Type assert each thing

// For each initialization
  x = <Dog>{ move: 'doggy paddle', woof: 'bark' };

This type is sometimes open and sometimes not

A clean solution to the "data model" problem using intersection types:

interface DataModelOptions {
  name?: string;
  id?: number;
}
interface UserProperties {
  [key: string]: any;
}
function createDataModel(model: DataModelOptions & UserProperties) {
 /* ... */
}
// findDataModel can only look up by name or id
function findDataModel(model: DataModelOptions) {
 /* ... */
}
// OK
createDataModel({name: 'my model', favoriteAnimal: 'cat' });
// Error, 'ID' is not correct (should be 'id')
findDataModel({ ID: 32 });
@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Jul 6, 2015
@danquirk
Copy link
Member

danquirk commented Jul 6, 2015

👍

I would note that traditionally we have tried to keep interfaces and type literals essentially the same from a semantic point of view, which would argue in favor of allowing strict on type literals. I can imagine people wanting to do this on single use options bags with init kind of functions:

function initProgram(url: string, port: number, opts: strict { debugMode: boolean; perfTraces: boolean });

where typeof opts is never re-used so they didn't feel the need to create a named type just for the options.

@JsonFreeman
Copy link
Contributor

Yeah I agree with @danquirk

@ahejlsberg
Copy link
Member

It feels like overkill to introduce a whole new kind of type to solve a very confined problem. There are some really counter-intuitive effects around extending and implementing these things, and it isn't clear to me that they could work as type arguments without some sort of "strict" constraint, adding even more complexity.

Perhaps we should consider a simpler and more targeted solution. For example, how about saying that a non-empty type S is not assignable to a type T containing only optional properties if S contains no properties from T. It appears to me this would solve the core problem.

@JsonFreeman
Copy link
Contributor

In particular, I have discussed with @RyanCavanaugh and it seems that strict interfaces have a really weird interaction with intersection types.

@RyanCavanaugh
Copy link
Member Author

It's not just all-optional types, though. Adding one required property to TextOptions only fixes some of the problems:

interface TextOptions {
    text: string;
    alignment?: string;
    color?: string;
    padding?: number;
}
function drawText(opts: TextOptions) { ... }

// No error
drawText({ text: 'foo', align: 'center'} );

// No error
function getDefaultOptions(): TextOptions {
    return { text: 'foo', colour: 'blue'; };
}

// No error
drawText(getDefaultOptions());


// No error
function getDefaultOptions2() {
    return { text: 'foo', colour: 'blue'; };
}
drawText(getDefaultOptions2());

@ahejlsberg
Copy link
Member

@RyanCavanaugh I guess there are two ways things go wrong with types that have only optional properties. One is when you accidentally pass a value of a completely unrelated type (i.e. no overlap whatsoever in property names). Another is when you accidentally misspell a property name (but there is some overlap in the remaining properties). I'm not sure which is the more common failure mode, but I have a hunch it is the first one. At least most, if not all, of the issues you link to have examples that fall into that category. So, I'm wondering if we can get there (or get close enough) with just a simple rule.

@JsonFreeman
Copy link
Contributor

It wouldn't be a new kind of type though. It would just be an attribute on an object type that says being assignable to it entails having no extra properties.

That said, it violates the assumption that providing more information about a type makes the type more specific. Our type system assumes that, especially with the new intersection rules.

@RyanCavanaugh
Copy link
Member Author

I'm not sure which is the more common failure mode, but I have a hunch it is the first one.

I think the property names in these examples are only disjoint because people are simplifying their code down to the minimum number of properties required to demonstrate the problem.

In practice, it's very easy to find code in the wild that typos a property name to e.g. $.ajax while getting other property names correct. Specifying timeOut instead of timeout or onfailure instead of onFailure is the kind of thing that slips by casual testing very easily and ought to be caught by a tool that claims to find errors in code.

Arguably we provide more value by catching errors in the non-disjoint case. Providing a completely-wrong object (e.g. forgetting to invoke a function) is usually caught at runtime because it just breaks completely (e.g. no url property means the call fails outright). Having 4 out of 5 properties be correct usually means your code is going to work, but with slightly different semantics than you intended.

Regarding all-optionality, we even have problems with object literals in our own compiler satisfying interfaces with no optional properties at all. There are places where we build an object literal to match some interface (e.g. SymbolDisplayBuilder), but end up including extra things in the object that aren't part of the target type and could have been safely removed. It's not an error per se (it's harmless outside of unmeasurable perf impact), but makes it more difficult for us to identify and remove dead code.

@ahejlsberg
Copy link
Member

Ok, here's another idea...

An interesting fact about object literals (and array literals) is that an object reference produced by a literal is known to be the only reference to that object. Thus, when an object literal is assigned to a variable or passed for a parameter of a type with fewer properties than the object literal, we know that information is irretrievably lost. This is a strong indication that something is wrong--probably stronger than the benefits afforded by allowing it.

Now, there are obviously scenarios where this loss of information is expected, for example when a function dynamically inspects an object passed to it (e.g. when object literals are used as maps). But I think we can recognize these situations: The target type would be any, Object, {}, or a type that includes a string or numeric index signature. I think it is highly unlikely to have a target type with actual declared properties and have loss of information be an expected scenario.

So, imagine that an object literal's type is considered "fresh" within the expression in which it occurs, and that we consider it an error to knowingly loose type information from a "fresh" type when the target is not one of the "dynamic" types listed above. This would not only catch the motivating scenarios listed in the introduction, but also the problem Ryan mentions above (where there are no optional properties). Note, BTW, that freshness would be similar in nature to widening of null and undefined: A type would loose it's freshness when we widen it.

@RyanCavanaugh
Copy link
Member Author

A while back I had a prototype that flagged errors on surplus properties on object literals when those literals were contextually typed by a non-empty object type. That seems very close to what you're describing (in terms of behavior I can't identify a difference).

This actually worked quite well. The only meaningful false negative was in one of the RWC suites because someone had a thing like this:

interface Book {
    name: string;
}
interface AudioBook extends Book {
    duration: number;
}
interface PhysicalBook extends Book {
    pages: number;
}

let books: Book[] = [
    { name: 'A', duration: 100 },
    { name: 'B', pages: 40 }
];

This code is only 'correct' under human inspection; I don't see any way to distinguish it from an error since we don't know the universe of Book subtypes. And someone writing this code today might write let books: Array<PhysicalBook|AudioBook> instead, or define interface AnyBook extends Book { [extras: string]: any; }, so it seems like an acceptable break in the name of finding so many other errors.

We'd have to combine that with the "all-optional types require at least one match" rule in order to catch drawText(getDefaultOptions); ?

@ahejlsberg
Copy link
Member

Yes, using the contextual type as you describe does indeed seem like it would have the same effect. All up I much prefer this targeted approach to introducing full blown "strict" interfaces.

Not sure about the "all-optional types require at least one match" rule if we do this. I think we should evaluate that one separately.

@corps
Copy link

corps commented Jul 8, 2015

In my experience, this tends to be an exceptional case around interfaces with optional parameters, where as the laxness of type interfaces is super useful most of the time. Golang has a similar problem with their interfaces, and suggests introducing fields to distinguish types if you really need it, although their interface semantics don't have optional attributes in the same way.

Some poor-man's solutions without introducing more language constructs:

interface TextOptionsBag {
    alignment?: string;
    color?: string;
    padding?: number;
}

export interface TextOptions extends TextOptionsBag {
    _isTextOptions: boolean;
}

export function textOptions(options: TextOptionsBag) { return <TextOptions>options };

You could even use a class with a constructor that uses bindings as the interface definition while avoiding excessive optional attributes. Most of the time, optional members on an interface exist to support one main use case: functions with optional parameters.

@JsonFreeman
Copy link
Contributor

@ahejlsberg It is interesting that you are talking about a rule involving optional properties with fresh object types. We have something that works like this today in the subtype relation (but not assignability). Specifically, an unwidened object literal type is not a subtype if it is missing an optional property in the target. My point is that we already have freshness detection for object literals. So we'd just have to add the rule.

@corps Optional parameters have nothing to do with optional properties really. So I wouldn't worry about any loss of expressiveness for optional parameters as a result of this discussion.

@corps
Copy link

corps commented Jul 8, 2015

@JsonFreeman

That's not quite what I mean. As per the top example, interfaces with optional properties are generally used to express optional configuration as a parameter. This is, of course, my experience. An interface's property should usually not be optional unless you are saving the effort of creating object literals with lots of properties (often the case of, say $.ajax({ blah: "foo" }) or draw({ notAllOfMyOptions: "abc" }). So my point was merely to consider what other solutions can help this main use case if an elegant typing solution was not found.

Just one user's concern, since 1.5 typescript is already heavy with new typing features around ES6, etc.

@RyanCavanaugh
Copy link
Member Author

Implemented both ideas, separately.

Referring to all-optional types as 'weak'. This found one real bug in our compiler! master...RyanCavanaugh:weakType

I implemented the extra properties in object literals using contextual typing since I was already familiar with that approach. This found 10 different cases with properties that were not declared.
master...RyanCavanaugh:noSurplus

@JsonFreeman
Copy link
Contributor

I like the weak types approach better. I think we have a pretty nice property that contextual typing cannot directly cause errors - we'd lose that with the second approach.

@ahejlsberg
Copy link
Member

I've been playing with an implementation of surplus property detection too. It finds exactly the same errors. I initially used the contextual type as you suggested, but it turns out that approach doesn't play well with overload resolution because it reports errors too early. For example:

declare function foo(x: { a: number }): void;
declare function foo(x: { a: number, b: number }): void;
foo({ a: 1, b: 2 });

This reports an error while attempting the first overload, and then succeeds on the second overload. Basically, the rule of thumb is that contextual types can't be the source of errors.

I have now switched to an approach that tracks "freshness" and detects surplus properties in assignment compatibility checks. It also covers some further subtleties around union and intersection types. I will put up the code as soon as I can.

@ahejlsberg
Copy link
Member

@RyanCavanaugh @JsonFreeman I've put up my code in #3823.

@NoelAbrahams
Copy link

Very happy to see progress on this.

@danquirk danquirk added the Breaking Change Would introduce errors in existing code label Jul 13, 2015
@Eisenspalter
Copy link

I don't think, that there is a progress. Strictness does not depend on optional properties. The decision for strictness can only made by the developer. So there is no alternative way for a clean implementation without a keyword like strict.
Currently I implement hundreds of interfaces for an large SPA with nodejs backend. The main goal to implement Typescript, is the ability to check for typos.

The first example need strict. It avoid typos for params.

strict interface IGetProducts {
    dbURL: string,
    collectionName: string;
    filter: {};
    skip?: number,
    limit?: number
}

function getProducts(params: IGetProducts) {
    // ...
}

getProducts({
    dbURL: '//demodb',
    collectionName: 'products',
    filter: { category: 'book' },
    skip:0,
    limi: 20 // strict can avoid this typo 
});

In the second example strict is not welcome, because the data are from a schemaless NoSQL database.

interface IProduct {
    id: string,
    title: string;
    color: string;
    description?: string;
}

// data from a database, I do not need strict
var product: IProduct = {
    id: '1',
    title: 'table tennis rubber',
    color: 'red',
    userProperty : 'bla bla bla ' 
}

I <3 Typescript.

@JsonFreeman
Copy link
Contributor

@Eisenspalter, if the data in the second example is from a database, then why is it an object literal?

@Eisenspalter
Copy link

json based like mongodb

@JsonFreeman
Copy link
Contributor

Yes, but if you get it from a database, it won't be a literal expression. It's only a literal expression if you define it yourself.

@Eisenspalter
Copy link

Sorry, the second example is nonsense. The first one is important

@JsonFreeman
Copy link
Contributor

But in the first one, we will indeed check for excess (misspelled) properties with change #3823. So the behavior will be what you want.

@NoelAbrahams
Copy link

@JsonFreeman,

Regarding the actual implementation by @ahejlsberg, if I'm correct the following won't be an error:

interface Foo {
  daiquiri?: string;
}

function getFoo(params: Foo) { }

getFoo({ daquiri: 'a bit rum' }); // No error

I guess the solution to get the additional checks is the following:

type Foo  = {
  daiquiri?: string;
}

function getFoo(params: Foo) { }

getFoo({ daquiri: 'a bit rum' }); // Error, excess property `daquiri`

Have I understood the implementation correctly? Or will types defined via interfaces be also checked?

@ahejlsberg
Copy link
Member

@NoelAbrahams Both will be errors with #3823. It doesn't matter how the target type is defined. The check is simply that the source is an object literal, the target is a non-empty type, and the source specifies one or more properties that don't exist in the target.

@NoelAbrahams
Copy link

@ahejlsberg,

Perfect! This is quite an elegant solution to having to worry about whether one had annotated an interface with strict or not. I'm happy to take this breaking change as default behaviour. I agree that the only breaks the compiler is going to discover are most likely bugs.

@ahejlsberg
Copy link
Member

Closed in #3823.

@RyanCavanaugh RyanCavanaugh changed the title Strict Interfaces Error when 'extra' properties appear in object literals Jul 21, 2015
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Aug 6, 2015
grahammendick added a commit to grahammendick/navigation that referenced this issue Dec 5, 2015
Typescript 1.6 tightened up object literal interface validation
(microsoft/TypeScript#3755). Added string
indexer to disable checking for StateInfo config interfaces
@Ciantic
Copy link

Ciantic commented Mar 16, 2016

This is now working when doing assignment only, would it be bad if it worked always when object literal is written? E.g. when returning object literal in callbacks. See my issue: #7547

Edit I think this is a bug. If it should work when object literal is typed, but it's not working with callbacks.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 16, 2016

@Ciantic, this is the intended design. maintain the "freshens" of an object literal on types would be a large breaking change.

@Ciantic
Copy link

Ciantic commented Mar 16, 2016

@mhegazy are you sure?

interface Test {
    name?: string;
}

function myMutator(fn: () => Test) {
    return fn();
}

/* Not an error */
let a: Test = myMutator(() => ({
    notAProperty : "woot?"
}));

/* This gives error */
function test(): Test {
    return {
        notAProperty : "what"
    }
}

/* This gives error */
let b: Test = {
    notAProperty : "huh"
};

See also @RyanCavanaugh comment on #7547

@mhegazy
Copy link
Contributor

mhegazy commented Mar 16, 2016

here is a simpler example.

var x = { unknown: 42 };
var y: Test = x; // OK

@mhegazy
Copy link
Contributor

mhegazy commented Mar 16, 2016

This is an old issue, please keep the discussion in #7547

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code Fixed A PR has been merged for this issue In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

9 participants