Skip to content

Commit 65cf198

Browse files
jamesarichclaude
andcommitted
feat: reject hand-set deprecated in the field_metadata annotation
The `deprecated` attribute is generator-managed — mirrored from the field's standard `[deprecated = true]` option. A hand-set value inside (meshtastic.field_metadata) previously produced silently divergent output: the Go and Kotlin/Wire generators emitted it while the Swift plugin ignored it (breaking byte-identity), and the schema carried two sources of truth for the same fact. Make it a hard error at generation time in all three generators, with the same message naming the field and the fix. Verified: both protoc plugins reject a hand-set fixture identically; parity over the real schema remains byte-identical; Go tests cover true/false/redundant hand-set cases. Also document the Go plugin's `target=swift` assumption that files set `option swift_prefix = ""` (all meshtastic protos do) — without it, swift-protobuf prefixes generated type names and the emitted extensions would reference nonexistent types; the parity CI catches that divergence. The bundled field_metadata.pb.swift regen picks up the proto comment change (comments only, no functional difference). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent da4b15c commit 65cf198

8 files changed

Lines changed: 106 additions & 23 deletions

File tree

meshtastic/field_metadata.proto

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,10 @@ message FieldMetadata {
6969
* Field is deprecated. MIRRORS the field's standard `[deprecated = true]`
7070
* option — the generators populate this automatically from that option so
7171
* every consumer can read it at runtime (protobuf runtimes strip options, so
72-
* the standard `deprecated` bit is otherwise invisible to apps). Do NOT set
73-
* this by hand in a (meshtastic.field_metadata) annotation; mark the field
74-
* `[deprecated = true]` as usual and it flows through here.
72+
* the standard `deprecated` bit is otherwise invisible to apps). Setting it
73+
* by hand in a (meshtastic.field_metadata) annotation is a generation-time
74+
* ERROR; mark the field `[deprecated = true]` as usual and it flows through
75+
* here.
7576
*/
7677
optional bool deprecated = 6;
7778
}

