enum indexer property not inferred properly

[villesau]

try flow

This prevents writing certain libdefs, e.g this: try flow

This has never worked. It's especially important when moving exact by default types since A & B would not work in that kind of cases anymore. Now the example is possible to make working with limitation that exact properties are not supported, and using A & B approach.

Would be very interesting to try to fix this by my self, but Flow codebase is a bit frightening. Any pointers what lines should I look at and would this be big task to fix?

[wchargin]

This seems like a special case of indexers being totally broken on exact
objects. Changing key: "b" to key: string in your example raises the
same error.

The simplest repro is

({a: 1}: {|[string]: number|});  // fails! :-(

@jbrown215: When objects become exact-by-default, this will be a
crippling pain point. Indexer properties will become basically unusable.
Do we have plans to fix this?

[jbrown215]

@wchargin: I've brought this up internally. We have no current plans for this, but I'll look into it more before we make the change to exact by default.

[wchargin]

Great; thanks.

[villesau]

Oh, didn't remember the comment from @wchargin made a new report about (apparently) the same issue: #7240 agreed on "this will be a crippling pain point." <- you always need to fallback into inexact types.

[villesau]

Looks like these lines are the culprit: https://github.com/facebook/flow/blob/master/src/typing/flow_js.ml#L6748-L6763

When taking that off, most of the stuff works fine and this specific problem is fixed, although some tests starts to fail.

[villesau]

@jbrown215 made a PR to fix this: #7271

Looks like it does not fix the second case in this: try flow but that's most likely some other issue related to spreading with indexers.

In overall I think that singleton string indexer properties should be treated as a regular keys from the get go but that's probably bigger change.

[jbrown215]

@villesau: I'm curious to know what purpose an "exact object with an indexer" serves. At first glance, an indexer in an exact object looks like it makes the object equivalent to an inexact object with the same properties/indexer.

Perhaps we should disallow indexers with exact objects altogether?

[villesau]

@jbrown215

First of all, consistency. You should be able to type everything as exact, otherwise it's not intuitive. Secondly, I think that {[key: string]: number} as inexact type does not actually make much sense. What ever key you have should be number, but the object also might have some other properties? There is clear conflict in the logic: the type needs to be something, but it also might be something else that you don't know of. But AFAIK the inexact indexer object does not work that way. So in essence the inexact indexer object actually already is exact. I think that it's just nice coincidence if inexact and exact indexer object types actually behaves identically.

See this example:

// @flow
type A = {|a: number|};
type B = {|...A, [key: 'b']: string |};

const test: B = {a: 321, b: 'abc'}; 

where exact types are spread. I want to keep them as exact. The key b might come from other types, this example is just simplified.

And lastly, like said, it would be at least prework to get this example working. When moving away from inexact types, we need to spread exact types with indexer properties, {a: nuber} & {b: string} would not work with exact types. that example is a simplified version of what would in future become Rosie libdef: https://github.com/rosiejs/rosie but not allowing exact indexers makes such libdef impossible with exact objects.

I opened it up a bit also in other issue that I did, which I ironically didn't notice to be the same issue that I already reported earlier: #7240

[jbrown215]

I think we view exact objects a little differently. Take this example:

type O = {| a: string, [string]: string |}

declare var x: O;
(x.a: string); // Seems reasonable
(x.b: string); // ?? Seems less reasonable

IMO, the exact object should fully capture the properties in the object. An indexer by nature is "imprecise" and feels weird when combined with a precise representation of an object.

First of all, consistency. You should be able to type everything as exact, otherwise it's not intuitive.

I think this friction is necessary to let people know that what they think is exact doesn't actually behave like an exact object should (as far I as view exact objects, which is a place where we disagree).

Secondly, I think that {[key: string]: number} as inexact type does not actually make much sense. What ever key you have should be number, but the object also might have some other properties?

I get why this seems weird from that perspective, but consider it from the other perspective. Why should you be allowed to specify an imprecise specification for an exact object?

When moving away from inexact types, we need to spread exact types with indexer properties

I think the solution there might be to keep the objects inexact and fix spreads. I'm not even sure what the correct behavior for an exact object with an indexer being spread should be. Couldn't it overwrite all of the object's props if the object with the indexer was spread after other props were specified?

[villesau]

Thanks for the profound arguments!

Note that indexer might have well defined string literal as indexer: ({str: 'str' }: {|['str']: string|}); this should behave 1:1 like ({str: 'str' }: {|str: string|}); but the former one fails currently. The PR I submitted has test cases validating that this case works as expected, at least in the simple and most important use cases.

This is necessary when building libdefs. That's the main issue here, and those are essentially the ones we need to spread too.

I think we view exact objects a little differently. Take this example:

type O = {| a: string, [string]: string |}

declare var x: O;
(x.a: string); // Seems reasonable
(x.b: string); // ?? Seems less reasonable

To me the main issue in this example is that indexer properties should always be optional types: #6980

I get why this seems weird from that perspective, but consider it from the other perspective. Why should you be allowed to specify an imprecise specification for an exact object?

I think it as precise: It precisely defines that the key can be anything, but values of any key is well defined. look at this inexact example:
({str: 'str', num: 123 }: {str: string}); try flow

as you see it has property num which is number, completely fine. Now if we consider inexact indexer objects and previous example, this should be fine too:
({str: 'str', num: 123 }: {[key: string]: string}); try flow

but it's not. So I'd argue that inexact indexer objects are broken here and they actually behave pretty much exactly how exact indexer objects should behave. That said, the strength of inexact indexer object should be very close to plain Object type, maybe even exactly the same.

I think the solution there might be to keep the objects inexact and fix spreads. I'm not even sure what the correct behavior for an exact object with an indexer being spread should be. Couldn't it overwrite all of the object's props if the object with the indexer was spread after other props were specified?

If the key is string literal, it should work like normal exact object. Otherwise I'd expect something like:

type O1 = {|a: number|};
type O2 = {|[key: string]: string, ...O1|}; //becomes {|[key: string]: string, a: number|}

type O3 = {|[key: 'a']: string|};
type O4 = {|[key: string]: string, ...O3|}; // becomes {|[key: string]: string, a: number|}

type O5 = {|[key: string]: string|};
type O6 = {|a: string, ...O5|}; // becomes {|a: string, [key: string]: string|}

where precisely defined keys would overrule indexers. I didn't validate that this would be the best behaviour, but this is what I'd expect at first glance and what would be the least surprising to me.

