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

NoInfer not working on callback parameter destructuring #60544

Open
miguel-leon opened this issue Nov 20, 2024 · 15 comments
Open

NoInfer not working on callback parameter destructuring #60544

miguel-leon opened this issue Nov 20, 2024 · 15 comments

Comments

@miguel-leon
Copy link

miguel-leon commented Nov 20, 2024

πŸ”Ž Search Terms

NoInfer parameter callback destructuring

πŸ•— Version & Regression Information

Latest to date.

⏯ Playground Link

https://www.typescriptlang.org/play/?#code/MYGwhgzhAEBiD28A8AVANNASgPmgbwCgBIYeAOwgBcAnAV2EvmoApqBTMAE3JAE9pgAIwBc0ZpVHpoAfVEA5eAEkyAMzbUkeaAA9RZWgFtB6gNzR4lABbr5S1eqQ5oAX2wBKaAF5cmD3mfEAPSBAuRUdAxMrBzcZHwCImIS0FKy+Dp6hsbUZhbW1LbKahpOzh7eWH4BRMGhFDT0jCzsXDz8QqLikhhpCkUOWrrQ+kam5lY2WC7uXj5VBAEEpPUyAIxew2wA7nCIzMxkYAZsouEAlmQA5hiDLuW4zFqHxxjad25mtQjI51cYtGQANZkeBbMjYAi1CCWeC0ECcaDGXY-GgXa7pZ4naC-S5mIYjbLTAhLMKUGQAJg2ZG2yP2mNOqKu9zETyObHenxCWyYgIgom+SBxN2GbIZ1DRRKAA

πŸ’» Code

class Foo<T, R> {
	constructor(readonly cb: (t: T, _: NoInfer<{ x: number; other: NoInfer<R> }>) => R) {}
}

const _1 = new Foo((name: string, { x }) => ({ name, x })); // Foo<string, unknown>
// should be Foo<string, { name: string; x: number }>

const _2 = new Foo((name: string) => ({ name })); // works: Foo<string, { name: string }>

πŸ™ Actual behavior

infers unknown.

πŸ™‚ Expected behavior

Should ignore the parameter tagged with NoInfer and infer correctly.

Additional information about the issue

Is this intended behavior? feels like a bug to me.
If the callback parameter is destructured partially it infers unknown, but it works if the parameter is omitted.
I tried NoInfer at various locations.

@jcalz
Copy link
Contributor

jcalz commented Nov 20, 2024

Note: not a TS team member

That parameter is not annotated so it can't infer from there anyway, with or without NoInfer. I think you're hoping that NoInfer can somehow break the dependency of the function return type on its parameter type, but that's not what NoInfer does. This looks more like #47599 and possibly even just a plain old design limitation.

@miguel-leon
Copy link
Author

I don't know what you mean here by "not annotated" and I'm not hoping anything about NoInfer, let alone "break the dependency" which I also don't know what you mean.
If this looks ok to you, πŸ‘.

@jcalz
Copy link
Contributor

jcalz commented Nov 20, 2024

{ x } is not annotated with a type. Therefore you are expecting it to be contextually typed. Since there's no type there, TypeScript cannot possibly infer the type from that position, whether or not you have NoInfer there.

As for "I'm not hoping anything" I was trying to say that you have an expectation about NoInfer that is not warranted. In the type (t: T, _: NoInfer<{ x: number; y: NoInfer<R> }>) => R the type of _.other depends on R and so does the return type. When you pass in a function like (t: string, other) => β‹―, TS wants to know the type of the return type before it knows what other is, but since, syntactically, the return type might well depend on the type of y.other, it can't do that and it fails. Your expectation here is that TS could possibly inspect the body of the function (the β‹―) and see that the return type actually does not depend on the type of y.other. But TS doesn't do this. See #47599 and the issues linked to that one.

@miguel-leon
Copy link
Author

In this example { x } is "not annotated" like you say but it's still inferred. So I still don't know what you mean.
As for you insisting in my expectations, like I say, I don't have any. Here is explained what NoInfer does, and I'm using it for the explained purpose.
If you're trying to say this is some sort of an undocumented gotcha (for other reasons or design limitations), then ok, just say it can't be done. No need to discuss my expectations.

@miguel-leon
Copy link
Author

In any case, not wanting to continue the discussion above and getting back to the issue at topic.

I managed to do a workaround that works. playground

class Foo<T, R> {
	constructor(readonly cb: <NoInferR = R>(t: T, _: { x: number; other: NoInferR }) => R) {}
}
const _1 = new Foo((name: string, { x }) => ({ name, x })); // Foo<string, { name: string; x: number }>

but it breaks again as soon as I try to merge the class with an interface. It doesn't make sense. playground

class Foo<T, R> {
	constructor(readonly cb: <NoInferR = R>(t: T, _: { x: number; other: NoInferR }) => R) {}
}

interface Foo<T, R> {}

const _1 = new Foo((name: string, { x }) => ({ name, x })); // Foo<string, unknown> again

@jcalz
Copy link
Contributor

jcalz commented Nov 20, 2024

I… the β€œexpected behavior” is all I was trying to say (as per the template for the issue) I sincerely don’t mean to offend, nor do I want to seem to derail your issue. I will disengage now. Good luck.

@Andarist
Copy link
Contributor

The behavior is a design limitation. And like @jcalz has mentioned it doesn't really have anything with NoInfer.

Your _2 variant isn't context-sensitive and this its parameter types don't have to be "resolved"). So by the time compiler gets to the return expression~ from which it can infer R it can do so because it has not been resolved and "fixed" (aka set in stone) yet.

In the _1 it has to resolve them before checking the function's body (and thus the return expression~). This involves fixing all the encountered type parameters with whatever the current information about it it has. This process can't be reversed and the decision can't be changed as other types could start depending on it/be derived from it. So by the time it gets to the return expression~ it's already too late. The R's type got fixed to unknown by this time.

I managed to do a workaround that works. playground

The type there might be what you expect here but I don't think it actually works like you might expect it too: TS playground

@miguel-leon
Copy link
Author

In this example: playground

class Foo<T, R> {
	constructor(readonly cb: (t: T, _: { x: number; other: NoInfer<R> }) => R) {}
}

const _1 = new Foo((name: string, { x }): { name: string; x: number } => ({ name, x })); // Foo<string, unknown>

I'm annotating both T and the return type R, so that R doesn't have to depend on anything, and it's still resolving to unknown. Nothing about the second parameter (context-sensitive or not), should matter because I'm trying to mark it as NoInfer so that R doesn't have to depend on it. Isn't that the purpose of NoInfer?

On the other hand, I understand that in the workaround NoInferR is meaningless inside the callback, so it doesn't really solves the problem.
But what's up with workaround 2? Why does it break with a change that changes nothing?

Or is this a different discussion? I feel like there are bugs everywhere.

Welp, focusing on the original example, should the title of the issue be something like "There's no way to mark parameters in callback types as NoInfer"? is it because NoInfer is recognized only at the top level of the parameter type? I found this #59668

@Andarist
Copy link
Contributor

But what's up with workaround 2? Why does it break with a change that changes nothing?

This is an interesting case. It seems like the compiler doesn't ignore from a return type annotation. It should be fairly straightforward to add this capability, I'll take a stab at it later.

Or is this a different discussion? I feel like there are bugs everywhere.

As it stands, this is a design limitation and not a bug. I understand it's frustrating to hit limitations but it's not like they exist for no reason. The type checker is a complex piece of software and its design comes with some tradeoffs here or there.

Welp, focusing on the original example, should the title of the issue be something like "There's no way to mark parameters in callback types as NoInfer"? is it because NoInfer is recognized only at the top level of the parameter type? I found this #59668

No. What you report here is not about NoInfer itself but about the fact that u'd like to use the inferred type automatically in the parameter type but at the same time you'd like to infer it from the return position. This is specific to the return thing here and not to NoInfer being somewhere deeper in the parameter's type. This doesn't work either after all:

class Foo<T, R> {
  constructor(readonly cb: (t: T, other: NoInfer<R>) => R) {}
}

const _4 = new Foo((name: string, other) => ({ name })); // Foo<string, unknown>

The NoInfer here is just a red herring. The core of the issue lies in how assigning contextual parameter types has to fix R here before it has a chance to even see the return expression~

