V2: Avoid the prototype chain with editions if possible #897
Conversation
| @@ -197,7 +199,7 @@ const messagePrototypes = new WeakMap< | |||
| */ | |||
| function createZeroMessage(desc: DescMessage): Message { | |||
| let msg: Record<string, unknown>; | |||
| if (desc.file.edition == EDITION_PROTO3) { | |||
| if (!needsPrototypeChain(desc)) { | |||
There was a problem hiding this comment.
Previously, we only avoided the prototype chain for proto3.
| /** | ||
| * Do we need the prototype chain to track field presence? | ||
| */ | ||
| function needsPrototypeChain(desc: DescMessage): boolean { | ||
| switch (desc.file.edition) { | ||
| case EDITION_PROTO3: | ||
| // proto3 always uses implicit presence, we never need the prototype chain. | ||
| return false; | ||
| case EDITION_PROTO2: | ||
| // proto2 never uses implicit presence, we always need the prototype chain. | ||
| return true; | ||
| default: | ||
| // If a message uses scalar or enum fields with explicit presence, we need | ||
| // the prototype chain to track presence. This rule does not apply to fields | ||
| // in a oneof group - they use a different mechanism to track presence. | ||
| return desc.fields.some( | ||
| (f) => f.presence != IMPLICIT && f.fieldKind != "message" && !f.oneof, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Now we avoid the prototype chain for messages that only use implicit presence. Message fields, oneof, lists, and maps never need the prototype chain.
We're keeping the cases for proto2 and proto3 because this is a hot path.
| describe("from edition2023 with proto3 features", () => { | ||
| const desc = test_messages_proto3_editions_ts.TestAllTypesProto3Schema; | ||
| test("without custom prototype", () => { | ||
| const msg = create(desc); | ||
| const hasCustomPrototype = | ||
| Object.getPrototypeOf(msg) !== Object.prototype; | ||
| expect(hasCustomPrototype).toBe(false); | ||
| }); |
There was a problem hiding this comment.
We're using editions/golden/test_messages_proto3_editions.proto to test that an editions file with file option features.field_presence = IMPLICIT does not use the prototype chain.
timostamm
changed the title
| // the prototype chain to track presence. This rule does not apply to fields | ||
| // in a oneof group - they use a different mechanism to track presence. | ||
| return desc.fields.some( | ||
| (f) => f.presence != IMPLICIT && f.fieldKind != "message" && !f.oneof, |
There was a problem hiding this comment.
What does this do for repeated fields? I'm guessing repeated fields return "IMPLICIT" for presence, right? (Pretty sure that's what happens in other runtimes, like C++, Java, Go.) So if a field had only repeated fields, message fields, and oneof fields, it would return "no need for prototype chain", even when the default field presence for the file/message is explicit, right? If so, might we want to apply this test for "proto2" also, just so that we can further minimize the cases where a prototype is used? On the other hand, will it be surprising to users that use editions that sometimes messages use a prototype and sometimes not?
There was a problem hiding this comment.
Correct, list (repeated) and map fields report presence IMPLICIT.
There was a problem hiding this comment.
I assume this is not a behavior change from when the prototype was used, right? IIUC, the message constructor populates the object with all zero/default vals (except nested messages), so the field values are the same, even when unset and even without the prototype, right?
If so, LGTM. I left one remark about possibly being able to have no prototype chain for proto2 messages, but also with a bit of concern about predictability of when a message has a prototype chain and if that will be a confusion/sore spot for users.
It is not. The only difference is that the object does not have an extra (empty) prototype in its chain. Predictability is a concern, especially if the schema isn't authored by the user of the generated code. For example, if a field is added that requires the prototype chain, code can fail at runtime (if a 3rd party library is involved that requires plain objects). I've considered a custom feature for explicit control, or a plugin option like We'll be getting a better understanding for the DX by updating our examples and writing docs, and can amend if necessary. |
In proto3, serialization skips scalar and enum zero values. This known as "implicit presence". In proto2, and in edition 2023 with default features, fields use explicit presence. In our generated code, messages are plain objects for implicit presence, and use the prototype chain to track explicit presence.
Since there are disadvantages to the prototype chain (some libraries expect only plain objects, without a custom prototype), we want to avoid it if possible.
Currently, converting a proto3 file to editions changes the behavior, adding a prototype to generated messages.
This PR adds a little bit of logic to avoid the prototype chain for editions, if a message uses only implicit presence.