Computed property union lifting #21070

sandersn · 2018-01-08T18:09:38Z

Fixes #13948
Fixes #16789
Fixes #20725

Computed properties of object literals with a union type distribute across the type to create a union type. In the following example,

let ab: 'a' | 'b'
let o = { [ab]: 'hi' }

o has the type { a: string } | { b: string }.

Previously, computed properties caused the object literal type to have a index signature instead. This now happens only if the type of the computed property is not a literal type or union of literal types.

This PR also improves two other aspects of computed properties:

Destructuring: Destructuring with a literal-typed computed property (or union-literal-typed) now correctly uses the type of the index signature for the resulting variable:

declare let one: 1
declare let onetwo: 1 | 2
declare let o: { [n: number]: boolean }
declare let o1: { '1': boolean }
var { [one]: d1 } = o1
var { [one]: d1s } = o
var { [onetwo]: d12 } = o

d1, d1s and d12 have the type boolean now, instead of any.

Printing: Computed properties with literal types or unions of literal types now print the string representation of the literal type instead of the source expression. This avoids duplicates from lifted unions, but makes the type more understandable even for other computed properties:

declare let a: 'x'
declare let ab: 'a' | 'b'
let o1 = { [a]: 'hi' }
let o2 = { [ab]: 'hi' }

Currently the quickinfo for o1 is { ["x"]: string }. Previously it was { [a]: string }. Similarly, the quickinfo for o2 is { ["a"]: string } | { ["b"]: string }. Without improved printing it is { [ab]: string } | { [ab]: string }.

Still uses getSpreadType (with modifications) for combining multiple unioned computed properties. This likely has problems that will be exposed by more tests after I write them.

Also improve some names

sandersn · 2018-01-08T18:10:29Z

@DanielRosenwasser you sounded interested in this too, although I don't know if you have time for a review right now.

weswigham · 2018-01-08T18:46:50Z

src/compiler/checker.ts

+                        if (!computedType) {
+                            return anyType;
+                        }
+                        else if (computedType.flags & TypeFlags.Literal) {


Both of the following two branches should just be the union and non-union cases under an isLiteralType(computedType) check, no? It already handles unions/enums correctly. Unless you're explicitly excluding literal type unions which include null/undefined (which, IMO, should be handled with getNonNullableType, if that's the case?)

I didn't think about null/undefined, but it seems like they should be handled the same as other literals. isLiteralType does this correctly.

weswigham · 2018-01-08T19:05:31Z

src/compiler/checker.ts

-                        hasComputedStringProperty = false;
-                        hasComputedNumberProperty = false;
-                        typeFlags = 0;
+                        updateIntermediateType(getSpreadType(intermediate, createObjectLiteralType(), node.symbol, propagatedFlags, /*fromComputedProperty*/ false));


This location no longer does

hasComputedStringProperty = false; hasComputedNumberProperty = false; typeFlags = 0;

Is that intentional?

Yes, this was a mistake because it reset these properties between spreads. I thought one of the baselines improved as a result, but I don't see it now. It was something like { [s1]: 1, ...o, [s2]: 2, ...o }

computedPropertyNames5_ES5/ES6

weswigham · 2018-01-08T19:15:22Z

src/compiler/checker.ts