Anyhow, there are three different cases here: Indexer object with arbitrary key (string type), indexer with string union ('key1' | 'key2') (which I didn't want to touch, that's a topic of it's own) and indexer object with well defined key (some string literal).

[jbrown215]

I think both of our interpretations are reasonable, but we disagree in our starting points. I'll talk to Sam, who has plans for our object types, to see what a good mental model for exact vs. inexact is going forward

[villesau]

Here is very related issue: #3162

[wchargin]

As of Flow 0.111.0, Flow is specifically asking us to make indexed
properties exact:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/plugins/git/gitUtils.js:41:11

Cannot determine a type for object literal [1]. object type [2] is inexact, so
it may contain TZ with a type that conflicts with TZ's definition in object
literal [1]. Can you make object type [2] exact?

 [2] 17│   return function git(args: string[], env?: {[string]: string}): string {
       :
 [1] 25│     const fullEnv = {
     26│       // Standardize output.
     27│       LANG: "C",
     28│       LC_ALL: "C",
     29│       PAGER: "cat",
     30│       TZ: "UTC",
     31│       // Short-circuit editing.
     32│       EDITOR: "true", // (this is `true` the command-line program)
     33│       GIT_MERGE_AUTOEDIT: "no",
     34│       // Ignore global Git settings, for test isolation.
     35│       GIT_CONFIG_NOSYSTEM: "1",
     36│       GIT_ATTR_NOSYSTEM: "1",
     37│       // Bring over the SSH configuration so that loading private repos is possible
     38│       // This post has some useful information on SSH_AUTH_SOCK:
     39│       // http://blog.joncairns.com/2013/12/understanding-ssh-agent-and-ssh-add/
     40│       SSH_AUTH_SOCK: process.env.SSH_AUTH_SOCK,
     41│       ...(env || {}),
     42│     };
     43│     const options = {env: fullEnv};
     44│     return execFileSync(

I don’t understand the team’s position on this—if exact objects with
indexers don’t make sense, why is Flow asking us to introduce them to
resolve newly added type errors?

[jbrown215]

This is a bug in the error message. It recommends making the object type exact because it is inexact, not because it has an indexer. We should only recommend making the object exact if there is no indexer. Will fix!

[villesau]

Just to document the TS functionality related to above conversation about indexers: http://www.typescriptlang.org/play/?ssl=12&ssc=3&pln=9&pc=1#code/C4TwDgpgBAglC8UDeAoK6oG0DkBDbAugFxQB2ArgLYBGEATigL4DcKKoksAjAsmhjhjYoAHwAF2aoRIUa9KAHoFZAPbAoAdxV0A1gEtSAcyat24aDABMvVBix5pZKrToAafuhxTiTuW5MeUGacAEI2gZg6ECAkAM7AdAaGPrIuAcHQITyItgJRMVDY8YlGjqnySqrqWrpJ6RyZ1jkR+STYQqIS3jLOFcqkapra+kYBdhlQIQDM4XaR0XEJSSm9-nOtvi4rfu4sbABuuHRQuFwkcDknJFyWM3uHx7iW5zZQ1Ne3UPdHJ1Mvl7gPlNXG8gV9TA8TpYzrAmsgrlAbndWJCns9Ya93ojPt9HrdznCkAikSCsUjwWwUJDqDCwgCwbi3ui6fCyTiUT9qH9Jq9AdjgaD+RTqQAWEgsol8kmCooJYSM6gAVnFvKBIPR5L2VM50PFM3pQoVlmZ+tZDI5x2o+MmpslasFmotTLFNtV-NJbWK8qdVuVroN0o17KAA

[jedwards1211]

I think a string union indexer is the case to focus on, because it comes up in enums, and {[UnionType]: Foo}/{|[UnionType]: Foo|} could be interpreted very differently.

type Unit = 'ft' | 'm'
const attributes: {[Unit]: {toMeters: number, displayText: string}} = {
  ft: {inMeters: 0.3048, displayText: 'Feet'},
  m: {inMeters: 1, displayText: 'Meters'},
}

And @villesau the corresponding TS type is actually Record, not indexer properties.

type UnitAttributes = Record<'ft' | 'm', {toMeters: number, displayText: string}>

[SamChou19815]

Now you can do it with mapped type: https://flow.org/try/#1N4Igxg9gdgZglgcxALlAJwKYEMwBcD6aArlLnALYYrgA2WAzvXGCADQgYAeOBARgJ74AJhhhYiNXClzEM7DFCLl602e0hQhcMtCw18ufgAcqyGUTkhyWIyaEHjp85cMn8CIljRCVZ2QF91CBFqAHpQgAIAARgaCAB3AB0oZJEwOkwI9IZ6CIAxHgg0fgAeABVWCIBBMDAlCSxcIoiAXgjgAB8O-wA+duSIiKwhISrcGRKAaWQIgBJJjH56cp7KgDUZsoBtSYBdHoAKAcGIgGtFmcnWY8GANz0LGbXjgEoZgrwi0or2gDp-mp1cgNJpoSr-X7ALb4SYROBQCJ7J7+CK9ADcxwQGFwACUMPQJLgDm9qrV6nRQcl-Mlkhp6LgImJPsVWhEoBh4vlCsUSp0sDNFOReBgwRFeDN6Wh4QhuocXhiUlA6QymaD+AAmVmqr6-YajcZoA4Acl4RsqRvo5FwAAsoAgAIRG+W06D0xnc-gAZi1Ht1IzGMmNWDNEU96oAjM7FQdtcV1b8sbj8YTiTNOuKIpLpZV+WylMK0N15RFwhESJByJRSIzmiK0EUVOxbiKmNBqLcAAy-CPxjsgfxAA