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

Excess property checks for destructured arguments #42419

Closed
5 tasks done
remcohaszing opened this issue Jan 20, 2021 · 11 comments
Closed
5 tasks done

Excess property checks for destructured arguments #42419

remcohaszing opened this issue Jan 20, 2021 · 11 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@remcohaszing
Copy link

Suggestion

πŸ” Search Terms

List of keywords you searched for before creating this issue. Write them down here so that others can find this suggestion more easily and help provide feedback.

pick destructured

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

If a function accepts destructured parameters, the type parameter can be treated as if it’s a Pick<>ed version of the type annotation.

When destructuring an argument in a function declaration, TypeScript can statically determine any unused properties can be omitted safely.

For example, the following two examples would be equivalent:

export function foo({ bar, baz }: FooOptions): void;
export function foo(options: Pick<FooOptions, 'bar' | 'baz'>): void;

Of course it’s already possible to combine destructured properties with Pick<>, but having to synchronize this manually is cumbersome and error prone.

πŸ“ƒ Motivating Example

TypeScript now automatically picks destructured properties from a call signature argument.

For example:

interface User {
  id: string;
  name: string;
  email: string;
}

function stringifyUser({ name, email }: User): string {
  return `${name} (${email})`
}

// This is ok, because even though `id` is required for users, its unused by `stringifyUser`.
stringifyUser({ name: 'Developer', email: '[email protected]' })

// This is not ok, because `email` is destructured.
stringifyUser({ name: 'Developer' })

πŸ’» Use Cases

A function signature may adhere to an interface where some properties exist. Using this interface as a type parameter would be the logical thing to do. However, there may be use cases where it’s desirable to call the function with only the parameters that are actually used. The first use case that comes to mind is when writing tests.

@MartinJohns
Copy link
Contributor

MartinJohns commented Jan 20, 2021

This would be one massive breaking change. Didn't think this comment through.

@remcohaszing
Copy link
Author

I see one minor breakage:

interface User {
  id: string;
  name: string;
  email: string;
}

function stringifyUser({ name, email }: User): string;

// `id` should be removed.
stringifyUser({ id: '1', name: 'Developer', email: '[email protected]' })

// This is still fine though
stringifyUser({ id: '1', name: 'Developer', email: '[email protected]' } as User)

@MartinJohns what else would this break?

@MartinJohns
Copy link
Contributor

I didn't think my comment through. I think a better explanation of what you want would be "excess property checks for destructured arguments".

@remcohaszing remcohaszing changed the title Treat destructured function arguments as if they are picked Excess property checks for destructured arguments Jan 20, 2021
@RyanCavanaugh
Copy link
Member

You're proposing that something currently OK to be made an error instead, which is sort of the definition of breaking change.

Parameter destructuring is considered to be an implementation detail, not an externally-visible facet of the function signature, so we don't modify the callee's view of the call based on whether or not the function uses destructuring. This is desirable both from a predictability perspective, and because if you really did want someone to pass a User, you'd have no way to indicate under this proposal.

Using Pick as shown in the OP is the right thing to do if you want the desired behavior.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jan 20, 2021
@typescript-bot
Copy link
Collaborator

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

@mindplay-dk
Copy link

You're proposing that something currently OK to be made an error instead, which is sort of the definition of breaking change.

It's a "breaking change" only for things that were provably incorrect to begin with. πŸ€”

If we explicitly wrote out the type-hint, like so:

interface User {
  id: string;
  name: string;
  email: string;
}

function stringifyUser({ name, email }: Pick<User, "name" | "email">): string {
  return `${name} (${email})`
}

What you've declared here, is that the function only depends on a subset of properties - which is exactly what you're declaring when you destructure.

Passing unused extra properties should be an error:

stringifyUser({ id: '1', name: 'Developer', email: '[email protected]' })

Why would the function demand values it can't access?

It's only a breaking change for cases where you are provably passing things you don't need, and can't actually access at all.

In any other case, such as when a compatible object originates from anywhere else than the call site, it's fine:

const u: User = { id: '1', name: 'Developer', email: '[email protected]' };

stringifyUser(u);

Even in strict mode, this is fine.

Parameter destructuring is considered to be an implementation detail, not an externally-visible facet of the function signature, so we don't modify the callee's view of the call based on whether or not the function uses destructuring. This is desirable both from a predictability perspective, and because if you really did want someone to pass a User, you'd have no way to indicate under this proposal.

But that's the intention.

Why would you force someone to pass a User, if only a subset of the properties are actually dependencies? If and when an actual User instance is a dependency, you would type-hint that as user: User - you wouldn't destructure an object if the whole object was a dependency.

As I wrote in #48220:

In general, there is no point in passing arguments to functions that can't even see them - destructuring hides the original object, and the function only receives the individual properties; since it can't access the object itself, it ultimately doesn't matter what the type of that argument is.

