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

Fixed propagating object flags when dealing with spreads #58180

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19495,7 +19495,11 @@ 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 always be preserved
// propagating flags of the left properties doesn't always happen since right properties can override them
let finalObjectFlags = objectFlags & ~ObjectFlags.PropagatingFlags;
for (const rightProp of getPropertiesOfType(right)) {
finalObjectFlags |= getObjectFlags(getTypeOfSymbol(rightProp)) & ObjectFlags.PropagatingFlags;
if (getDeclarationModifierFlagsFromSymbol(rightProp) & (ModifierFlags.Private | ModifierFlags.Protected)) {
skippedPrivateMembers.add(rightProp.escapedName);
}
Expand Down Expand Up @@ -19531,12 +19535,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}
else {
finalObjectFlags |= getObjectFlags(getTypeOfSymbol(leftProp)) & ObjectFlags.PropagatingFlags;
members.set(leftProp.escapedName, getSpreadSymbol(leftProp, readonly));
}
}

const spread = createAnonymousType(symbol, members, emptyArray, emptyArray, sameMap(indexInfos, info => getIndexInfoWithReadonly(info, readonly)));
spread.objectFlags |= ObjectFlags.ObjectLiteral | ObjectFlags.ContainsObjectOrArrayLiteral | ObjectFlags.ContainsSpread | objectFlags;
spread.objectFlags |= ObjectFlags.ObjectLiteral | ObjectFlags.ContainsObjectOrArrayLiteral | ObjectFlags.ContainsSpread | finalObjectFlags;
return spread;
}

Expand Down
44 changes: 44 additions & 0 deletions tests/baselines/reference/noImplicitAnySpreads1.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
noImplicitAnySpreads1.ts(18,3): error TS7018: Object literal's property 'other' implicitly has an 'any' type.


==== noImplicitAnySpreads1.ts (1 errors) ====
// https://github.com/microsoft/TypeScript/issues/58150#issuecomment-2052517378

function getMore() {
return {
c: "foo",
p: "bar",
s: "baz",
};
}

const foo = {
p: null,
...getMore(),
};

const bar = {
p: null, // no error, this gets overriden
other: null, // error, this does not get overriden
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

~~~~~~~~~~~
!!! error TS7018: Object literal's property 'other' implicitly has an 'any' type.
...getMore(),
};

function doSthWithParams(params: unknown) {
return {
c: 'foo',
p: 'bar',
s: 'baz',
};
}

const baz = {
p: null,
s: null,
...doSthWithParams({
p: 'hello',
s: 'world',
}),
};

84 changes: 84 additions & 0 deletions tests/baselines/reference/noImplicitAnySpreads1.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
//// [tests/cases/compiler/noImplicitAnySpreads1.ts] ////

=== noImplicitAnySpreads1.ts ===
// https://github.com/microsoft/TypeScript/issues/58150#issuecomment-2052517378

function getMore() {
>getMore : Symbol(getMore, Decl(noImplicitAnySpreads1.ts, 0, 0))

return {
c: "foo",
>c : Symbol(c, Decl(noImplicitAnySpreads1.ts, 3, 10))

p: "bar",
>p : Symbol(p, Decl(noImplicitAnySpreads1.ts, 4, 13))

s: "baz",
>s : Symbol(s, Decl(noImplicitAnySpreads1.ts, 5, 13))

};
}

const foo = {
>foo : Symbol(foo, Decl(noImplicitAnySpreads1.ts, 10, 5))

p: null,
>p : Symbol(p, Decl(noImplicitAnySpreads1.ts, 10, 13))

...getMore(),
>getMore : Symbol(getMore, Decl(noImplicitAnySpreads1.ts, 0, 0))

};

const bar = {
>bar : Symbol(bar, Decl(noImplicitAnySpreads1.ts, 15, 5))

p: null, // no error, this gets overriden
>p : Symbol(p, Decl(noImplicitAnySpreads1.ts, 15, 13))

other: null, // error, this does not get overriden
>other : Symbol(other, Decl(noImplicitAnySpreads1.ts, 16, 10))

...getMore(),
>getMore : Symbol(getMore, Decl(noImplicitAnySpreads1.ts, 0, 0))

};

function doSthWithParams(params: unknown) {
>doSthWithParams : Symbol(doSthWithParams, Decl(noImplicitAnySpreads1.ts, 19, 2))
>params : Symbol(params, Decl(noImplicitAnySpreads1.ts, 21, 25))

return {
c: 'foo',
>c : Symbol(c, Decl(noImplicitAnySpreads1.ts, 22, 10))

p: 'bar',
>p : Symbol(p, Decl(noImplicitAnySpreads1.ts, 23, 13))

s: 'baz',
>s : Symbol(s, Decl(noImplicitAnySpreads1.ts, 24, 13))

};
}

const baz = {
>baz : Symbol(baz, Decl(noImplicitAnySpreads1.ts, 29, 5))

p: null,
>p : Symbol(p, Decl(noImplicitAnySpreads1.ts, 29, 13))

s: null,
>s : Symbol(s, Decl(noImplicitAnySpreads1.ts, 30, 10))

...doSthWithParams({
>doSthWithParams : Symbol(doSthWithParams, Decl(noImplicitAnySpreads1.ts, 19, 2))

p: 'hello',
>p : Symbol(p, Decl(noImplicitAnySpreads1.ts, 32, 22))

s: 'world',
>s : Symbol(s, Decl(noImplicitAnySpreads1.ts, 33, 15))

}),
};

142 changes: 142 additions & 0 deletions tests/baselines/reference/noImplicitAnySpreads1.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
//// [tests/cases/compiler/noImplicitAnySpreads1.ts] ////

=== noImplicitAnySpreads1.ts ===
// https://github.com/microsoft/TypeScript/issues/58150#issuecomment-2052517378

function getMore() {
>getMore : () => { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

return {
>{ c: "foo", p: "bar", s: "baz", } : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

c: "foo",
>c : string
> : ^^^^^^
>"foo" : "foo"
> : ^^^^^

p: "bar",
>p : string
> : ^^^^^^
>"bar" : "bar"
> : ^^^^^

s: "baz",
>s : string
> : ^^^^^^
>"baz" : "baz"
> : ^^^^^

};
}

const foo = {
>foo : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>{ p: null, ...getMore(),} : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

p: null,
>p : null
> : ^^^^

...getMore(),
>getMore() : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>getMore : () => { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

};

const bar = {
>bar : { c: string; p: string; s: string; other: any; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>{ p: null, // no error, this gets overriden other: null, // error, this does not get overriden ...getMore(),} : { c: string; p: string; s: string; other: null; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

p: null, // no error, this gets overriden
>p : null
> : ^^^^

other: null, // error, this does not get overriden
>other : null
> : ^^^^

...getMore(),
>getMore() : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>getMore : () => { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

};

function doSthWithParams(params: unknown) {
>doSthWithParams : (params: unknown) => { c: string; p: string; s: string; }
> : ^ ^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>params : unknown
> : ^^^^^^^

return {
>{ c: 'foo', p: 'bar', s: 'baz', } : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

c: 'foo',
>c : string
> : ^^^^^^
>'foo' : "foo"
> : ^^^^^

p: 'bar',
>p : string
> : ^^^^^^
>'bar' : "bar"
> : ^^^^^

s: 'baz',
>s : string
> : ^^^^^^
>'baz' : "baz"
> : ^^^^^

};
}

const baz = {
>baz : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>{ p: null, s: null, ...doSthWithParams({ p: 'hello', s: 'world', }),} : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

p: null,
>p : null
> : ^^^^

s: null,
>s : null
> : ^^^^

...doSthWithParams({
>doSthWithParams({ p: 'hello', s: 'world', }) : { c: string; p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>doSthWithParams : (params: unknown) => { c: string; p: string; s: string; }
> : ^ ^^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>{ p: 'hello', s: 'world', } : { p: string; s: string; }
> : ^^^^^^^^^^^^^^^^^^^^^^^^^

p: 'hello',
>p : string
> : ^^^^^^
>'hello' : "hello"
> : ^^^^^^^

s: 'world',
>s : string
> : ^^^^^^
>'world' : "world"
> : ^^^^^^^

}),
};

This file was deleted.

41 changes: 41 additions & 0 deletions tests/cases/compiler/noImplicitAnySpreads1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// @strictNullChecks: false
// @noImplicitAny: true
Comment on lines +1 to +2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// @noEmit: true

// https://github.com/microsoft/TypeScript/issues/58150#issuecomment-2052517378

function getMore() {
return {
c: "foo",
p: "bar",
s: "baz",
};
}

const foo = {
p: null,
...getMore(),
};

const bar = {
p: null, // no error, this gets overriden
other: null, // error, this does not get overriden
...getMore(),
};

function doSthWithParams(params: unknown) {
return {
c: 'foo',
p: 'bar',
s: 'baz',
};
}

const baz = {
p: null,
s: null,
...doSthWithParams({
p: 'hello',
s: 'world',
}),
};