Fix codegen output for object with indexer #35344

ZihanChen-MSFT · 2022-11-14T22:00:12Z

Summary

The current implementation think {[key:T]:U} and {key:object} are the same type, which is semantically wrong.

This pull request fixes the problem and return {type:'GenericObjectTypeAnnotation'}, so that {[key:T]:U} is Object. The current schema cannot represent dictionary type, Object is the closest one.

The previous incorrect implementation actually bring in code with logic that doesn't make sense (treating indexer as property), those and related unit test are all undone.

Changelog

[General] [Changed] - Fix codegen output for object with indexer

Test Plan

yarn jest react-native-codegen passed

analysis-bot · 2022-11-14T22:23:10Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,776,630	-57
android	hermes	armeabi-v7a	7,096,414	-57
android	hermes	x86	8,251,531	-5
android	hermes	x86_64	8,108,901	-47
android	jsc	arm64-v8a	8,969,654	+0
android	jsc	armeabi-v7a	7,702,739	+0
android	jsc	x86	9,033,808	+0
android	jsc	x86_64	9,510,746	+0

Base commit: 0f089ea
Branch: main

christophpurrer · 2022-11-14T22:29:15Z

I fear this will conflict with #35098 and won't pass our internal builds as we rely on parsing indexed properties for (std::)map data structures in C++ TM.

I agree that mixing properties and indexed types is not ideal - but it was a cheap initial solution.

If we want to 'Fix codegen output for object with indexer' I would expect that this change:

splits properties and indexes apart
correctly parses them and exports them to the output

I don't think the current PR can be landed as it - as it simply removes existing logic.

