Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace "label" check with "cardinality" check #2927

Merged
merged 8 commits into from May 2, 2024
Merged

Conversation

jhump
Copy link
Member

@jhump jhump commented Apr 29, 2024

The name change is because some of the values of "cardinality" differ from the label field in FieldDescriptorProto. Also, in "editions" some of these values are configured via a field_presence feature, not the field label.

Previously, I mentioned replacing "label" checks with two separate checks: "cardinality" and "field presence". But I decided to keep them together in a single check because it makes the wire and JSON compatibility story easier IMO.

So the values of "cardinality" are as follows:

  1. optional with implicit presence: This applies to fields in proto3 files other than proto3-optional, oneof, extension, and message fields. (Those exceptions all have explicit presence.) In editions, this applies to fields that are configured with "implicit" field presence. If "implicit" is configured as the default for the file, the above exceptions still apply.
  2. optional with explicit presence: This applies to all optional fields in a proto2 file and is the default for non-repeated fields in editions. In proto3 files, this is the cardinality for fields with a message type, with an explicit optional keyword (aka proto3-optional), fields in oneofs, and extension fields. In editions, if a file-level default of "implicit" field presence is configured, it can be overridden on a per-field-level with "explicit" field presence.
  3. required: This applies to required fields in proto2 files and fields in editions files that are configured with "legacy_required" field presence.
  4. repeated: This applies to all fields that have an explicit repeated label.
  5. map: This applies to all map fields. This is slightly different from "repeated" cardinality since it ignores/drops values with duplicate keys.

For wire format compatibility, repeated and map are compatible as are the two kinds of optional fields. But for JSON format, repeated and map are not compatible since one uses JSON arrays and the other JSON objects/maps.

This also updates the oneof-related checks to ignore synthetic oneofs. Synthetic oneofs are a side effect of proto3-optional fields. Changes to them will now be covered by the cardinality check, which can now distinguish between optional fields with implicit vs explicit field presence (proto3-optional fields provide explicit presence in proto3).

Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved given we keep the FIELD_SAME_LABEL test (copy/paste)

@jhump
Copy link
Member Author

jhump commented May 1, 2024

Approved given we keep the FIELD_SAME_LABEL test (copy/paste)

@bufdev, I added it back.

And then I was considering what you said a little more about how it's fine to maybe report multiple errors for the same change... So I added two more commits.

What those last two commits do is to make the deprecated FIELD_SAME_CARDINALITY rule map to all three of the new checks: the file/package-level check, the JSON-level check, and the wire-level check (instead of only the file/package version). I think in most situations that this is exactly what we want. It does a better job of checks w/out false positives. It also does exactly what you'd expect if someone had a configuration to ignore FIELD_SAME_CARDINALITY (admittedly unlikely). The only downside I can think of, in the unlikely event that someone explicitly configured this rule to be used (instead of it being used implicitly via FILE or PACKAGE categories), is that it can report errors in triplicate since its use effectively enables all three versions of the check. (See updated test expectation for an example.)

So the question is: is this better for "v1" and backwards compatibility? I think it is. Without those last two commits, the behavior for "v1" is that it could start complaining about changes when using WIRE or WIRE_JSON categories that aren't really incompatible -- things that it did not complain about before because of the subtle differences between FIELD_SAME_LABEL and its replacement FIELD_SAME_CARDINALITY. But with those commits, the behavior is better -- except in the case where someone configures FIELD_SAME_LABEL to be used directly, in which case they may get errors in triplicate.

What do you think is better? Should I back those last two commits out?

@jhump jhump merged commit 271f421 into dev May 2, 2024
8 checks passed
@jhump jhump deleted the jh/new-buf-breaking-check branch May 2, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants