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

setter inferred from union has incorrect variance #56894

Closed
craigphicks opened this issue Dec 29, 2023 · 8 comments
Closed

setter inferred from union has incorrect variance #56894

craigphicks opened this issue Dec 29, 2023 · 8 comments

Comments

@craigphicks
Copy link

craigphicks commented Dec 29, 2023

πŸ”Ž Search Terms

setter union variance

is:issue in-title: setter (nothing relevant found)

is:issue in-title: setter union variance (nothing relevant found)

πŸ•— Version & Regression Information

confirmed 4.3.5 ~ 5.3.2

⏯ Playground Link

playground

πŸ’» Code

declare const id:<T>()=>T

function f2(x: {f:((a:1|2)=>1|2)} | {f:((a:2|3)=>2|3)} ){
    const r = x.f(id())
//                ^ const id: <2>() => 2,
//        ^ const r: 1 | 2 | 3
}

function f3(f: ((a:1|2)=>1|2) | ((a:2|3)=>2|3) ){
    const r = f(id())
//              ^ const id: <2>() => 2,  
//        ^ const r: 1 | 2 | 3
}


function f4(x: {set d(a:1|2); get r():1|2} | {set d(a:2|3); get r():2|3} ){
//           ^ (parameter) x: { d: 1 | 2; readonly r: 1 | 2; } | { d: 2 | 3; readonly r: 2 | 3; }
    x.d = id();
//        ^ const id: <1 | 2 | 3>() => 1 | 2 | 3
    x.r; 
//    ^    (property) r: 1 | 2 | 3
}

πŸ™ Actual behavior

function f4(x: {set d(a:1|2); get r():1|2} | {set d(a:2|3); get r():2|3} ){
    x.d = id();
//        ^ const id: <1 | 2 | 3>() => 1 | 2 | 3

The setter function has range 1|2|3.

πŸ™‚ Expected behavior

function f4(x: {set d(a:1|2); get r():1|2} | {set d(a:2|3); get r():2|3} ){
//           ^ (parameter) x: {set d(a:1|2); get r():1|2;} | {set d(a:2|3); get r():2|3;}
    x.d = id();
//        ^ const id: <2>() => 2
  • The setter function has range 2, because that is the valid range that can be assigned to both setter functions in the union.
    • Compare to the ranges 2 and 2 computed for function parameters a in f2 and f3.
  • The type display for x should show getters and setters instead of plain (readonly for getter) properties.

Additional information about the issue

  • The parameter range of the union of two functions is the intersection of those two functions parameter ranges. That is because anything outside of that intersection would be invalid input in at least one of the two.

  • Theoretically, writes and reads of plain properties of unions of object could also "legally" have opposite variances, with writes being narrower - but they are not treated that way, which although unsound is ok because type widening is allowed for convenience.

  • For the special cases where user want a setter, or setter-getter same-name pair, to behave write-unsoundly exactly like a plain property, they can specify the type as being a plain type, e.g. { d: T } instead of { set d(value:T): }. That's an argument for allowing write-sound behavior by default in a type definition { set d(value:T): }.

  • The setters are converted to plain properties (and the getters are converted to a readonly plain properties (probably at point of declaration) as can be seen by tool tipping to x which shows (parameter) x: { d: 1 | 2; readonly r: 1 | 2; } | { d: 2 | 3; readonly r: 2 | 3; }. Related to Flow is improperly narrowing getter valuesΒ #56899.

@Andarist
Copy link
Contributor

The issue title should mention setters here :p I prepared a fix for this one: #56895

I also have found that variance is actually computed incorrectly (or rather... it's not observable at all):

// this really should be `in` but you can make it whatever you want here, TS won't complain
type MyType<out T> = {
  set k(a: T);
  get k(): unknown;
};

The problem is that isPropertySymbolTypeRelated doesn't account for write types at all. Perhaps this deserves a new issue?

@fatcerberus
Copy link

Theoretically, writes and reads of plain properties of unions of object could also "legally" have opposite variances

They actually do, in certain cases. See e.g. #30769.

@craigphicks craigphicks changed the title getter inferred from union has incorrect variance setter inferred from union has incorrect variance Dec 29, 2023
@craigphicks
Copy link
Author

craigphicks commented Dec 29, 2023

@Andarist

The issue title should mention setters here :p I prepared a fix for this one: #56895

I also have found that variance is actually computed incorrectly (or rather... it's not observable at all):

// this really should be `in` but you can make it whatever you want here, TS won't complain
type MyType<out T> = {
  set k(a: T);
  get k(): unknown;
};

The problem is that isPropertySymbolTypeRelated doesn't account for write types at all. Perhaps this deserves a new issue?

Please help me. I don't understand why in or out are associated with types rather than property names, how out differs from readonly, and how in differs from set <property name>(value): void.

@craigphicks
Copy link
Author

craigphicks commented Dec 29, 2023

I am re-opening this issue because because it seems to have been incorrectly closed without any resolution attributed.

@craigphicks craigphicks reopened this Dec 29, 2023
@craigphicks
Copy link
Author

craigphicks commented Dec 29, 2023

@fatcerberus

Theoretically, writes and reads of plain properties of unions of object could also "legally" have opposite variances

They actually do, in certain cases. See e.g. #30769.

Great reference. However, I am puzzled by this test case:

function f3<T extends { [key: string]: any }>(obj: T) {
    let foo = obj['foo'];
    let bar = obj['bar'];
    obj['foo'] = 123;  // Error
    obj['bar'] = 'x';  // Error
}

I can't see the reasoning for the errors. This is runtime-error-free node JS:

> obj = {}
{}
> obj.a = 1
1
> obj["b"] = 2
2

@Andarist
Copy link
Contributor

It likely prevents invalid writes in situations like this:

declare const obj: {
  foo: boolean;
  bar: boolean;
};

f3(obj);

@craigphicks
Copy link
Author

craigphicks commented Dec 29, 2023

It likely prevents invalid writes in situations like this:

declare const obj: {
  foo: boolean;
  bar: boolean;
};

f3(obj);

Another function format exists to get that same error behavior:

function f5<T extends Readonly<{ [key: string]: any }>>(obj: T) {
    let foo = obj['foo'];
    let bar = obj['bar'];
    obj['foo'] = 123;  // Error
    obj['bar'] = 'x';  // Error
}
f5(obj);

What function should the user call when they don't want those errors?
I think it would be better to trust that the user is reading the function declaration and understands what it means.

@craigphicks
Copy link
Author

Closing as this is superseded by #56922

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