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

No excess property checks when dealing with tagged unions #12745

Closed
DanielRosenwasser opened this issue Dec 8, 2016 · 12 comments
Closed

No excess property checks when dealing with tagged unions #12745

DanielRosenwasser opened this issue Dec 8, 2016 · 12 comments
Assignees
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

Version: 2.1.4

interface None {
    hasValue: false;
}

interface Some {
    hasValue: true;
    value: any;
}

var x: None | Some = {
    hasValue: false,
    value: [],
}

Expected: Excess property error on value in the object literal.
Actual: No error.

@mhegazy mhegazy added the Suggestion An idea for TypeScript label Dec 8, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Dec 8, 2016

We will need a proposal for this, since this has to take into consideration the discriminant property in the union and filter on this.

@ghost
Copy link

ghost commented May 23, 2017

Just ran into this.

interface A {
    type: "A";
    data: { b: boolean };
}

interface B {
    type: "B";
    data: { n: number };
};

const x: A | B = {
    type: "B",
    data: { n: 1, excess: 2 }
};

@sandersn
Copy link
Member

sandersn commented Jun 8, 2017

Fix is up at #16363

@DanielRosenwasser DanielRosenwasser modified the milestone: TypeScript 2.4 Jun 9, 2017
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 2.6 milestone Aug 2, 2017
@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Aug 2, 2017

Is the fact that the following is not checked for excess properties also related to this? In other words, should I open another issue on how the behavior is present regardless of the union being tagged?

type Props<T> = PropsBase | PropsWithConvert;

interface PropsBase {
    data: string;
}

interface PropsWithConvert extends PropsBase {
    convert: (t: number) => string;
}

function doIt<T>(props: Props<T>) {
}

// 'convert' here returns a 'number', so it can't be assignable to 'PropsWithConvert',
// But the check still appears to succeed because the object is assignable to 'PropsBase'.
doIt({
  data: "",
  convert(x) {
    return 10;
  }
})

@sandersn sandersn added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Aug 24, 2017
@lukescott
Copy link

lukescott commented Nov 15, 2017

I'm not sure if this is completely fixed. I have code that looks like this:

export type Condition = And | Or | Not | {
	[type: string]: {
		target: string
		[key: string]: any
	}
}

export interface And {
	and: Condition[]
}

export interface Or {
	or: Condition[]
}

export interface Not {
	not: Condition
}

const c: Condition = {
	and: [],
	not: { // <-- should be producing an error
		equals: {
			target: "name",
			value: "foo"
		},
	},
}

And there are no errors. Also if I do this there are no errors as well:

const c: Condition = {
	foo: "bar", // <--- should be error here. does not exist in any type
	not: {
		equals: {
			target: "name",
			value: "foo"
		},
	},
}

@RyanCavanaugh
Copy link
Member

@lukescott that's the intended behavior - a property isn't excess if it appears in any of the target types

@lukescott
Copy link

Ah - I missed hasValue was defined on both.

@lukescott
Copy link

lukescott commented Nov 15, 2017

@RyanCavanaugh ok I get how and and or are allowed on the same object with |. I misunderstood what the fix was about. However, why is foo: "bar" not emitting an error? As far as I know that shouldn't be matching up against:

type Misc = {
	[type: string]: {
		target: string
		[key: string]: any
	}
}

@sandersn
Copy link
Member

Your foo: "bar" example looks like a bug, but not one with excess property checks; it's not doing type checking, just properties. With respect to excess properties, foo is not excess because Misc has a string index signature.

However, you should see a type error because "bar" is missing the property target.

@sandersn
Copy link
Member

oh, never mind. You're falling between two checks.

  1. The excess property check is satisfied by Misc, which has a string indexer. No property will be marked as excess.
  2. Structural assignability is satisfied by Not, since the object literal has the property not, but also an extra (excess!) property foo. But this is fine according to the rules of structural assignability.

The basic problem is that excess property checks happen before structural assignability finds the "best match" of the union; it uses a faster, heuristic check that doesn't perfectly match the exhaustive search used by structural assignability.

@sandersn
Copy link
Member

@lukescott I opened a bug #20060 to track your repro. I can't promise we'll do anything about it because we rely on the early-fail property of excess property checking for some speed in isRelatedTo.

@lukescott
Copy link

@sandersn Awesome, thank you!

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

No branches or pull requests

5 participants