Parsing from proto should keep field ID. (fixes #7645)

[enum-class]

Fix issue #7645. When parsing from a proto that have explicit field identifiers, flatbuffers need to keep those identifiers stable by explicitly using the id attribute. This is because protos allow new field placement anywhere in their message, even before other fields, as long as they use a new field ID.

[enum-class]

Under construction ...

[enum-class]

@dbaileychess could you please help me with review this ?

[enum-class]

Hello @dbaileychess , I found in parser for union add a second auto-generated vector field to hold the types, with a special suffix. The generated field will get id equal to union id minus 1 : Link I wonder if this is a correct behavior for parser or not ? Is this documented ? Actually, It violates consecutive invariant id rules. because it force fbs writer to reserve an id before union id . You can find In monster tests that we do not use id 7, because union wants to use it, And if we use this id, It wont generate code

[dbaileychess]

Hello @dbaileychess , I found in parser for union add a second auto-generated vector field to hold the types, with a special suffix. The generated field will get id equal to union id minus 1 : Link I wonder if this is a correct behavior for parser or not ? Is this documented ? Actually, It violates consecutive invariant id rules. because it force fbs writer to reserve an id before union id . You can find In monster tests that we do not use id 7, because union wants to use it, And if we use this id, It wont generate code

See https://le-michael.github.io/flatbuffers-doc-demo/Schemas.html#attributes on id. We do mention that Unions have two fields associated with them, the Union value N and an implicit hidden Union-Type N-1.

[enum-class]

Hello @dbaileychess

1. Can you help me with buildifier check
2. What should I do about --conform ? What is final decision about it ?
3. It seems ready to review. Could you please review it ?

[dbaileychess]

@enum-class Let me get back to you on this.

[dbaileychess]

For conform, lets just output a message (warning level) that when you use --proto, that you should check for conformity yourself, using the existing --conform implementation.

The builtkite isn't a build cleaner, but another CI platform for bazel. The error looks temporary to me, so lets just try retriggering it.

[dbaileychess]

Oops, you meant the buildifer within builtkite. That can be fixed automatically with the following command:

buildifier tests/BUILD.bazel

[dbaileychess]

Looks like you need to alphabetize the entries, if you don't want to download buildifer

[enum-class]

Thank you for your time, Great review, I love it

[enum-class]

@dbaileychess Could please help me with review this p.r.

[enum-class]

@dbaileychess This tiny p.r. from last year needs review :)

[dbaileychess]

@enum-class Sorry for missing this, I will get to it today or tomorrow.

[dbaileychess]

Going to approve this now. Since it is a new feature, we can fix forward any issues.