Fixed propagating object flags when dealing with spreads #58180
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
|
|
||
| const bar = { | ||
| p: null, // no error, this gets overriden | ||
| other: null, // error, this does not get overriden |
There was a problem hiding this comment.
I am confused. This is the behaviour I get when I paste the test into the playground and turn off strictNullChecks. What does this PR fix about this test?
Also, this isn't the case mentioned in the linked comment, as far as I can tell.
There was a problem hiding this comment.
To be honest... I'm confused too. I've checked out previous commits, re-run this test and all and it doesn't make a lot of sense to me why I have added this here.
Maybe this is a test case validating that I didn't break this behavior with my changes and I forgot to add a test case from the referenced comment? That case from the comment does exhibit a change. I just added it to the test file here and u can compare it with the current non-sensical behavior here
src/compiler/checker.ts
Outdated
| @@ -19023,7 +19023,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const skippedPrivateMembers = new Set<__String>(); | |||
| const indexInfos = left === emptyObjectType ? getIndexInfosOfType(right) : getUnionIndexInfos([left, right]); | |||
|
|
|||
| // propagating flags of the right properties should be preserved if they override the left properties | |||
There was a problem hiding this comment.
this comment doesn't describe the code change as far as I can see; from my reading propagating flags of right properties are propagated up to the containing object. Overriding doesn't seem related. Am I misunderstanding things?
There was a problem hiding this comment.
No, i dont think u are misunderstanding - this comment wasn't super clear. I pushed out a change to make it more specific, I hope this helps
| // @strictNullChecks: false | ||
| // @noImplicitAny: true |
There was a problem hiding this comment.
This is a weird combination of flags -- maybe common in 2016 or so, but not now. Is it possible to observe the effect of adding finalObjectFlags with other flags? I'm default sceptical of fixes for anything with strictNullChecks or noImplicitAny off, but especially this combination of them.
There was a problem hiding this comment.
I default all the tests I add to @strict: true, @noEmit: true. This combination of flags is important to showcase the behavior change here.
I agree it would be great if we'd have some other test case that wouldn't require those. I'll try to come up with some, it might be the easiest with ObjectFlags.ContainsObjectOrArrayLiteral.
There was a problem hiding this comment.
Hm, I have tried to come up with a test case that would rely on the change in how ObjectFlags.ContainsObjectOrArrayLiteral or ObjectFlags.NonInferrableType might get propagated but I failed.
There was a problem hiding this comment.
this "conflicted" with the recently merged #58157
The resulting type of the variable here is { c?: undefined; p: any; s: any; } | { c: string; p: string; s: string; }. I think that in this situation it should error as bar.p results in any. But at the same time, i think it will be easier/better to fix the union normalization in this case so the result of this would become { c?: undefined; p: string; s: string; } | { c: string; p: string; s: string; }
There was a problem hiding this comment.
After more thinking, I'm not sure if this would be the right solution. @strictNullChecks: false, @noImplicitAnt: true is really a confusing combination.
This behavior doesn't help me either when it comes to determining what's the right solution here:
// @strictNullChecks: false
// @noImplicitAny: true
let test1 = null; // no error 😱
let test2 = {
prop: null, // Object literal's property 'prop' implicitly has an 'any' type.(7018)
};It all depends on what kind of conceptual safety this combination of settings is supposed to give the user.
spotted by @fatcerberus here: #58150 (comment)