+                    else if (computedType.flags & TypeFlags.Union && (computedType as UnionType).types.every(t => !!(t.flags & TypeFlags.Literal))) {
+                        type = isTypeAny(objectLiteralType)
+                            ? objectLiteralType
+                            : (computedType as UnionType).types.every(t => !!(t.flags & TypeFlags.NumberLiteral)) && getIndexTypeOfType(objectLiteralType, IndexKind.Number) ||


This code looks suspiciously similar to the above code in getTypeForBindingElement maybe a helper for getTypeOfComputedDestructuredProperty (akin to getTypeOfDestructuredProperty) or something is in order?

Good idea. The resulting function issues errors, so I called it checkComputedDestructuredProperty

weswigham · 2018-01-08T19:20:29Z

src/compiler/checker.ts

+                }
+                else {
+                    const text = getTextOfPropertyName(name);
+                    type = isTypeAny(objectLiteralType)


I know you just moved this from below, but I think this could probably be

type = isTypeAny(objectLiteralType) ? objectLiteralType : getTypeOfDestructuredProperty(objectLiteralType, name); if (type === unknownType) { type = undefined; } // Not sure if this is necessary, but is the only difference in behavior between what was here and `getTypeOfDestructuredProperty`

During checking, I'd rather check type flags than parse+serialise the name like isNumericLiteralName does.

weswigham · 2018-01-08T19:26:49Z

tests/baselines/reference/computedPropertyUnionLiftsToUnionType.errors.txt

@@ -0,0 +1,62 @@
+tests/cases/conformance/es6/computedProperties/computedPropertyUnionLiftsToUnionType.ts(32,4): error TS2459: Type '{ a: string; } | { b: string; }' has no property '[ab]' and no string index signature.


has no property '[ab]' and no string index signature - this error seems somewhat lacking. I'd expect either has no property 'a' and no string index signature or has no property 'b' and no string index signature, given the other changes you've made.

I improved the error reporting and put it behind the noImplicitAny flag where it belongs.

weswigham · 2018-01-08T19:30:44Z

tests/baselines/reference/exportDefaultParenthesize.types

@@ -144,7 +144,7 @@ export default {

 === tests/cases/compiler/comma.ts ===
 export default {
->{    ['foo']: 42} : { ['foo']: number; }
+>{    ['foo']: 42} : { ["foo"]: number; }


I see this is no longer preserving quotemarks. Does this affect declaration emit? For example, in

const ab: 'a' | 'b' = 'a'; export obj = { [ab]: "quas" };

what is the declaration emit? Is it export obj: { ['a']: string } | { ['b']: string }?
Do we use single quotes on 'a' and 'b', like the code does when the type is made, or no? I know it's best-effort, so I can understand if we don't.

Actually, it seems to be using a completely different code path, because the output is { a: string } | { b: string }. I guess this is OK, but I didn't expect it. See the latest commits for the declaration baseline.

weswigham · 2018-01-08T20:23:18Z

src/compiler/checker.ts

+                        !((symbol as TransientSymbol).checkFlags & CheckFlags.Late) &&
+                        !isWellKnownSymbolSyntactically((name as ComputedPropertyName).expression) &&
+                        symbol.escapedName) {
+                        return '["' + unescapeLeadingUnderscores(symbol.escapedName) + '"]';


Previously this would print [0] if the computed property originally was a numeric literal. I know this is more correct with how we view the type (since keys are always strings), but it does feel unfortunate to be always quoting everything in the result. Maybe not quoting numeric names here would make me feel better? It would probably also reduce baseline noise.

True on all points. It's easy enough to change, so I did. But as you noticed below, we aren't really trying to preserve what the user typed with this change.

weswigham · 2018-01-08T20:24:39Z

src/compiler/checker.ts

+                        symbol.flags & SymbolFlags.Transient &&
+                        !((symbol as TransientSymbol).checkFlags & CheckFlags.Late) &&
+                        !isWellKnownSymbolSyntactically((name as ComputedPropertyName).expression) &&
+                        symbol.escapedName) {


Can the escapedName be the empty string? That would also be falsey here.

Yes, { ['']: 1 } is perfectly legal. I added !== undefined here.

mhegazy · 2018-01-09T01:33:15Z

@sandersn this will resolve the issue in #21030, correct?

Jessidhia · 2018-01-09T01:40:35Z

~~IIUC this should also fix #16789~~

EDIT: hmm, almost. That bug uses a non-literal type so it may not be covered 🤔

Also add declaration baselines for computedPropertyUnionLiftsToUnionType

sandersn · 2018-01-09T17:53:45Z

@Kovensky The new destructuring code was almost enough, I just needed to add a couple of cases. With the latest commit, #16789 is also fixed.

sandersn · 2018-01-09T18:52:10Z

@mhegazy No, after in-person discussion we decided that #21030 needs something like the proposed Unionize type.

Let callers rely on closure instead of passing in the source type unchanged to the lambda.

sandersn · 2018-01-09T22:59:11Z

@weswigham I think this is ready for another look.

weswigham

Minor style nit, but otherwise looks good. I'm not the biggest fan of a boolean parameter on getSpreadType, but it'll do.

weswigham · 2018-01-10T00:56:23Z

src/compiler/checker.ts

@@ -4391,22 +4400,28 @@ namespace ts {
                    // Use explicitly specified property name ({ p: xxx } form), or otherwise the implied name ({ p } form)
                    const name = declaration.propertyName || <Identifier>declaration.name;
                    if (isComputedNonLiteralName(name)) {
-                        // computed properties with non-literal names are treated as 'any'
-                        return anyType;
+                        type = checkComputedDestructuredProperty(parentType, name, text => {


I'd write a named function in scope for this lambda and then reuse it below in the else branch.

mhegazy · 2018-01-10T19:17:35Z

The scope of this change is bigger than i previously anticipated. We need to discuss this change in the design meeting.

- Issues with dynamic keys, microsoft/TypeScript#21070 - Issues with React.SFC array return type, microsoft/TypeScript#21699

typescript-bot · 2018-06-14T19:26:38Z

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

IanKemp · 2018-08-31T10:30:25Z

Is this ever going to be implemented?

weswigham · 2018-08-31T15:51:30Z

I may actually be able to integrate fixes for the same issues into #26797. I'll look into it.

sandersn · 2018-08-31T16:11:03Z

It may also be possible to excise a less-ambitious chunk of this PR. It's on my list of bugs to fix for 3.1 or 3.2, but near the bottom. I'll talk with @weswigham to decide on an approach.

sandersn added 7 commits January 3, 2018 11:36

Union literal computed properties lift to union

3cf0bf7

Still uses getSpreadType (with modifications) for combining multiple unioned computed properties. This likely has problems that will be exposed by more tests after I write them.

Test literal union computed property lifting

b39688c

Print known constants in computed property types

d407c92

Also improve some names

Clean up variable naming

3123bef

More computed property cases + Update baselines

6bec753

Improve spread type parameter names

c242f20

Further improve readability of checkObjectLiteral

59095cd

sandersn requested review from weswigham and mhegazy January 8, 2018 18:10

This was referenced Jan 8, 2018

Computed property key names should not be widened #13948

Open

Partial and indexed strings not reporting an error #20201

Closed

weswigham requested changes Jan 8, 2018

View reviewed changes

mhegazy mentioned this pull request Jan 9, 2018

Object literal with variable property is overly generic #21030

Closed

sandersn added 2 commits January 9, 2018 09:21

Print computer numeric literal property names as numeric

25f981f

Improve destructuring:str/num computed properties

c69c4ff

Also add declaration baselines for computedPropertyUnionLiftsToUnionType

sandersn mentioned this pull request Jan 9, 2018

Destructuring property ends up as any when using { [key]:x} = aObject #20725

Closed

sandersn added 4 commits January 9, 2018 13:20

Error reporting:improve destructured computed properties

ddd9714

Computed properties with null and undefined literals

ea8fc7c

Combine destructured computed property checking

772c75b

Simplify parameter type

85e5054

Let callers rely on closure instead of passing in the source type unchanged to the lambda.

sandersn added 2 commits January 9, 2018 15:05

Merge branch 'master' into computed-property-union-lifting

a979a0d

Fix lint:add semicolon

cd3b688

DanielRosenwasser mentioned this pull request Jan 9, 2018

Design Meeting Notes, 8/25/2017 #18155

Closed

weswigham approved these changes Jan 10, 2018

View reviewed changes

DanielRosenwasser mentioned this pull request Feb 20, 2018

Creating object literal with generic key type fails to compile #22060

Closed

ebramanti added a commit to VideoAmp/tslint-config-videoamp that referenced this pull request Mar 24, 2018

fix(react): Remove any requirement

74271a1

- Issues with dynamic keys, microsoft/TypeScript#21070 - Issues with React.SFC array return type, microsoft/TypeScript#21699

typescript-bot closed this Jun 14, 2018

ferdaber mentioned this pull request Jul 10, 2018

[@types/react] cannot setState with dynamic key name type-safe DefinitelyTyped/DefinitelyTyped#26635

Closed

4 tasks

weswigham mentioned this pull request Sep 20, 2018

Regression with >= v3 and React, generics, defaultProps #26395

Closed

jakebailey deleted the computed-property-union-lifting branch November 7, 2022 17:32

Andarist mentioned this pull request Jun 19, 2023

Do not widen computed properties with template literal types #54706

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Computed property union lifting #21070

Computed property union lifting #21070

sandersn commented Jan 8, 2018 •

edited

Loading

sandersn commented Jan 8, 2018

weswigham Jan 8, 2018

sandersn Jan 9, 2018

weswigham Jan 8, 2018

sandersn Jan 9, 2018

sandersn Jan 9, 2018

weswigham Jan 8, 2018

sandersn Jan 9, 2018

weswigham Jan 8, 2018

sandersn Jan 9, 2018

weswigham Jan 8, 2018

sandersn Jan 9, 2018

weswigham Jan 8, 2018

sandersn Jan 9, 2018

weswigham Jan 8, 2018

sandersn Jan 9, 2018

weswigham Jan 8, 2018

sandersn Jan 9, 2018

mhegazy commented Jan 9, 2018

Jessidhia commented Jan 9, 2018 •

edited

Loading

sandersn commented Jan 9, 2018

sandersn commented Jan 9, 2018

sandersn commented Jan 9, 2018

weswigham left a comment

weswigham Jan 10, 2018

mhegazy commented Jan 10, 2018

typescript-bot commented Jun 14, 2018

IanKemp commented Aug 31, 2018

weswigham commented Aug 31, 2018

sandersn commented Aug 31, 2018

		@@ -0,0 +1,62 @@
		tests/cases/conformance/es6/computedProperties/computedPropertyUnionLiftsToUnionType.ts(32,4): error TS2459: Type '{ a: string; } \| { b: string; }' has no property '[ab]' and no string index signature.

Computed property union lifting #21070

Computed property union lifting #21070

Conversation

sandersn commented Jan 8, 2018 • edited Loading

sandersn commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Jan 9, 2018

Jessidhia commented Jan 9, 2018 • edited Loading

sandersn commented Jan 9, 2018

sandersn commented Jan 9, 2018

sandersn commented Jan 9, 2018

weswigham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Jan 10, 2018

typescript-bot commented Jun 14, 2018

IanKemp commented Aug 31, 2018

weswigham commented Aug 31, 2018

sandersn commented Aug 31, 2018

sandersn commented Jan 8, 2018 •

edited

Loading

Jessidhia commented Jan 9, 2018 •

edited

Loading