The way I see it, asking for things you can't use or access is bad practice.

When { name, email }: User demands you pass objects with a unused id, at most, this provides an "increased probability" that what you're passing is the expected kind of User and not just something resembling a User.

But there's no guarantee either way - an Admin type could have the exact same properties as a User, and you could still create bugs. But nothing that affects a function like this one, where it's true dependency is actually just at type with those two properties.

If we wanted to get around that problem, we would need nominal types, one of the earliest feature requests in the history of TypeScript.

Short of that, type-checks in TS are structural - everything is "duck typed", so, in my opinion, this proposal perfectly aligns with the philosophy of being able to call a function with whatever the function actually needs, without having to "prove" anything.

In the example used here, passing a literal id: '1' to this function very likely is a bug. What was the caller expecting? The function doesn't need and cannot use this value for anything - the function does what it was intended to do without this useless extra value. (That's why type-checking is closer to "nominal" when passing object literals in the first place, right?)

Plenty of changes have been introduced over the years that made certain code "invalid" according to new and stricter standards - if improved strictness helps identify dead or incorrect code, as it does in the example here, those aren't "breaking changes", are they?

If those are breaking changes, those are the kind of breaking changes I want. πŸ˜„

Can you point to anything else about this feature that would point to an real problem?

Please give this issue more careful consideration. πŸ™

@RyanCavanaugh
Copy link
Member

What we have now is the desired behavior, and that we consider the proposed alternative to be extremely confusing at best. If you want the other behavior, it's there with Pick. If we changed this, people would have no way to opt back in to the behavior that has been in place for many years with approximately four complaints about it ever.

It fails several major predictability tests: It's generally accepted that constructs should work like their desugared equivalents, which this definitely does not. Everywhere else in TypeScript, a type annotation is the "last word" in terms of typing, and this would be a special case for that.

It's a major refactoring hazard, because code using destructuring can't opt back in to using properties on the declared type later without incurring the risk that callers of the function written in the meantime didn't provide those properties.

If we did this, it would quickly become accepted guidance to never use destructuring in TypeScript because it would create such unpredictable effects on checking.

@mindplay-dk
Copy link

This sounds a bit hypothetical.

If we changed this, people would have no way to opt back in to the behavior that has been in place for many years with approximately four complaints about it ever.

Who would need to opt back into dependencies on properties they don't actually depend on and why?

It's generally accepted that constructs should work like their desugared equivalents, which this definitely does not. Everywhere else in TypeScript, a type annotation is the "last word" in terms of typing, and this would be a special case for that.

The way I see it, destructuring says: "my function depends on an object with these properties" - with the type annotation, you're saying "the destructured properties use types from this object type".

This would be exactly equivalent to the what destructuring actually means and does.

(I don't know what you mean by "desugared equivalents"? We're talking about destructuring, which is a language feature and not sugar - and types, which have no run-time footprint. What "desugars"?)

It's a major refactoring hazard, because code using destructuring can't opt back in to using properties on the declared type later without incurring the risk that callers of the function written in the meantime didn't provide those properties.

So it's better for functions to require a lot of values now, in case you need them in the future?

I just don't follow. That's just not the way I do programming. If something is not a dependency, I do my best not to make it a dependency. Introducing new dependencies to a function is and should be a breaking change.

If you see this in plain JS:

function stringifyUser({ name, email }) { ... }

Are you going to start passing id fields to every call?

In JS, destructuring is the closest analog we have to named arguments - adding a new argument is a breaking change. In JS, destructuring is also the closest thing we have to typed arguments - { name, email } says "the parameter type is an object with these properties".

I don't know why TS should demand more of a type than JS itself demands. More precision in what it demands, yes - but demanding properties that the underlying JS code doesn't ask for? That doesn't seem to add precision. If anything, it's less accurate than vanilla JS on this point.

If we did this, it would quickly become accepted guidance to never use destructuring in TypeScript because it would create such unpredictable effects on checking.

In a language with structural type-checking, these effects are completely predictable. We're talking about making the type-system accurately reflect what JS does when you destructure arguments.

I think this comes back to the nominal vs structural issue.

You've chosen structural type-checking (for which I love TS) but it seems like you're using the lack of a nominal type-checking option as an argument to keep types wider than necessary? In my opinion, this provides, at best, a false sense of security. (the Admin vs User example in my last comment.)

I'm not trying to say there aren't cases where those concerns are valid - there most definitely is. But wider than necessary type checking doesn't solve that - or if and when it does, that's just lucky coincidence. The reality with structural type-checking is it's predicated on having "probabilities of correctness" - as opposed to "guarantees", which only a nominal type designation (or perhaps nominal type-hints) could address that.

