Fixed union/intersection write types #56895
Conversation
typescript-bot
added
the
For Uncommitted Bug
| @@ -14674,7 +14674,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| else { | |||
| result.links.type = isUnion ? getUnionType(propTypes) : getIntersectionType(propTypes); | |||
| if (writeTypes) { | |||
| result.links.writeType = isUnion ? getUnionType(writeTypes) : getIntersectionType(writeTypes); | |||
| result.links.writeType = isUnion ? getIntersectionType(writeTypes) : getUnionType(writeTypes); | |||
There was a problem hiding this comment.
after the original change has been merged it has been mentioned in the comments that likely this should work in the way I have adjusted it here but I don't see any open issue that would be the result of this discussion there
There was a problem hiding this comment.
- Currently the getters and setter are displayed as plain readonly property (for getter) and plain property (for setter). See setter inferred from union has incorrect variance #56894 and Flow is improperly narrowing getter values #56899 which show the display. I think the displayed types should show getter and setter declarations so the user can understand the difference in behavior.
- What about the case where
if (propTypes.length > 2){ ... }before thiselseclause? Isn't that branch handled later in different code which also has to be updated? - A question: Apparently the
writeTypesexists here for setters, because test results changed. But the tests results didn't change for plain types, sowriteTypesdoes not exist for them, right?
There was a problem hiding this comment.
- if I understand you correctly - what is displayed depends on the location. In write locations, the write type is displayed and in read locations, you get the read type. To change this the TS team would have to receive a well-motivated feature request with many examples of how it would differ from the current behavior.
- Yes - you are right. I'll take a look at this.
- What do you consider a plain type here? Could you rephrase this question?
|
I think that part of what you mention here is likely related to this and I'd treat this as another issue. If you prefer, I can remove the reference to your issue from this PR. |
Of course I prefer if this pull (appropriately modified) fixes #56894!!! |
|
I changed the referenced issue here as I don't want to fix the variance measurement for setters as part of this PR. |
|
@typescript-bot perf test this |
|
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 425647d. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the regular perf test suite on this PR at 425647d. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the diff-based user code test suite on this PR at 425647d. You can monitor the build here. Update: The results are in! |
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 425647d. You can monitor the build here. |
|
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@andrewbranch Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@andrewbranch Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I think the currently linked issue can be closed with this. From my PoV, its main focus is on those unsound writes that are fixed here. I agree that the other issue could be reopened to track the variance computation issue. |
|
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
|
I'm surprised I can't find discussion on this, but I think we discussed this when we added the divergent accessor type feature and decided to make the implementation consistent with the (obviously incorrect) way that normal properties work. This PR does not make this an error: class One {
x!: string
}
class Two {
x!: number
}
declare let u1: One | Two;
u1.x = "";I would like to see this fixed everywhere, but that's of course a much bigger and more disruptive change, and I'm not sure if it makes sense to fix one without the other. Even more confusing, the logic changed here doesn't even kick in unless the property has a getter with a different type. This is still not an error: declare class OneAccessor {
set x(value: string);
}
declare class TwoAccessor {
set x(value: number);
}
declare let u2: OneAccessor | TwoAccessor;
u2.x = "";And it's still not an error when adding getters of the same type. But once one of the getters—theoretically irrelevant to this assignment—diverges from its setter, suddenly the assignment is an error? No, unfortunately, I think this is wrong on purpose. |
To confirm - is this something that you want to get fixed? I could certainly try to do this, I'm just looking for a soft confirmation that fixing this is even on the table. As you have said, it could potentially be a big and disruptive change - and I know that sometimes the team errs on the side of backwards compatibility. |
|
I'm not sure when it was last discussed, but my impression was it's not on the table. I think the problem is that it's too slippery of a slope down to the realization that all non- let obj: { x: string } | { x: number };
obj.x = ""; // Error, right?
let obj2: { x: unknown } = obj; // ...Uh oh
obj2.x = ""; // Must be ok, so previous line must be error |
fixes #56922
cc @andrewbranch