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

Narrow object by property #33205

Closed
5 tasks done
MicahZoltu opened this issue Sep 3, 2019 · 16 comments
Closed
5 tasks done

Narrow object by property #33205

MicahZoltu opened this issue Sep 3, 2019 · 16 comments
Labels
Duplicate An existing issue was already created

Comments

@MicahZoltu
Copy link
Contributor

MicahZoltu commented Sep 3, 2019

Search Terms

narrow object by property
(unfortunately, this returned way too many results to look through, so sorry if this is a dupe!)

Suggestion

Narrow objects based on checks of their properties.

Use Cases

Some function receives an object as input and then does validation on that object prior to passing it on to some other function, or returning it as a narrowed object.

Examples

declare const apple: { readonly prop: number }
if (apple.prop !== 2) throw new Error()
const appleProp: 2 = apple.prop // no error

// Type 'Apple' is not assignable to type '{ prop: 2; }'.
//   Types of property 'prop' are incompatible.
//     Type 'number' is not assignable to type '2'.
const banana: {prop:2} = apple

In this example, we can see that the compiler is tracking the narrowed type of apple.prop because it allows us to assign it to a variable of type 2. However, when we try to assign the containing object to something that wants a narrowed prop we get a type error.

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, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 3, 2019

I am against this being allowed.

const banana: {prop:2} = apple
apple.prop = 5
console.log(banana.prop) //5, not 2, whoops

@MicahZoltu
Copy link
Contributor Author

I updated original to make apple.prop readonly. Though, your example does show why this may be harder to implement than I original thought, since it needs to only narrow for readonly properties.

@AnyhowStep
Copy link
Contributor

Copy-pasting from Gitter,

I understand your pain because, in some cases, you know that apple's props won't be modified for the duration that banana is in scope. So, at that point, the assignment to banana is sound.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 3, 2019

Also, readonly isn't a guarantee that the property won't be modified, even if TS started handling readonly "properly".

const writable = { prop : 2 }
const apple : { readonly prop : number } = writable
//Snip narrow apple.prop to 2
const banana : { prop : 2 } = apple
writable.prop = 9001
console.log(banana.prop) //9001, not 2, whoops

We can't modify apple.prop because it is readonly but writable.prop is fair game.

@MicahZoltu
Copy link
Contributor Author

@AnyhowStep Is the fact that you can assign writable to apple in your example a bug? It feels like a bug since you are narrowing writable without any explicit assertion or type guard.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Sep 3, 2019

It's not a bug.
{ prop : number } is assignable to { readonly prop : number }

The current meaning of readonly is,

This property (prop) of the object is not modifiable through the identifier (apple) pointing to the object.

We are not saying,

All references to the object cannot modify this property


If readonly meant the latter, then it would be a bug to allow the assignment from writable to apple.


Perhaps more surprising is that { readonly prop : number } is assignable to { prop : number } (if memory serves, I'm not on the computer)

But if we keep in mind what readonly currently means, then it "makes sense".

It does limit the usefulness of readonly, in my opinion, but readonly is still useful, despite its shortcomings

const apple : { readonly prop : number } = { prop : 2 }
const writable : { prop : number } = apple
writable.prop = 1337

@fatcerberus
Copy link

Perhaps more surprising is that { readonly prop : number } is assignable to { prop : number } (if memory serves, I'm not on the computer)

IIRC this is historical. readonly was a later addition to the language and there’s not yet a writeable annotation, so for compatibility with existing code, it can’t disallow the assignment because there was (at one time) no way to distinguish read-only from writable.

I’m reasonably sure most of the unsoundness surrounding readonly traces to the above historical note.

@jack-williams
Copy link
Collaborator

jack-williams commented Sep 3, 2019

Related #29827, #30506, #31755.

@sandersn
Copy link
Member

sandersn commented Sep 3, 2019

@MicahZoltu can you give an example that doesn't involve numbers? Right now it sounds like you want number to be treated as the union of all numbers. This is not feasible in our current system but might be possible by introducing numeric ranges into the type system.

I also do not understand the intent of disallowing

typeof apple{ prop: 2 }

but allowing

(typeof apple)['prop']2.

Did the second example predate the addition of readonly?

@sandersn sandersn added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Sep 3, 2019
@AnyhowStep
Copy link
Contributor

typeof apple{ prop: 2 } should be disallowed because,

#33205 (comment)

@fatcerberus
Copy link

@sandersn From the discussion on Gitter that led to this issue, the main point of contention was that apple.prop itself gets narrowed to 2 but typeof apple doesn’t change. So depending on how you use apple after narrowing, you’re in an odd state where prop is simultaneously narrowed to 2 or not, depending if you rely on the narrowing directly or indirectly.

What @MicahZoltu wanted, essentially, was for typeof apple to become Apple & { prop: 2 } after the typeguard.

@MicahZoltu
Copy link
Contributor Author

@fatcerberus is correct. To help exemplify the problem:

declare const apple: { readonly prop: number }
if (apple.prop !== 2) throw new Error()
const appleProp: 2 = apple.prop // no error

// Type '{ readonly prop: number; }' is not assignable to type '{ readonly prop: 2; }'.
//   Types of property 'prop' are incompatible.
//     Type 'number' is not assignable to type '2'.
const banana: {readonly prop:2} = apple // error
const cherry: {readonly prop:2} = {prop: apple.prop}

The assignment to cherry shows that clearly TS knows that apple.prop is of type 2, not number, yet when I try to assign the object itself it doesn't agree with that. I suspect this is because of the reasons that @AnyhowStep went into above in that readonly doesn't actually mean readonly in TypeScript, which makes my blood boil but at least explains the oddity here.

Reading the issues linked above, a comment from @jack-williams in one of them indicates this is a design constraint:

Type guards do not propagate type narrowings to parent objects. The narrowing is only applied upon access of the narrowed property which is why the destructing function works, but the reference function does not. Narrowing the parent would involve synthesizing new types which would be expensive.

Also, it would seem (based on @AnyhowStep's commentary above) that TypeScript thinks that while apple.prop is currently 2, there is no guarantee that it will remain 2 indefinitely. This is because readonly appears to not mean what one thinks it means, and in fact apple.prop can be assigned any number later on, so TS cannot narrow apple.prop to 2.

So IIUC, the current behavior is a design constraint, and @AnyhowStep thinks that even if it wasn't, it should not be fixed because readonly is a lie.

@sandersn
Copy link
Member

sandersn commented Sep 4, 2019

@MicahZoltu Oh, I misunderstood your idea, and also didn't know that we already narrow numbers to numeric literals. Anyway, as @jack-williams says, this is exactly a duplicate of previous issues. And since it would require the compiler to synthesise new types, we haven't done it because of the performance implications.

@sandersn sandersn added Duplicate An existing issue was already created and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Sep 4, 2019
@MicahZoltu
Copy link
Contributor Author

For future readers, this is a duplicate of #31755, which is a duplicate of #30506, which is closed as a Design Limitation (though it is missing a tag).

The crux of the problem is:

Type guards do not propagate type narrowings to parent objects. The narrowing is only applied upon access of the narrowed property which is why the destructing function works, but the reference function does not.

The reason this is a Design Limitation is because:

Narrowing the parent would involve synthesizing new types which would be expensive.

@typescript-bot
Copy link
Collaborator

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

@typescript-bot
Copy link
Collaborator

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

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

6 participants