packages/kmp/buildSrc/src/main/kotlin/org/meshtastic/proto/build/FieldMetadataRegistryHandler.kt

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,21 @@ class FieldMetadataRegistryHandler : SchemaHandler() {
8080
val relative = if (packageName != null) fqn.removePrefix("$packageName.") else fqn
8181
val typePath = relative.split(".")
8282
for (field in type.fieldsAndOneOfFields) {
83+
val raw = field.options.get(optionMember)
84+
// `deprecated` is generator-managed (mirrored from the standard option); a
85+
// hand-set value in the annotation is a hard error, matching the other
86+
// generators, so they can't disagree about it.
87+
val handSet = (raw as? Map<*, *>)?.keys?.any { key ->
88+
((key as? ProtoMember)?.simpleName ?: key.toString()) == DEPRECATED_ATTR
89+
} == true
90+
check(!handSet) {
91+
"$fqn.${field.name}: the \"$DEPRECATED_ATTR\" attribute is generator-managed and cannot be " +
92+
"set in (meshtastic.field_metadata); mark the field `[deprecated = true]` instead and " +
93+
"it is mirrored automatically"
94+
}
8395
// A field earns an entry if it carries the custom annotation OR the standard
8496
// `deprecated` option, which we mirror into the registry (see renderConstructor).
85-
val ctor = renderConstructor(field.options.get(optionMember), field.isDeprecated, metaFieldTypes)
97+
val ctor = renderConstructor(raw, field.isDeprecated, metaFieldTypes)
8698
?: continue
8799
out += Entry(fqn, typePath, field.name, field.tag, ctor)
88100
}
@@ -96,9 +108,9 @@ class FieldMetadataRegistryHandler : SchemaHandler() {
96108
* Renders the `FieldMetadata(...)` constructor call for one field, or null if the field has no
97109
* metadata at all. [raw] is the decoded `(meshtastic.field_metadata)` option (may be null);
98110
* [isDeprecated] is the field's standard `deprecated` option, mirrored in as the `deprecated`
99-
* attribute so apps can read deprecation at runtime. Args are keyed by attribute name (sorted,
100-
* deduped) so the standard `deprecated` option is authoritative and ordering matches the other
101-
* generators.
111+
* attribute so apps can read deprecation at runtime (a hand-set `deprecated` in the annotation
112+
* is rejected in [collect]). Args are keyed by attribute name and sorted so ordering matches
113+
* the other generators.
102114
*/
103115
private fun renderConstructor(
104116
raw: Any?,

tools/protoc-gen-fieldmeta-swift/README.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,9 @@ Swift consumer doesn't need Go on contributor machines or CI.
2929
`[deprecated = true]` option (read off `field.options.deprecated`), not from
3030
the custom annotation — so fields already marked deprecated surface as
3131
`FieldMetadata(deprecated: true)` and apps can read deprecation at runtime.
32-
Entry/attribute ordering matches the Go plugin (entries by proto type then
33-
tag; accessors by type path then field name), keeping the output
32+
Hand-setting `deprecated` inside the annotation is a hard error, same as the
33+
Go plugin. Entry/attribute ordering matches the Go plugin (entries by proto
34+
type then tag; accessors by type path then field name), keeping the output
3435
byte-identical even across many message types.
3536

3637
## Usage (Apple / swift-protobuf)

tools/protoc-gen-fieldmeta-swift/Sources/protoc-gen-fieldmeta-swift/Generator.swift

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,14 @@ struct FieldMetaSwiftGenerator: CodeGenerator {
7777
// A field earns an entry if it carries the custom annotation OR
7878
// the standard `deprecated` option (which we mirror below).
7979
for field in message.fields where field.options.hasFieldMetadata || field.options.deprecated {
80+
// `deprecated` is generator-managed (mirrored from the standard
81+
// option); a hand-set value in the annotation is a hard error,
82+
// matching the Go plugin, so the generators can't disagree.
83+
if field.options.fieldMetadata.hasDeprecated {
84+
throw GenError.message(
85+
"\(message.fullName).\(field.name): the \"deprecated\" attribute is generator-managed and cannot be set in (meshtastic.field_metadata); mark the field `[deprecated = true]` instead and it is mirrored automatically"
86+
)
87+
}
8088
entries.append(Entry(
8189
swiftTypePath: namer.fullName(message: message),
8290
protoTypeName: message.fullName,

tools/protoc-gen-fieldmeta-swift/Sources/protoc-gen-fieldmeta-swift/meshtastic/field_metadata.pb.swift

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,10 @@ public struct FieldMetadata: Sendable {
112112
/// Field is deprecated. MIRRORS the field's standard `[deprecated = true]`
113113
/// option — the generators populate this automatically from that option so
114114
/// every consumer can read it at runtime (protobuf runtimes strip options, so
115-
/// the standard `deprecated` bit is otherwise invisible to apps). Do NOT set
116-
/// this by hand in a (meshtastic.field_metadata) annotation; mark the field
117-
/// `[deprecated = true]` as usual and it flows through here.
115+
/// the standard `deprecated` bit is otherwise invisible to apps). Setting it
116+
/// by hand in a (meshtastic.field_metadata) annotation is a generation-time
117+
/// ERROR; mark the field `[deprecated = true]` as usual and it flows through
118+
/// here.
118119
public var deprecated: Bool {
119120
get {_deprecated ?? false}
120121
set {_deprecated = newValue}

tools/protoc-gen-fieldmeta/README.md

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,15 @@ separate module, so there's no collision). Rust could instead use an inherent
5050
`impl` on the prost types once the Rust generation pipeline is wired into this
5151
repo; the standalone module is used until then.
5252

53+
> **`target=swift` assumes `option swift_prefix = "";`** (which every
54+
> `meshtastic/*.proto` sets). The emitted `extension Config.PositionConfig`
55+
> blocks use raw proto type paths; for a file *without* that option,
56+
> swift-protobuf prefixes generated type names with the package name
57+
> (`Meshtastic_Config`), so this plugin's extensions would reference types that
58+
> don't exist. The pure-Swift sibling plugin derives names from swift-protobuf's
59+
> own namer and is immune — the divergence is caught by the parity check in CI
60+
> (`.github/workflows/field-metadata.yml`).
61+
5362
The plugin reads the option **dynamically** (no generated Go bindings) and is
5463
generic over the contents of the `FieldMetadata` message: adding a scalar
5564
attribute to `field_metadata.proto` requires no change here. The `FieldMetadata`
@@ -69,10 +78,12 @@ If a module doesn't define the extension (e.g. buf generates the option-free
6978
and apps therefore can't read at runtime. So any field already marked
7079
`[deprecated = true]` shows up in the registry (`deprecated: true`) with no
7180
`(meshtastic.field_metadata)` annotation, and a field carrying both a custom
72-
attribute and `[deprecated = true]` gets both. The standard option is
73-
authoritative. This is the one attribute that costs a generator change (the
74-
sibling Kotlin/Wire and pure-Swift generators mirror it identically); every
75-
other attribute remains a schema-only addition.
81+
attribute and `[deprecated = true]` gets both. Setting `deprecated` by hand
82+
inside the annotation is a **hard error at generation time** (in all three
83+
generators) — it would create a second source of truth for the same fact. This
84+
is the one attribute that costs a generator change (the sibling Kotlin/Wire and
85+
pure-Swift generators mirror it identically); every other attribute remains a
86+
schema-only addition.
7687

7788
## Build & test
7889

tools/protoc-gen-fieldmeta/generate_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,38 @@ func TestGenerateRejectsBadTarget(t *testing.T) {
230230
}
231231
}
232232

233+
// TestGenerateRejectsHandSetDeprecated verifies that setting the
234+
// generator-managed `deprecated` attribute by hand inside the custom annotation
235+
// is a hard error — regardless of its value or of the standard option — instead
236+
// of silently producing output the other generators would disagree with.
237+
func TestGenerateRejectsHandSetDeprecated(t *testing.T) {
238+
for name, fieldDef := range map[string]string{
239+
"true, no standard option": `uint32 p = 1 [(meshtastic.field_metadata) = { deprecated: true }];`,
240+
"false, among other attrs": `uint32 p = 1 [(meshtastic.field_metadata) = { deprecated: false, diy_only: true }];`,
241+
"redundant with standard opt": `uint32 p = 1 [deprecated = true, (meshtastic.field_metadata) = { deprecated: true }];`,
242+
} {
243+
t.Run(name, func(t *testing.T) {
244+
req := compileRequest(t, map[string]string{
245+
"meshtastic/field_metadata.proto": fieldMetadataProtoSrc,
246+
"meshtastic/test.proto": `
247+
syntax = "proto3";
248+
package meshtastic;
249+
import "meshtastic/field_metadata.proto";
250+
message M { ` + fieldDef + ` }
251+
`,
252+
}, "python", "meshtastic/test.proto", "meshtastic/field_metadata.proto")
253+
254+
_, err := generate(req)
255+
if err == nil {
256+
t.Fatal("expected error for hand-set deprecated attribute, got nil")
257+
}
258+
if !strings.Contains(err.Error(), "meshtastic.M.p") || !strings.Contains(err.Error(), "generator-managed") {
259+
t.Errorf("error should name the field and the rule, got: %v", err)
260+
}
261+
})
262+
}
263+
}
264+
233265
// TestGenerateRejectsNonScalarAttribute verifies the scalar-only guard: a
234266
// non-scalar FieldMetadata attribute (here a repeated field) must fail the build
235267
// rather than emit meaningless, non-deterministic output.

tools/protoc-gen-fieldmeta/main.go

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -152,13 +152,19 @@ func generate(req *pluginpb.CodeGeneratorRequest) (*pluginpb.CodeGeneratorRespon
152152
}
153153

154154
var entries []entry
155+
var collectErr error
155156
files.RangeFiles(func(fd protoreflect.FileDescriptor) bool {
156157
if len(toGen) > 0 && !toGen[fd.Path()] {
157158
return true
158159
}
159-
collectMessages(fd.Messages(), extType, resolver, &entries)
160+
if collectErr = collectMessages(fd.Messages(), extType, resolver, &entries); collectErr != nil {
161+
return false
162+
}
160163
return true
161164
})
165+
if collectErr != nil {
166+
return nil, collectErr
167+
}
162168

163169
sort.Slice(entries, func(i, j int) bool {
164170
if entries[i].MessageType != entries[j].MessageType {
@@ -235,7 +241,7 @@ func isScalarKind(k protoreflect.Kind) bool {
235241
return isIntKind(k)
236242
}
237243

238-
func collectMessages(msgs protoreflect.MessageDescriptors, extType protoreflect.ExtensionType, resolver *protoregistry.Types, out *[]entry) {
244+
func collectMessages(msgs protoreflect.MessageDescriptors, extType protoreflect.ExtensionType, resolver *protoregistry.Types, out *[]entry) error {
239245
for i := 0; i < msgs.Len(); i++ {
240246
md := msgs.Get(i)
241247
// Package-relative path, e.g. meshtastic.Config.PositionConfig -> [Config, PositionConfig].
@@ -245,10 +251,18 @@ func collectMessages(msgs protoreflect.MessageDescriptors, extType protoreflect.
245251
for j := 0; j < fields.Len(); j++ {
246252
f := fields.Get(j)
247253
mf := readMetadata(f, extType, resolver)
248-
// Mirror the standard `deprecated` field option into the registry.
249-
// The custom annotation isn't expected to set it (see
250-
// field_metadata.proto); the standard option is authoritative, so
251-
// upsert to a single deprecated=true attribute either way.
254+
// `deprecated` is generator-managed: it mirrors the field's standard
255+
// option, and a hand-set value in the annotation is rejected rather
256+
// than merged — the generators would otherwise disagree about it (the
257+
// Swift plugin reads only the standard option), and the schema would
258+
// carry two sources of truth for the same fact.
259+
for _, a := range mf {
260+
if a.Name == deprecatedAttr {
261+
return fmt.Errorf(
262+
"%s.%s: the %q attribute is generator-managed and cannot be set in (meshtastic.field_metadata); mark the field `[deprecated = true]` instead and it is mirrored automatically",
263+
md.FullName(), f.Name(), deprecatedAttr)
264+
}
265+
}
252266
if fieldIsDeprecated(f) {
253267
mf = upsertBool(mf, deprecatedAttr, true)
254268
}
@@ -263,8 +277,11 @@ func collectMessages(msgs protoreflect.MessageDescriptors, extType protoreflect.
263277
Fields: mf,
264278
})
265279
}
266-
collectMessages(md.Messages(), extType, resolver, out) // nested message types
280+
if err := collectMessages(md.Messages(), extType, resolver, out); err != nil { // nested message types
281+
return err
282+
}
267283
}
284+
return nil
268285
}
269286

270287
// readMetadata returns the set attributes of the field_metadata option on a

0 commit comments

Comments
 (0)