Can you provide an example of where and how this change would be harmful in practice?

My hope is that either you realize while trying to do so that your concerns are imagined - or that you prove me wrong. πŸ˜„

I would be happy with either outcome. I just haven't seen anything yet that points to a practical problem with this idea - what was initially described as "breakage" just looks to me like bad code getting caught by better type-checking, which is why I use TS. πŸ™ƒ

@RyanCavanaugh
Copy link
Member

Can you provide an example of where and how this change would be harmful in practice?

Sure:

function isEmptyArray({ length }: unknown[]) {
    return length === 0;
}
// No error??? TypeScript is broken?
isEmptyArray("hello world");

You could also imagine something like this:

// Duration for for some number of milliseconds
type Millisecond = { units: "ms", length: number };
// Duration for for some number of seconds
type Second = { unit: "s", length: number };

const oneSecond: Second = { unit: "s", length: 1 };
// Why this only sleeps for 1ms? TypeScript bug?
sleepFor(oneSecond);

function sleepFor({ length }: Millisecond) {
    Thread.sleep(length);
}

etc.

It's just not the case that every property stands alone as a singularly interpretable thing. Often, other properties are critical to the interpretation of data, whether that be about format, meaning, or context.

@mindplay-dk
Copy link

It's just not the case that every property stands alone as a singularly interpretable thing.

If you destructure every property into a free standing variable, that implies each property can stand alone as "a singularly interpretable thing". It literally is.

Often, other properties are critical to the interpretation of data, whether that be about format, meaning, or context.

Isn't that the trade-off you make for the convenience of structural type-checking though?

Essentially, every type-check is probabilistic.

You've showed two examples essentially with code that hopes for a stricter type-check than what's actually required to implement a working function - and surely this has better odds of achieving type-safety, but both of those examples will produce what I think is a misleading kind of safety:

  • You've made it appear as though isEmptyArray does something with arrays, when in fact the function could have been hasZeroLength({ length }: { length: number}) and would perform the same function in a more general way, more clearly expressing the input it takes into account, and the operation it performs.

  • You've made it appear as though sleepFor will somehow take into account the unit discriminator, which it will not.

You want the the type-system to check for assumptions or implications about certain types - you're essentially trying or hoping for some way to verify the identity of the type, but what you're getting is still only some degree of likelihood and not a guarantee. You're never really going to get that with structural type-checking - only nominal type-checks truly give you that.

To be clear, I do see the practical problems you're pointing out with this change.

I don't agree that it's a breaking change, because your code would still work - some of your type-checks wouldn't be as narrow as you'd have liked, but nothing would break.

If this feature were implemented, there would need to be an opt-in to check for unused properties in those cases where you were hoping for more type-checking than what is actually required by the function implementation - maybe something like sleepFor({ length, ... }: Millisecond), essentially creating a dependency on the rest of the properties by "destructuring them to nowhere".

As argued, this is not really safe or explicit either though.

In many cases, a type like Millisecond would be much better off with a nominal type-check - requiring you not only to pass a compatible structure, but also explicitly indicating at the call site the meaning and expected interpretation of that structure, e.g.:

nominal type Millisecond = { unit: "ms", length: number };
// πŸ‘† structural compatibility is insufficient for this type

const time = {
  unit: "ms",
  length: 1,
  weekday: "monday", // πŸ‘ˆ some unrelated data in this inferred type
};

sleepFor(time); // untyped structure - could be Millisecond or just happens to be compatible πŸ‘Ž

sleepFor(time as Millisecond); // type cast required to indicate the expected interpretation πŸ‘

I think, at the end of the day, this kind of discussion comes down to opinions. There are those of us who would have wished for soundness - and maybe it seems odd for me, being one of those people, arguing for less strictness in this case. But soundness is not only about being strict, it's also about being accurate - it's hard to argue the inference is accurate here, and it definitely gets in the way of writing tests. There are practical reason why I've spent time debating this issue.

I do think maybe this feature is at an impasse - with so much code already in the wild, destructuring out of convenience, rather than with any real consideration for whether each property is a "singularly interpretable thing", maybe it is too late to fix this without causing too much upset...

(Fingers crossed for a "sound": true option someday, ha ha, but yes I understand this was never your goal. I do think you would have had an easier time if you had stuck to an objective set of rules and principles, rather than needing to weigh the importance of each developer's use-cases, always needing to decide which one is more important...)

@RyanCavanaugh
Copy link
Member

If you destructure every property into a free standing variable, that implies each property can stand alone as "a singularly interpretable thing". It literally is

This is the root of the question-begging: You're making the claim that if a property gets destructured, then it has context-free meaning, therefore a function using destructuring should have different semantics. But it presupposes the thing being debated - that destructuring is a semantic differentiator rather than a syntactic convenience.

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

5 participants