@miguel-leon
Copy link
Author

miguel-leon commented Nov 21, 2024

The core of the issue lies in how assigning contextual parameter types has to fix R here before it has a chance to even see the return expression~

I think I understand now with this wording. Although, I still think that NoInfer should prevent it from "fixing" R because it should mean something like "deciding on R is not up to you". But I also understand that then the compiler would have to somehow go back after discovering R in the return type.

I tried to see if the limitation can be circumvented with an overload that doesn't have R in the second parameter but I've had no luck there when the property with R is indeed present.

This is an interesting case. It seems like the compiler doesn't ignore from a return type annotation. It should be fairly straightforward to add this capability, I'll take a stab at it later.

On workaround 2, I found out that if you extract the type to a auxiliary interface, it works again. playground. It really is shocking seeing how changes that don't change anything make it or break it.

@Andarist
Copy link
Contributor

On workaround 2, I found out that if you extract the type to a auxiliary interface, it works again. playground. It really is shocking seeing how changes that don't change anything make it or break it.

It really doesn't work here. It's the same thing I mentioned at the end of this comment. You can hover over things to see that your NoInferR only benefits from its default type param being resolved but it still can't be used at all: TS playground

@miguel-leon
Copy link
Author

miguel-leon commented Nov 21, 2024

I meant on resolving the merge with the interface breaking it. Because you said you would have a stab at it.

I understand that the workarounds don't really help inside the callback itself. Although that doesn't matter too much, as long as the variable outside (_1) is inferred correctly. Which is the original issue. Inside the callback, sure, you have to go ahead and annotate it.

@miguel-leon
Copy link
Author

miguel-leon commented Nov 21, 2024

From what was discussed, I think I discerned the best hack for it to work as intended. playground

interface Aux<R> { // required so that the interface merge doesn't break it.
	x: number;
	other: R;
}

class Foo<R, T> {
	constructor(cb: (t: T, _: Aux<unknown>) => R);
	constructor(cb: (t: T, _: Aux<R>) => R);
	constructor(readonly cb: (t: T, _: Aux<R>) => R) {}
}

interface Foo<R, T> {}

const _1 = new Foo((name: string, { x }) => ({ name, x })); // Foo<{ name: string; x: number }>

const _2 = new Foo((name: string, { other }: { other: { name: string } }) => {
	other?.name;
	return { name };
}); // Foo<{ name: string; x: number }>
  • With an alias so that the interface merge doesn't break it.
  • An overload with unknown (same as a generic parameter) so that it infers _1 outside when R is disregarded in the callback parameters.
  • An overload with R so that it typechecks correctly if the callback parameter is indeed annotated.

In my opinion, the unknown overload shouldn't be necessary if the property with R is disregarded in the callback signature. If anything, NoInfer would be correctly interpreted by the user as "don't try to figure out R".

And the alias shouldn't be necessary either, though you already mentioned it is a pretty straightforward fix.

What's concerning is that there's not a clear strategy on how to reach these "hacks", or for that matter, on how to report the usability issue here (regardless of what the compiler does behind the scene), besides throwing stuff at the wall to see what sticks.

@miguel-leon
Copy link
Author

Is it too complex to not fix R before it has a chance to see the return position and be inferred by it?

Perhaps using NoInfer to tell the checker to do such thing? So that the code here works #60544 (comment)

If this were to be a suggestion, is it considerable or is it too outlandish and out of the question?

@Andarist
Copy link
Contributor

In my opinion, the unknown overload shouldn't be necessary if the property with R is disregarded in the callback signature. If anything, NoInfer would be correctly interpreted by the user as "don't try to figure out R".

It has to figure out the parameter's type that contains R though. It might not be apparent here because you have destructured that parameter but the type for the overall parameter still has to be resolved. It would be a big undertaking to change this now. Potentially not impossible for certain scenario but the ROI seems to be pretty low. And even with such an improvement, I still wouldn't expect this to work for cases in which you actually end up destructuring other. Having part of your parameter type dependent on the return type... isn't exactly a popular pattern. To figure out it's type the compiler has to typecheck the return expression, that in turn might depend on the information about the function's parameters - it's very easy to end up with circularity issues in those scenarios.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants