Perform excess property checking on intersection and union members #30853
Conversation
weswigham
requested review from
sandersn,
DanielRosenwasser,
ahejlsberg and
RyanCavanaugh
|
@typescript-bot test this & @typescript-bot run dt & @typescript-bot user test this What a command. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at ac0af46. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the community code test suite on this PR at ac0af46. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at ac0af46. You can monitor the build here. It should now contribute to this PR's status checks. |
|
RWC Summary
Extracted: interface IStringDictionary<V> {
[name: string]: V;
}
interface INumberDictionary<V> {
[idx: number]: V;
}
declare function forEach<T>(from: IStringDictionary<T> | INumberDictionary<T>, callback: (entry: { key: any; value: T; }, remove: () => void) => any);
let count = 0;
collections.forEach({ toString: 123 }, () => count++);
~~~~~~~~
!!! error TS2322: Type 'number' is not assignable to type '() => string'.I don't really disagree with the error but it'd be good to understand why it popped up |
|
Yeah, I saw that and was equally confused. Definitely need to look at 1. what that code's even meant to be doing and 2. why does this affect it? DT results also seem noisy (there's failures on latest master), but there's around 16 new changes there - they mostly look like new excess prop errors (example: |
src/compiler/checker.ts
Outdated
| result = typeRelatedToEachType(getRegularTypeOfObjectLiteral(source), target as IntersectionType, reportErrors); | ||
| if (result && isPerformingExcessPropertyChecks) { | ||
| // Validate against excess props using the original `source` | ||
| if (!propertiesRelatedTo(source, target, reportErrors)) { |
There was a problem hiding this comment.
Is the propertiesRelatedTo check more expensive than the typeRelatedToEachType check? Does it make sense to do the checks in the other order?
There was a problem hiding this comment.
I think usually, yeah - we typically consider structural decomposition more expensive than algebraic comparison.
There was a problem hiding this comment.
Ah ok, fair enough. I thought that typeRelatedToEachType would end up calling into propertiesRelatedTo for each union component anyway via isRelatedTo.
There was a problem hiding this comment.
It can; but that's the last kind of comparison we make - any other comparison succeeding avoids it.
There was a problem hiding this comment.
That makes sense. Ty!
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at f87ba3f. You can monitor the build here. It should now contribute to this PR's status checks. |
|
That error message looks suspiciously like what you'd get from an |
|
Wew, so having finally finished going thru them, every related DT break seemed like a really good change where we were catching a bug in either the test code or the type definitions. I've opened: to fix up those breaks on DT - all of them are corrections or improvements to the tests or declarations for a package. |
Fix't. The |
src/compiler/types.ts
Outdated
| HasLiteralType = 1 << 7, // Synthetic property with at least one literal type in constituents | ||
| ContainsPublic = 1 << 8, // Synthetic property with public constituent(s) | ||
| ContainsProtected = 1 << 9, // Synthetic property with protected constituent(s) | ||
| ContainsPrivate = 1 << 10, // Synthetic property with private constituent(s) |
|
Before I merge: @typescript-bot test this & @typescript-bot run dt & @typescript-bot user test this |
|
Heya @weswigham, I've started to run the community code test suite on this PR at f54e5ab. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at f54e5ab. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at f54e5ab. You can monitor the build here. It should now contribute to this PR's status checks. |
Found while working on microsoft/TypeScript#30853 - asd is never augmented into HandlerDecorations in any tests, so it's excess and, with the mentioned PR, an error. #34701 but again for the new location of the types, pretty much.
|
Only DT failure is a new copy of a hapi test I already fixed once (they got copied to a namespace while I was working, apparently); opened DefinitelyTyped/DefinitelyTyped#34790, RWC changes are the remaining error message changes; so everything looks good. Going to merge~ |
Found while working on microsoft/TypeScript#30853 - asd is never augmented into HandlerDecorations in any tests, so it's excess and, with the mentioned PR, an error. #34701 but again for the new location of the types, pretty much.
Found while working on microsoft/TypeScript#30853 - asd is never augmented into HandlerDecorations in any tests, so it's excess and, with the mentioned PR, an error. DefinitelyTyped#34701 but again for the new location of the types, pretty much.
Fixes #28616
Fixes #18075
Fixes #13813
cc @JasonGore - I believe this enhances excess property checking to the point where it should cover all the faculties JSX tags in 2.8 had.