Require at least a single match when defining discriminable properties #53654
Conversation
typescript-bot
added
the
For Milestone Bug
MariaSolOs
changed the title
| } | ||
| else { | ||
| discriminable[i] = false; | ||
| if (discriminable[i] === undefined) { |
There was a problem hiding this comment.
To explain this change: This line says that "discriminated types stop at 'true'", but the code is overwriting a potentially true discriminable[i] to false if one of the union variants has a property with a type that's unrelated to the target type (which is what's happening when the property's value is an incomplete string literal).
| Type 'S' is not assignable to type '{ a: N; b: N; c: 2; }'. | ||
| Types of property 'c' are incompatible. | ||
| Type 'N' is not assignable to type '2'. | ||
| Type '0' is not assignable to type '2'. |
There was a problem hiding this comment.
I'm not a fan of these errors not being included anymore and I'll try to keep them, but IMO the diagnostic changes in the other files do make sense.
There was a problem hiding this comment.
@DanielRosenwasser @ahejlsberg Wait I think that those errors are a bug (?). The example says that s should be assignable to t (and if you look at it, it is), but it errors because of the number of combinations being too complex. The diagnostics are incorrect, and so I actually prefer them going away.
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 86295fb. You can monitor the build here. |
|
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 86295fb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite on this PR at 86295fb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite on this PR at 86295fb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite (tsserver) on this PR at 86295fb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the parallelized Definitely Typed test suite on this PR at 86295fb. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite (tsserver) on this PR at 86295fb. You can monitor the build here. Update: The results are in! |
|
@RyanCavanaugh Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@RyanCavanaugh Here are the results of running the user test suite comparing Everything looks good! |
|
@RyanCavanaugh Here they are:
CompilerComparison Report - main..53654
System
Hosts
Scenarios
TSServerComparison Report - main..53654
System
Hosts
Scenarios
StartupComparison Report - main..53654
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @RyanCavanaugh, the results of running the DT tests are ready. |
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailsbackstage/backstageThat is a filtered view of the text. To see the raw error text, go to RepoResults4/backstage.backstage.rawError.txt in the artifact folder
|
Ehhh this looks unrelated to this change... right? |
I think so. |
src/compiler/checker.ts
Outdated
| if (discriminable[i] === undefined) { | ||
| discriminable[i] = !!(targetType && related(getDiscriminatingType(), targetType)); |
There was a problem hiding this comment.
What happens if you switch this to check discriminable[i] !== true so that as long as some type has a successful discriminant, the property is discriminable?
Or alternatively,
for (const type of target.types) {
const targetType = getTypeOfPropertyOfType(type, propertyName);
const targetIsDiscriminable = discriminable[i] = !!(targetType && related(getDiscriminatingType(), targetType));
if (targetIsDiscriminable) {
break;
}
i++
}There was a problem hiding this comment.
That doesn't change the logic though, it just optimizes the looping a bit.
There was a problem hiding this comment.
I do like your suggestion BTW, just making sure I'm not missing something.
There was a problem hiding this comment.
Doesn't it?
In the original version, a single false takes precedence over everything else.
With your change, the first result takes precedence over every other (which sounds suspicious - this doesn't even need to be a loop anymore).
In my suggestion, a single true takes precedence over any other result.
There was a problem hiding this comment.
Right, that makes sense. Boolean logic gets tricky with truthy and falsy stuff.
There was a problem hiding this comment.
Also, with my change the first result doesn't always take precedence, because the objects in the union could potentially contain different items and so the writes to discriminable could differ between iterations.
There was a problem hiding this comment.
@DanielRosenwasser your suggested change breaks this test (among other suspicious diagnostic changes).
foo2({
type2: 'y',
~~~~~
!!! error TS2345: Argument of type '{ type2: string; value: string; method(): void; }' is not assignable to parameter of type 'X2 | Y2'.
!!! error TS2345: Object literal may only specify known properties, but 'type2' does not exist in type 'X2'. Did you mean to write 'type1'?
value: 'done',
method() {
this;
this.value;
}
});
|
@typescript-bot perf test this faster |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at b267256. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:Comparison Report - main..53654
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The fix in this PR unfortunately doesn't work right in all cases. Consider: type Foo = { x: 0, y: 0, z: 'a' } | { x: 0, y: 1, z: 'b' } | { x: 1, y: 0, z: 'c' } | { x: 1, y: 1, z: 'd' };
let foo: Foo = {
x: 0,
y: 1,
z: // Suggests "a" | "b" here
}Above, only In general, I find the original logic in I'm going to put up a PR with the suggested changes. |
|
@ahejlsberg oh well, I tried 😅 Thank you for coming up with a proper solution! |
|
Closing as this PR has been superseded by #53709 |
Fixes #53165