For the current sample we would then output something as:

                  'typeAnnotation': {
                    'type': 'IndexedObjectTypeAnnotation',
                    'properties': [
                      {
                        'name': 'key',
                        'typeAnnotation': {
                          'type': 'GenericObjectTypeAnnotation'
                        }
                      }
                    ]

analysis-bot · 2022-11-14T22:58:08Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: a3c47d3
Branch: main

pull-bot · 2022-11-14T23:09:29Z

PR build artifact for 32bc4f8 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

cipolleschi

Thank you for taking the time to solve this semantic issue.

I left a few comments to improve the code maintainability, let me know what do you think.

Functionality-wise, it looks good to me.

packages/react-native-codegen/src/parsers/flow/modules/index.js

packages/react-native-codegen/src/parsers/flow/parser.js

packages/react-native-codegen/src/parsers/parsers-commons.js

packages/react-native-codegen/src/parsers/typescript/modules/index.js

facebook-github-bot · 2022-11-15T13:37:55Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

packages/react-native-codegen/src/parsers/flow/parser.js

ZihanChen-MSFT · 2022-11-15T20:14:09Z

as we rely on parsing indexed properties for (std::)map data structures in C++ TM.

Hi @christophpurrer . As I mentioned in the description, the current behavior parses {[key:T]:U} exactly like {key:object}. I think this is a incorrect behavior, and you can't tell the difference between them, I don't really understand how will it be translated to a map instead of a struct with 'key'.

I think making a IndexedObjectTypeAnnotation would be good, but unfortunately @cipolleschi said it has been rejected by another internal group of people. If I could choose, I would add a new type, it is better.

But @cipolleschi I think here is exactly a useful use case. I am totally fine if we could add a new type here. Otherwise, the only correct way will be treating it like an object, and code using std::map should just be reverted, because it relies on a bug.

christophpurrer · 2022-11-15T20:20:54Z

as we rely on parsing indexed properties for (std::)map data structures in C++ TM.

Hi @christophpurrer . As I mentioned in the description, the current behavior parses {[key:T]:U} exactly like {key:object}. I think this is a incorrect behavior, and you can't tell the difference between them, I don't really understand how will it be translated to a map instead of a struct with 'key'.

But @cipolleschi I think here is exactly a useful use case. I am totally fine if we could add a new type here. Otherwise, the only correct way will be treating it like an object, and code using std::map should just be reverted, because it relies on a bug.

Totally agree we shouldn't treat index and property the same.
Ideally it would be great to still parse and emit the indexes to the parsed output - we already have the logic, we just need to stored them under indexes instead of properties > If not doing this here, do you want to send out a follow up PR for that work?
If we know the key type and value type of of indexed objects it makes it safer down the road when we convert them to std:map representation

@ZihanChen-MSFT > What is your main motivation for this PR? Are you fixing an actual issue / bug on your side OR you 'just' want to make things semantically correct?

ZihanChen-MSFT · 2022-11-15T20:43:07Z

What is your main motivation for this PR?

Hi @christophpurrer

in Microsoft/react-native-windows we have our backend too, the C++ codegen there will print the most detailed types we could find to improve type safety.

By having the codegen treat {[key:T]:U} like {key:U}, it will generate a struct {JsonObject key;}; for a dictionary type, and make real fields missing in native world, it is a blocker for us. We need a way to tell the differences between them, but since adding IndexedObjectTypeAnnotation is not approved (as in offline conversation to @cipolleschi ), so the only correct way is to treat it like Object, which will be translated to a JSON object in native world, and we could read all values by looking into the JSON object, just like a dictionary.

My original idea was to add a IndexedObjectTypeAnnotation. I would be happy to do that.

pull-bot · 2022-11-15T21:42:23Z

PR build artifact for 8a559d6 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

facebook-github-bot · 2022-11-16T13:01:14Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi

I applied the comments @christophpurrer added internally.

I think that they are valid, so I forwarded them here.

Thank you for taking care of these PRs.

packages/react-native-codegen/src/parsers/parser.js

cipolleschi · 2022-11-17T13:37:31Z

packages/react-native-codegen/src/parsers/typescript/modules/index.js

+        ).length > 0
+      ) {
+        // no need to do further checking
+        return emitObject(nullable);


from @christophpurrer:

What if we emit a new type here?
Instead of GenericObjectTypeAnnotation we can use IndexedObjectTypeAnnotation

I think this is the idea of DictionaryObjectTypeAnnotation but you said you don't want me to add that type am I right?

I don't have a strong opinion of adding or not adding a new type for indexed objects at this point.

If we would add a new type we should keep its name as close as possible to the internal representation in flow-parser (ObjectTypeIndexer) or babel-parser with the TypeScript plugin (TSIndexSignature)

Dictionary is not a term used in either of the 2 parsers.

packages/react-native-codegen/src/parsers/flow/modules/index.js

packages/react-native-codegen/src/parsers/parsers-commons.js

pull-bot · 2022-11-17T18:09:28Z

PR build artifact for eb28c31 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

cipolleschi · 2022-11-25T12:10:54Z

/rebase

github-actions · 2022-11-29T12:42:57Z

	Warnings
⚠️	packages/react-native-codegen/src/parsers/tests/parsers-commons-test.js#L31 - packages/react-native-codegen/src/parsers/tests/parsers-commons-test.js line 31 – 'typeScriptParser' is assigned a value but never used. (no-unused-vars)

Generated by 🚫 dangerJS against 49c8252

pull-bot · 2022-11-29T13:48:29Z

PR build artifact for aba3568 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

cipolleschi

I left a couple of comments for some unused return values.
I'll import it to see if something breaks

cipolleschi · 2022-11-29T14:49:34Z

packages/react-native-codegen/src/parsers/typescript/modules/index.js

+        if (indexSignatures.length > 0) {
+          // check the property type to prevent developers from using unsupported types
+          const propertyType = indexSignatures[0].typeAnnotation;
+          translateTypeAnnotation(


We are not using the returned value of translateTypeAnnotation. Is it right?

The return value is ignored, since we don't really have DictionaryTypeAnnotation, there is no place to return it. But I call this function to check if the type is supported.

packages/react-native-codegen/src/parsers/flow/modules/index.js

facebook-github-bot · 2022-11-29T14:53:40Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cipolleschi · 2022-11-29T16:25:27Z

Unfortunately, things are breaking internally.

We have a bunch of
UnsupportedTypeAnnotationParserError: Module NativeFeliosSceneModule: Flow type annotation 'MixedTypeAnnotation' is unsupported in NativeModule specs
I think we should add some logging on which file is being parsed in case of errors 🤔
I cannot understand which module is actually emitting the error because is only see the name of the file that aggregates all the modules specs...

This is the most likely cause of the failure. In a TM we have this export

export type MyType = {[name: string]: (args: mixed) => mixed};

So I think that the Flow mixed is not allowed.

ZihanChen-MSFT · 2022-11-29T21:54:00Z

@cipolleschi https://flow.org/en/docs/types/mixed/ Looks like mixed, 'any' and Object all give no information about types but:

Object: Flow knows it is an Object (so it could not be undefined, but it could be null though).
'any': Flow would treat it as any type if needed.
'mixed': Flow assume it could be anything, but the type is unknown, no automatic conversion is performed.

So I could generate mixed as Object. mixed is supported in C++ but it reports MixedTypeAnnotation, I don't know what it means, but I will generate GenericObjectTypeAnnotation in other cases for backward compatibility. If you think we should always use MixedTypeAnnotation, I'm fine.

    case 'MixedTypeAnnotation': {
      if (cxxOnly) {
        return emitMixedTypeAnnotation(nullable);
      } else {
        return emitObject(nullable);
      }
    }

pull-bot · 2022-11-29T22:34:52Z

PR build artifact for 099766b is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

facebook-github-bot · 2022-11-30T09:46:59Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pull-bot · 2022-12-01T23:51:13Z

PR build artifact for 56f69b8 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-12-12T22:56:26Z

PR build artifact for 49c8252 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

facebook-github-bot · 2022-12-13T09:45:51Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-12-13T11:24:00Z

@cipolleschi merged this pull request in f07490b.

Summary: The current implementation think `{[key:T]:U}` and `{key:object}` are the same type, which is semantically wrong. This pull request fixes the problem and return `{type:'GenericObjectTypeAnnotation'}`, so that `{[key:T]:U}` is `Object`. The current schema cannot represent dictionary type, `Object` is the closest one. The previous incorrect implementation actually bring in code with logic that doesn't make sense (treating indexer as property), those and related unit test are all undone. ## Changelog [General] [Changed] - Fix codegen output for object with indexer Pull Request resolved: facebook#35344 Test Plan: `yarn jest react-native-codegen` passed Reviewed By: rshest Differential Revision: D41304475 Pulled By: cipolleschi fbshipit-source-id: caab8e458d83f9850c5c28b67cc561a764738372

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner labels Nov 14, 2022

cipolleschi requested changes Nov 15, 2022

View reviewed changes

christophpurrer reviewed Nov 15, 2022

View reviewed changes

packages/react-native-codegen/src/parsers/flow/parser.js Outdated Show resolved Hide resolved

ZihanChen-MSFT requested review from cipolleschi and christophpurrer and removed request for cipolleschi and christophpurrer November 15, 2022 20:18

cipolleschi requested changes Nov 17, 2022

View reviewed changes

ZihanChen-MSFT requested a review from cipolleschi November 17, 2022 17:57

ZihanChen-MSFT mentioned this pull request Nov 28, 2022

☂️ Improving Codegen #34872

Closed

ZihanChen-MSFT force-pushed the ts-rncodegen15 branch from eb28c31 to aba3568 Compare November 29, 2022 12:41

cipolleschi reviewed Nov 29, 2022

View reviewed changes

cipolleschi approved these changes Nov 30, 2022

View reviewed changes

ZihanChen-MSFT force-pushed the ts-rncodegen15 branch from 099766b to 56f69b8 Compare December 1, 2022 22:41

ZihanChen-MSFT added 13 commits December 12, 2022 13:42

[TS] Fix dictionary type schema output

a6a7432

[TS] Delete unnecessary test cases

baffa9f

[Flow] Fix dictionary type schema output

f55ee35

[Flow] Delete unnecessary test cases

11e39c2

yarn prettier

d76e478

yarn flow-check-ios

d2f6506

Fix code review comments

2d36d51

...

1ebab2d

Fix code review comments

dd46fe7

Update module-parser-snapshot-test.js.snap

f0fc95e

Check property type of indexed object

5f11adc

yarn prettier

a319914

Fix code review comments

49c8252

ZihanChen-MSFT force-pushed the ts-rncodegen15 branch from 56f69b8 to 49c8252 Compare December 12, 2022 21:44

facebook-github-bot closed this in f07490b Dec 13, 2022

facebook-github-bot added the Merged This PR has been merged. label Dec 13, 2022

ZihanChen-MSFT deleted the ts-rncodegen15 branch December 13, 2022 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix codegen output for object with indexer #35344

Fix codegen output for object with indexer #35344

ZihanChen-MSFT commented Nov 14, 2022

analysis-bot commented Nov 14, 2022 •

edited

Loading

christophpurrer commented Nov 14, 2022

analysis-bot commented Nov 14, 2022 •

edited

Loading

pull-bot commented Nov 14, 2022

cipolleschi left a comment •

edited

Loading

facebook-github-bot commented Nov 15, 2022

ZihanChen-MSFT commented Nov 15, 2022 •

edited

Loading

christophpurrer commented Nov 15, 2022

ZihanChen-MSFT commented Nov 15, 2022 •

edited

Loading

pull-bot commented Nov 15, 2022

facebook-github-bot commented Nov 16, 2022

cipolleschi left a comment

cipolleschi Nov 17, 2022

ZihanChen-MSFT Nov 17, 2022

christophpurrer Nov 17, 2022

pull-bot commented Nov 17, 2022

cipolleschi commented Nov 25, 2022

github-actions bot commented Nov 29, 2022 •

edited

Loading

pull-bot commented Nov 29, 2022

cipolleschi left a comment

cipolleschi Nov 29, 2022

ZihanChen-MSFT Nov 29, 2022

facebook-github-bot commented Nov 29, 2022

cipolleschi commented Nov 29, 2022 •

edited

Loading

ZihanChen-MSFT commented Nov 29, 2022 •

edited

Loading

pull-bot commented Nov 29, 2022

facebook-github-bot commented Nov 30, 2022

pull-bot commented Dec 1, 2022

pull-bot commented Dec 12, 2022

facebook-github-bot commented Dec 13, 2022

facebook-github-bot commented Dec 13, 2022

Fix codegen output for object with indexer #35344

Fix codegen output for object with indexer #35344

Conversation

ZihanChen-MSFT commented Nov 14, 2022

Summary

Changelog

Test Plan

analysis-bot commented Nov 14, 2022 • edited Loading

christophpurrer commented Nov 14, 2022

analysis-bot commented Nov 14, 2022 • edited Loading

pull-bot commented Nov 14, 2022

cipolleschi left a comment • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Nov 15, 2022

ZihanChen-MSFT commented Nov 15, 2022 • edited Loading

christophpurrer commented Nov 15, 2022

ZihanChen-MSFT commented Nov 15, 2022 • edited Loading

pull-bot commented Nov 15, 2022

facebook-github-bot commented Nov 16, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi Nov 17, 2022

Choose a reason for hiding this comment

ZihanChen-MSFT Nov 17, 2022

Choose a reason for hiding this comment

christophpurrer Nov 17, 2022

Choose a reason for hiding this comment

pull-bot commented Nov 17, 2022

cipolleschi commented Nov 25, 2022

github-actions bot commented Nov 29, 2022 • edited Loading

pull-bot commented Nov 29, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi Nov 29, 2022

Choose a reason for hiding this comment

ZihanChen-MSFT Nov 29, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Nov 29, 2022

cipolleschi commented Nov 29, 2022 • edited Loading

ZihanChen-MSFT commented Nov 29, 2022 • edited Loading

pull-bot commented Nov 29, 2022

facebook-github-bot commented Nov 30, 2022

pull-bot commented Dec 1, 2022

pull-bot commented Dec 12, 2022

facebook-github-bot commented Dec 13, 2022

facebook-github-bot commented Dec 13, 2022

analysis-bot commented Nov 14, 2022 •

edited

Loading

analysis-bot commented Nov 14, 2022 •

edited

Loading

cipolleschi left a comment •

edited

Loading

ZihanChen-MSFT commented Nov 15, 2022 •

edited

Loading

ZihanChen-MSFT commented Nov 15, 2022 •

edited

Loading

github-actions bot commented Nov 29, 2022 •

edited

Loading

cipolleschi commented Nov 29, 2022 •

edited

Loading

ZihanChen-MSFT commented Nov 29, 2022 •

edited

Loading