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

Strict object literal assignment checking #3823

Merged
merged 23 commits into from Jul 21, 2015

Conversation

Projects
None yet
@ahejlsberg
Member

ahejlsberg commented Jul 11, 2015

This PR implements stricter object literal assignment checks for the purpose of catching excess or misspelled properties. The PR implements the suggestions in #3755. Specifically:

  • Every object literal is initially considered "fresh".
  • When a fresh object literal is assigned to a variable or passed for a parameter of a non-empty target type, it is an error for the object literal to specify properties that don't exist in the target type.
  • Freshness disappears in a type assertion or when the type of an object literal is widened.

Some examples:

var x: { foo: number };
x = { foo: 1, baz: 2 };  // Error, excess property `baz`

var y: { foo: number, bar?: number };
y = { foo: 1, baz: 2 };  // Error, excess or misspelled property `baz`

The rationale for the errors above is that since the fresh types of the object literals are never captured in variables, static knowledge of the excess or misspelled properties should not be silently lost. No errors occur when the fresh types are captured in variables:

var x: { foo: number };
x1 = { foo: 1, baz: 2 };
x = x1;

var y: { foo: number, bar?: number };
y1 = { foo: 1, baz: 2 };
y = y1;

A type can include an index signature to explicitly indicate that excess properties are permitted:

var x: { foo: number, [x: string]: any };
x = { foo: 1, baz: 2 };  // Ok, `baz` matched by index signature

UPDATE 1: This PR also tightens certain parts of the implementation of union types. Specifically:

  • We no longer perform subtype reduction on union types but instead perform a non-lossy deduplication of the constituent type set.
  • The process for determining call and constuct signatures of a union type is now less restrictive.
  • Similar to intersection types, union types now preserve order of the constituent types.

These changes are detailed in the comment thread below.

