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

Type inference with destructured parameters is overly specific #55596

Closed
clshortfuse opened this issue Aug 31, 2023 · 7 comments
Closed

Type inference with destructured parameters is overly specific #55596

clshortfuse opened this issue Aug 31, 2023 · 7 comments
Labels
Duplicate An existing issue was already created

Comments

@clshortfuse
Copy link

clshortfuse commented Aug 31, 2023

πŸ”Ž Search Terms

  • destructure
  • destructured
  • parameters

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about function declarations

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.3.0-dev.20230831#code/MYGwhgzhAEAK0G8CwAoa7oDMD23oF5oByAIzACciBuVDabAFwAsBTc2CsAWwOIDtG5FgEcArgEshAE2qoAvqlTBsfCA2hSWmMKJAMAkqoZg+wFgHlM8QnxYB3OAAoAlDRSpMo0w3EroAcxYGADFcRwQsXGg5Xk1tXQMjEzNLWGdEWgwhBlFyPkjsNwV3FECQsIicbAAuYjIALyJo51QgA

πŸ’» Code

class P {
    foo = 'bar';
    otherParam = 'notrequired';
}

const defaultInstanceOfP = new P();

function getFoo({ foo } = defaultInstanceOfP) {
    return foo;
}

getFoo({ foo: 'baz' })

πŸ™ Actual behavior

declare function getFoo({ foo }?: P): string;

Argument of type '{ foo: string; }' is not assignable to parameter of type 'P'.
Property 'otherParam' is missing in type '{ foo: string; }' but required in type 'P'.

πŸ™‚ Expected behavior

declare function getFoo({ foo }?: {
    foo: string;
}): string;

Additional information about the issue

This is just an example, but having to cast it on the function appears needless to me. If P is actually required because, perhaps, the code will reference arguments[0], then that should be the explicit cast needed (as P), not other way around.

In more complex setups, destructing many variables with a cast is needlessly verbose:

const element = document.getElementById('el');
function computeShape({ clientWidth, clientLeft, clientTop, clientHeight } 
    = element as { clientWidth: number, clientLeft: number, clientTop: number, clientHeight: number }) {
    // Do work
}
@MartinJohns
Copy link
Contributor

Duplicate of #51179.

@clshortfuse
Copy link
Author

clshortfuse commented Sep 1, 2023

@MartinJohns I see the relation, but don't see how it's duplicated. That one talks about explicitly stated types while this is talking about type inference.

@MartinJohns
Copy link
Contributor

MartinJohns commented Sep 1, 2023

The type is inferred by the default value you provide, which is of type P. That it's being destructured then is irrelevant. Destructuring applies to the implementation, not to the callsite. See also this comment: #43725 (comment) (#43725 is a more matching duplicate)

So your parameter argument is typed P, but then you provide an object literal that is not compatible with P due to the missing property.

@clshortfuse
Copy link
Author

@MartinJohns Thanks. I see that's much more related and almost identical to what I've presented.

I do wonder what the consequences of narrowing the type would be. I understand it's not a bug, though I would like the ability to narrow this somehow, even if it's behind a flag. I can do a post-processing of the typings file and rewrite, though I wish I didn't have to. I don't see any consequence of refactoring:

declare function getFoo({ foo }?: {foo:string, bar:boolean): string;

as

declare function getFoo({ foo }?: {foo:string): string;

I was presenting a limited example of the issue I see, but a more detailed explanation of what I'm trying to accomplish is here

function extendObject<
    T = any,
    M = Record<string, (source?: any) => string | boolean | null>
>(target: T, methods: M):
    T & { [K in keyof M & string]: M[K] extends (...args: infer R) => any ?
        (data?: R[0]) => ReturnType<M[K]> : never } {

    // @ts-expect-error testing
    for (const [key, value] of Object.entries(methods)) {
        // @ts-expect-error testing
        t[key] = value.bind(t);
    }
    // @ts-expect-error testing
    return t;
}

const methods = {
    method1({ ATTRIBUTE_NODE } = document) {
        return ATTRIBUTE_NODE;
    },
    method2({ innerWidth} = window) {
        return window.innerWidth;
    }
}

const result = extendObject({}, methods);

result.method1(); // Optional param0
result.method1(document); // Redundant param0
result.method1({ ATTRIBUTE_NODE: 2 }) // param0-like  (Error)

result.method2(); // Optional param0
result.method2(window); // Redundant param0
result.method2({ innerWidth: 360 }) // param0-like (Error)

Essentially I want to manipulate an object and in some cases the prototype directly. It's related to polymorphic this which we have to crunch ourselves


The real world example is here: https://github.com/clshortfuse/materialdesignweb/blob/42d3679026733648908758ae5f0d17d63db9ce99/components/Button.js#L57-L64

It all works with the types I have built and automatically through inference. But I have to strip the type because it goes on this extremely long, recursive type that leaves files ending up in megabyte sizes, trashing performance.

clshortfuse/materialdesignweb@bc34a31#diff-bd37c6657f46f2ec3dfb03aaa075b8a802a10da7e6a8c8b59363c6e9c8e5fb3bL383-L1145 (It's the Button.d.ts file if GitHub doesn't expand it automatically.)

If I could infer the destructured keys used from the object I could rewrite/optimize the resulting type myself, but I don't think there exists a utility type for listing destructured keys (eg: DestructuredKeys<Parameters<FN>[0]>). If that would be possible somehow, then I could possible cook up my own reduced-size type.

@MartinJohns
Copy link
Contributor

even if it's behind a flag

If the team introduced a flag every time it's suggested I would quickly run out of disk space for the tsconfig alone.

You can use a type annotation and Pick<T, K> and just pick the properties you want, matching your destructuring.

{ foo }: Pick<typeof defaultInstanceOfP, "foo"> = defaultInstanceOfP

Inferring a different type based on the destructuring is not going to happen.

@clshortfuse
Copy link
Author

You can use a type annotation and Pick<T, K> and just pick the properties you want, matching your destructuring.

That means every single expression would have to redundantly be rewritten, which is the original point. Also, I would suggest you continue to read the rest to comment to why the sample code you provided doesn't work.

If the team introduced a flag every time it's suggested I would quickly run out of disk space for the tsconfig alone.

It's contradictory to state that disk space is a concern, when implementing the flag in question would reduce file space. Right now the overly specific types are resulting the types folder being 6.1M vs 1.7M if they were reduced or removed. A 72% size reduction is nothing to scoff at.


@RyanCavanaugh Let me know what you think about possible solution for tackling this issue. It's slightly tied to another issue related to recursion that tagged for v5.2.1 milestone here:

#54533

I was able to mostly reconstruct the code base to avoid heavy recursion, but kinda stuck on this one.

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Sep 1, 2023
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" 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 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants