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

Regression in nightly build related to keyof, type narrowing, and exactOptionalPropertyTypes #55756

Closed
mtreder opened this issue Sep 15, 2023 · 14 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@mtreder
Copy link

mtreder commented Sep 15, 2023

🔎 Search Terms

keyof
type narrowing
exactOptionalPropertyTypes

🕗 Version & Regression Information

  • This is newly identified as error by the compiler
  • This changed between versions nightly build on 9/13 and 9/14
  • This is the behavior in every version I tried prior to Nightly

⏯ Playground Link

https://www.typescriptlang.org/play?exactOptionalPropertyTypes=true&ts=5.3.0-dev.20230915#code/KYDwDg9gTgLgBASwHY2FAZgQwMbDgSQCFMBnPAbwFgAoASAH0AjKTJAEwC45WBPAbhoBfGjVCRYiFGiy4CAETQIAbsDYBBOKFTsSBYmThU6YKBDBoYPNQH4ujCBAA2wVgOrDqo8NHjJUGHDx8BShlVUJNEG02XSJSChpaEzMLHkJbOBIYUKQAczcPL3F4S3MCNUdHAHkwGAQIJF0AXj14uAAyRODFFXUOrpCwtkI3Ip84dABXJGw6hrhsRwbgGrnGgApEkihsLnwK6tr6xoAaRLZgLLgW4ABbWp5V45IAHn3Kp4aSAD51gEoztQ-oZEusqowAFbAWYAOgA1sAeCR1ttsMDSHB1gieBB0OUPkcvn8ANoAXT+MPQ0AAojgABbrMDXb4gui0BB49alYC4zI7YlgUlwACETRaACIIJDoTBxcCjLRFRcsgKhS1Uaq3IqPLRBH9RnQoMAYJMoEg4MqYAURNQpjM1pp7pZPo0XgAVSLRWIHF0-f5cD0Ko0ms2GOBMFjsLhKCAINhwAAMcEE3F0boKQA

💻 Code

// @strict: true
// @exactOptionalPropertyTypes: true

export interface IBase {
	_brand: any;
}

export interface IDerivedA extends IBase {
	propertyA?: boolean;
}

export interface IDerivedB extends IBase {
	propertyB?: string;
}

export type IAllOptions = IBase &
	IDerivedA &
	IDerivedB;

export function cloneOptions(
	src: IAllOptions,
	dest = emptyOptions<IAllOptions>(),
) {
	(Object.keys(src) as (keyof IAllOptions)[]).forEach(p => {
		if (typeof src[p] !== "object") {
			dest[p] = src[p];
		}
	});

	return dest;
}

function emptyOptions<T extends IAllOptions>(): T {
	return { _brand: void 0 } as T;
}

🙁 Actual behavior

There is an error on the line attempting to copy src[p] into dest[p]:

type 'any' is not assignable to type 'never'.

🙂 Expected behavior

Previously this compiled without error. Both objects are of the same type, so they should have the same properties.

Additional information about the issue

No response

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 15, 2023

The change between origin/release-5.2 and origin/main occurred at e6321d7.

@RyanCavanaugh
Copy link
Member

The change between origin/release-5.2 and origin/main occurred at 9cbcf01.

Uh, no

@RyanCavanaugh
Copy link
Member

This seems like a correct change. The example is not, from TypeScript's perspective, any different from

	let m1: keyof IAllOptions = 'propertyA' as keyof IAllOptions;
	let m2: keyof IAllOptions = 'propertyB' as keyof IAllOptions;
	dest[m1] = src[m2];

which should be an error (and also changed at the same time).

That said, we should narrow down which PR did this to make sure it was at least somewhat intended.

@fatcerberus
Copy link

dest[p] = src[p];

I'm like 99% positive this has come up before in different contexts but basically, yeah, the compiler only sees the types in zero-order and doesn't understand the higher-order concept that p necessarily refers to the same property in both positions. Supporting that would likely require #30581 or something similar.

we should narrow down which PR did this

@RyanCavanaugh Looks like that'd be #55585, if typescript-bot's updated bisect is correct (the commit you quoted is different from the one that's linked now).

@jakebailey
Copy link
Member

A local bisect confirms that this is due to #55585. @Andarist

@Andarist
Copy link
Contributor

I can't say this one was intended but it also looks to me like a correctness improvement. What I can offer is to add some tests covering this but other than that LGTM and the issue could be closed.

@SlurpTheo
Copy link

This seems like a correct change. The example is not, from TypeScript's perspective, any different from

	let m1: keyof IAllOptions = 'propertyA' as keyof IAllOptions;
	let m2: keyof IAllOptions = 'propertyB' as keyof IAllOptions;
	dest[m1] = src[m2];

which should be an error (and also changed at the same time).

I can't say this one was intended but it also looks to me like a correctness improvement.

Adding an explicit | undefined to either propertyA's -OR- propertyB's declaration makes this new error disappear. Seems like this new checking/error is incomplete (i.e., still possible to get undefined explicitly stored in a signature that doesn't have | undefined in its declaration).

@fatcerberus
Copy link

@SlurpTheo I’d recommend opening a new issue for that because it definitely sounds like a bug.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 19, 2023
@SlurpTheo
Copy link

Adding an explicit | undefined to either propertyA's -OR- propertyB's declaration makes this new error disappear. Seems like this new checking/error is incomplete (i.e., still possible to get undefined explicitly stored in a signature that doesn't have | undefined in its declaration).

@SlurpTheo I’d recommend opening a new issue for that because it definitely sounds like a bug.

I can't come up with a reproduction that doesn't involve { _brand: any }; and with any involved, I don't feel like quibbling over ~inconsistencies.

Playground link

@fatcerberus
Copy link

Yeah, that's expected; once you have an any involved all bets are off because both any | T and any & T just reduce to any. To be specific, what the assignability check for dest[p] = src[p] asks is: "Is the union of all properties potentially selected by p assignable to the intersection of those properties?" So that behavior for any is actually consistent, despite outward appearances.

@fatcerberus
Copy link

#30769 was a similar change made years ago that was likewise a big source of consternation when first introduced, for what it's worth.

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 22, 2023
@SlurpTheo
Copy link

SlurpTheo commented Oct 6, 2023

#30769 was a similar change made years ago that was likewise a big source of consternation when first introduced, for what it's worth.

Whoa 👀
(~fn3() example in the main text of that issue)

type T = Record<string, unknown>;
function f3<U extends T>(p: { objT: T; objU: U }) {
	p.objT["foo"] = 11;

	// !!! I'm not sure I understand !!!
	// @ts-expect-error  ts(2536): Type '"foo"' cannot be used to index type 'U'.
	p.objU["foo"] = 22;

	// @ts-expect-error  ts(2352): Property 'foo' is missing in type 'T' but required in type '{ foo: unknown; }'.
	(p.objU as { foo: unknown })["foo"] = 222;

	(p.objU as { foo?: unknown })["foo"] = 2222;

	(p.objU as T)["foo"] = 22222;
}
f3({ objT: {}, objU: {} });
f3<{ bar: unknown }>({
	objT: { baz: 33 },
	objU: { bar: 44 },
});
f3<{ bar: unknown }>({
	objT: { baz: 33 },
	objU: {
		bar: 44,
		// @ts-expect-error  ts(2322): Object literal may only specify known properties, and 'qux' does not exist in type '{ bar: unknown; }'.
		qux: 55,
	},
});
const objU1 = {
	bar: 44,
	qux: 55,
};
f3<{ bar: unknown }>({
	objT: { baz: 33 },
	objU: objU1,
});
const objU2 = {
	qux: 55,
};
f3<{ bar: unknown }>({
	objT: { baz: 33 },

	// @ts-expect-error  ts(2741): Property 'bar' is missing in type '{ qux: number; }' but required in type '{ bar: unknown; }'.
	objU: objU2,
});

Setting properties on U is different than setting properties on T even though U extends T?

Playground

@SlurpTheo
Copy link

Setting properties on U is different than setting properties on T even though U extends T?

I mean that pull request does state the following:

  • Given a type variable T with a constraint C, when an indexed access T[K] occurs on the target side of a type relationship, index signatures in C are now ignored. This is because a type argument for T isn't actually required to have an index signature, it is just required to have properties with matching types.

But it still makes me 😵

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

7 participants