UPDATE 2: The -suppressExcessPropertyErrors compiler option can be used to disable strict object literal assignment checking. See #4484.

}
class ActionB extends Action {
trueNess: boolean;

This comment has been minimized.

@RyanCavanaugh

RyanCavanaugh Jul 11, 2015

Member

I think this should be trueness -- it doesn't look like this test was intended to fail. Another 🏆 for this change's effectiveness.

@RyanCavanaugh

This comment has been minimized.

Member

RyanCavanaugh commented Jul 11, 2015

👍 . Pulled it down and tried it out for a while -- behavior seems perfect.

I was trying to find an idiomatic way to remove freshness without casting to any. Going through function identity<T>(x: T): T works but has some runtime impact. Any ideas?

@ahejlsberg

This comment has been minimized.

Member

ahejlsberg commented Jul 12, 2015

The only way to remove freshness that doesn't generate additional code is a type assertion. I think this is consistent with other situations in which you want to override the type checker. I'm not sure there is a need for a special idiom to defeat the checks associated with freshness.

@JsonFreeman

This comment has been minimized.

Contributor

JsonFreeman commented Jul 13, 2015

I agree that a type assertion is a reasonable escape hatch. You also do not have to cast to any, you can cast to a better type if you have one.

@@ -1743,10 +1743,12 @@ namespace ts {
FromSignature = 0x00040000, // Created for signature assignment check
ObjectLiteral = 0x00080000, // Originates in an object literal
/* @internal */
ContainsUndefinedOrNull = 0x00100000, // Type is or contains Undefined or Null type
FreshObjectLiteral = 0x00100000, // Fresh object literal type

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Please define fresh object literal in the comment.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Why do we need a new flag here? This is set in the same place as ObjectLiteral. And when you get the regular type, you turn off FreshObjectLiteral, but not ObjectLiteral, and I don't really understand why. So a regular type is allowed to have excess properties, but it still does not need to have all the optional properties of the target in a subtype check?

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

Well, it's related to the issue of removing freshness after the first assignment, but still remembering that the source was an object literal. It may be that we can combine the two if we give up on the first assignment bit.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Yup, I think we should give up the first assignment bit. I think it is a strange rule.

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

So, I don't think we can get rid of the new flag. We use TypeFlag.ObjectLiteral to indicate that a type originated in an object literal and may contain null or undefined types. We can't turn off that flag unless we also widen the type. Yet, in type assertions and during subtype reduction we want to turn off freshness but not widen.

That said, we could still drop the assignment rule. They're really orthogonal issues.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 14, 2015

Contributor

Yes, let's drop the assignment rule.

I understand what you mean about TypeFlags.ObjectLiteral, but I still think we can use it. We actually use ContainsUndefinedOrNull and ContainsObjectLiteral for what you are talking about, not ObjectLiteral directly. For type assertions, I think it's fine to widen, we already do in the downcast direction, and I think it's fine to do it for the upcast (it uses assignability so we should be fine). For creation of a union type, we already give it TypeFlags.Union plus all the widening flags (which do not include ObjectLiteral anyway). So all the flags that getWidenedType checks for would still be there.

So I think it is safe to remove it.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 14, 2015

Contributor

To elaborate about the type assertion case, widening would essentially do two things:

  1. null and undefined become any. They are bottom types anyway, so changing them to any should have no effect.
  2. The object literal is changed with respect to the optional-properties-being-required rule. This rule only applies to subtype, and type assertions use assignability.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 14, 2015

Contributor

I admit it's still possible that I missed something, but if I did, I'd like to understand it before allowing us to have two flags that sound really similar and are easy to confuse.

@@ -1839,6 +1841,11 @@ namespace ts {
numberIndexType?: Type; // Numeric index type
}
/* @internal */
export interface FreshObjectLiteralType extends ResolvedType {
regularType: ResolvedType; // Regular version of fresh type

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Does regular mean not fresh? Are they opposites?

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

They are exactly the same, except the TypeFlags.FreshObjectLiteral flag is set in the fresh version.

@@ -9544,7 +9604,7 @@ namespace ts {
return getUnionType([leftType, rightType]);
case SyntaxKind.EqualsToken:
checkAssignmentOperator(rightType);
return rightType;
return getRegularTypeOfObjectLiteral(rightType);

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Why does an assignment expression give the regular type of the right as opposed to the original type? I thought the type that gets assigned to the left is the regular type, but the type of the expression should be the original right type.

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

I added this because of code similar to the following in tsserver:

interface A { a: number }
interface B extends A { b: number }
var last: B;
function foo(): A {
    return last = { a: 1, b: 2 };
}

Once the object literal is successfully assigned to a variable it seems pedantic to insist that b is an unknown property. And, really, following an assignment, the object literal isn't "fresh" anymore.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

My reasoning was that assigning it to last only renders the object "not fresh" if you refer to it via last. In this case, you are still returning the object literal, and it has excess properties with respect to A.

Maybe we could rationalize it by saying that if we tried to assign it to last and it had excess properties relative to last, then we would have already given an error. But then again, this seems weird when you consider overload resolution.

Using the same A and B that you've defined:

declare function foo(param: A): A;
declare function foo(param: B): B;
var last: B;

foo({a: 1, b: 2 }); // returns B
foo(last = {a: 1, b: 2 }); // returns A

I don't think taking the regular type here makes sense.

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

Hmm. Contextual typing only comes from the first assignment target, so we're currently consistent with that. I suppose you could argue we should check against a union of all assignment targets, i.e. as long as each property is known in some assignment target we're good. But that would add a bunch of complexity that I don't think is justified.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

I don't think we would need to explicitly check against the union of all assignment targets. Each assignment target does an assignability check, and if a certain source type passes all the assignability checks, it's good. So the correct behavior should just fall out if we remove the call to getRegularTypeOfObjectLiteral, no?

It's true that contextual typing only comes from the first target, but I'm not sure why contextual typing is relevant. We chose to build this check into a mechanism other than contextual typing, so I don't see why we would consider contextual typing a factor here.

@@ -4499,7 +4525,19 @@ namespace ts {
if (source === numberType && target.flags & TypeFlags.Enum) return Ternary.True;
}
}
if (relation === assignableRelation && source.flags & TypeFlags.ObjectLiteral && source.flags & TypeFlags.FreshObjectLiteral) {

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Why check ObjectLiteral if all FreshObjectLiterals are ObjectLiterals? In fact, why not make the FreshObjectLiteral mask include the ObjectLiteral mask?

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

It seems strange that we only do this for assignability. Prior to this, every pair of types that return true for subtype also return true for assignability. That is no longer true, and in fact, you will observe this in overload resolution.

interface MyObject {
   prop1: string;
}
declare function foo(param: MyObject): any;

foo({ prop1: "", prop2: "" }); // fails

This will fail, but if you add a second overload, it suddenly succeeds:

declare function foo(param: MyObject): any;
declare function foo(param: string): any;

foo({ prop1: "", prop2: "" }); // succeeds

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

Yeah, TypeFlags.FreshObjectLiteral already implies both, so it's enough to just check for that.

Agreed, we should do the check for subtype as well.

@@ -5255,6 +5297,24 @@ namespace ts {
return (type.flags & TypeFlags.Tuple) && !!(<TupleType>type).elementTypes;
}
function getRegularTypeOfObjectLiteral(type: Type): Type {

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

I would just make this take a FreshObjectLiteralType

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

No, the point of the method is to remove "freshness" from the type regardless of the kind of type. I suppose we could call it getRegularTypeOfType.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Okay, that sounds fine

@@ -4499,7 +4525,19 @@ namespace ts {
if (source === numberType && target.flags & TypeFlags.Enum) return Ternary.True;
}
}
if (relation === assignableRelation && source.flags & TypeFlags.ObjectLiteral && source.flags & TypeFlags.FreshObjectLiteral) {
if (hasExcessProperties(<ObjectType>source, target, reportErrors)) {

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Cast to FreshObjectLiteralType

This comment has been minimized.

@ahejlsberg
}
return Ternary.False;
}
source = getRegularTypeOfObjectLiteral(source);

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Why is it necessary to do this?

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

Because we otherwise will fail in cases like this:

interface A { a: number }
interface B { b: number }
var x: A & B = { a: 1, b: 2 };

We make the check upfront for the entire target type, but then as we descend into the structure of the target type we no longer want to make the check again (as it would now fail).

@JsonFreeman

This comment has been minimized.

Contributor

JsonFreeman commented Jul 13, 2015

When we discussed this, we had some thoughts about indexers. Namely if the object type has an indexer, we want to allow excess properties to be provided, since the object might behave as a map. What is the reason for abandoning that idea? Is this going to limit the usefulness of indexers with object literals?

@JsonFreeman

This comment has been minimized.

Contributor

JsonFreeman commented Jul 13, 2015

Where is it that an object literal type loses its freshness? I don't see any place in the code where inferring an object type for a variable causes us to infer the regular type.

To clarify, I would assume this "freshness" gets lost when we widen an object literal type. This is what we do to indicate for example that for subtype, optional properties in the target must be declared in the source, unless the source is a "fresh" (unwidened) object literal. So I am looking for similar logic here.

@JsonFreeman

This comment has been minimized.

Contributor

JsonFreeman commented Jul 13, 2015

I got my indexer question answered.

var resolved = resolveStructuredTypeMembers(type);
return !!(resolved.properties.length === 0 ||
resolved.stringIndexType ||
resolved.numberIndexType ||

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Ah, so you do these checks on the target side as opposed to when contextually typing the object literal. Actually, this is probably a good thing, because not every assignability check is guaranteed to be accompanied by contextual typing. So if the object did not get contextually typed, the assignment would still be allowed, which is good I think.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

But I would add some comments explaining the rationale behind these heuristics.

}
if (type.flags & TypeFlags.UnionOrIntersection) {
for (let t of (<UnionOrIntersectionType>type).types) {
if (isKnownProperty(t, name)) {

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

I agree that this loop is doing the right thing for intersections. For unions, I would not say that a property is known if some constituent has it. Doesn't it seem more correct to demand that all union constituents have the property?

Actually, this does make sense. Really you're trying to figure out if you have heard of this property anywhere in the target. If you have, then it would not be considered excess in the source. So it's not that the target is known to have this property, it's that this property was mentioned as a potential property of the target. This also why optional property are "known" even though they might not actually be present on the target.

This comment has been minimized.

@ahejlsberg

ahejlsberg Jul 13, 2015

Member

Exactly.

// Above we check for excess properties with respect to the entire target type. When union
// and intersection types are further deconstructed on the target side, we don't want to
// make the check again (as it might fail for a partial target type). Therefore we obtain
// the regular source type and proceed with that.

This comment has been minimized.

@JsonFreeman

JsonFreeman Jul 13, 2015

Contributor

Thanks. This makes sense now

@danquirk

This comment has been minimized.

Member

danquirk commented Jul 13, 2015

Note this is a breaking change. We should probably put this in a widely distributed beta before unleashing it as a default in 1.6 or some future version.

@ahejlsberg

This comment has been minimized.

Member

ahejlsberg commented Jul 13, 2015

@danquirk Agreed.

@jonathandturner

This comment has been minimized.

Contributor

jonathandturner commented Jul 13, 2015

We talked about this a bit today in-person. I'm not sure I'm keen on co-opting object literals to mean this, but I think my real discomfort is that, ideally, interfaces should be able to describe any type. They're the fundamental building block of the type system*.

Because of this, I think the natural place is to describe this first in the interface first. Strictness is opt-in there, and would not break code.

I get that there is a lot of interest in putting this in and catching bugs, but I'm not yet seeing the advantage of taking the breaking change over our philosophy of "interface is the foundation of the type system"

That said, I could be swayed...

  • - there are a couple corner cases where this isn't true, but arguably we may want to fix those over time.

mhevery added a commit to angular/angular that referenced this pull request Aug 13, 2015

chore: Remove IRequestOptions / IResponseOptions
BREAKING CHANGE:

Reasons:

1) Interfaces should not start with letter ‘I’
2) Interfaces do not help with mistype properties, but literal types do.
   - Microsoft/TypeScript#3823
   - https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#strict-object-literal-assignment-checking

mhevery added a commit to angular/angular that referenced this pull request Aug 13, 2015

chore: Remove IRequestOptions / IResponseOptions
BREAKING CHANGE:

Reasons:

1) Interfaces should not start with letter ‘I’
2) Interfaces do not help with mistype properties, but literal types do.
   - Microsoft/TypeScript#3823
   - https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#strict-object-literal-assignment-checking

mhevery added a commit to angular/angular that referenced this pull request Aug 13, 2015

chore: Remove IRequestOptions / IResponseOptions
BREAKING CHANGE:

Reasons:

1) Interfaces should not start with letter ‘I’
2) Interfaces do not help with mistype properties, but literal types do.
   - Microsoft/TypeScript#3823
   - https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#strict-object-literal-assignment-checking

mhevery added a commit to angular/angular that referenced this pull request Aug 13, 2015

chore: Remove IRequestOptions / IResponseOptions
BREAKING CHANGE:

Reasons:

1) Interfaces should not start with letter ‘I’
2) Interfaces do not help with mistype properties, but literal types do.
   - Microsoft/TypeScript#3823
   - https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#strict-object-literal-assignment-checking

mhevery added a commit to angular/angular that referenced this pull request Aug 13, 2015

chore: Remove IRequestOptions / IResponseOptions
BREAKING CHANGE:

Reasons:

1) Interfaces should not start with letter ‘I’
2) Interfaces do not help with mistype properties, but literal types do.
   - Microsoft/TypeScript#3823
   - https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#strict-object-literal-assignment-checking

mhevery added a commit to angular/angular that referenced this pull request Aug 13, 2015

chore: Remove IRequestOptions / IResponseOptions
BREAKING CHANGE:

Reasons:

1) Interfaces should not start with letter ‘I’
2) Interfaces do not help with mistype properties, but literal types do.
   - Microsoft/TypeScript#3823
   - https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#strict-object-literal-assignment-checking

robertknight added a commit to robertknight/passcards that referenced this pull request Aug 16, 2015

Fix compatibility with TS 1.6 object literal typing
TS 1.6 requires that object literals assigned
to variables or parameters with an interface typing
may only contain properties that are listed in the interface.

See Microsoft/TypeScript#3823
for details.

This mostly affected React component props interfaces
which did not extend react.Props so were missing the common
key, ref and children props.

@kimamula kimamula referenced this pull request Sep 6, 2015

Closed

add Event3 and Event4 #1

@kitsonk kitsonk referenced this pull request Dec 20, 2015

Closed

problem with interfaces #6175

@Microsoft Microsoft locked and limited conversation to collaborators Jul 31, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.