From 2d86ffec7ea9b97d6a341347261e64c795fb955f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 8 Sep 2026 03:09:05 +0200 Subject: [PATCH] [api] Drop the unused base decode(), group the vtable asserts, and test the decode emitters A generated decodable message that lost its decode() would now fail to compile instead of silently keeping its defaults. The asserts cover exactly the classes that derive from ProtoDecodableMessage, under one ifdef per run instead of one per line, and the generator tests pin the inline decode() wrapper, its absence on fixed vector messages, and the this-> free static body. --- esphome/components/api/api_pb2.cpp | 45 ------------------- esphome/components/api/proto.h | 2 - script/api_protobuf/api_protobuf.py | 35 ++++++++++++--- .../api/test_api_protobuf_generator.py | 36 ++++++++++++++- 4 files changed, 64 insertions(+), 54 deletions(-) diff --git a/esphome/components/api/api_pb2.cpp b/esphome/components/api/api_pb2.cpp index 916aa2ba7e..5457d084ce 100644 --- a/esphome/components/api/api_pb2.cpp +++ b/esphome/components/api/api_pb2.cpp @@ -4191,9 +4191,6 @@ uint32_t BluetoothSetConnectionParamsResponse::calc_size_msg(const void *self) { #ifndef HAS_PROTO_MESSAGE_DUMP static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); #ifdef USE_COVER static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); #endif @@ -4221,8 +4218,6 @@ static_assert(!std::is_polymorphic_v, "decodable messages carry static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); #ifdef USE_API_USER_DEFINED_ACTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_API_USER_DEFINED_ACTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); #endif #ifdef USE_CAMERA @@ -4257,23 +4252,11 @@ static_assert(!std::is_polymorphic_v, #endif #ifdef USE_BLUETOOTH_PROXY_CONNECTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_BLUETOOTH_PROXY_CONNECTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_BLUETOOTH_PROXY_CONNECTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_BLUETOOTH_PROXY_CONNECTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_BLUETOOTH_PROXY_CONNECTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_BLUETOOTH_PROXY_CONNECTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_BLUETOOTH_PROXY_CONNECTIONS static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); #endif #ifdef USE_BLUETOOTH_PROXY @@ -4281,32 +4264,14 @@ static_assert(!std::is_polymorphic_v, "decodable #endif #ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_VOICE_ASSISTANT static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); #endif #ifdef USE_ALARM_CONTROL_PANEL @@ -4332,8 +4297,6 @@ static_assert(!std::is_polymorphic_v, "decodable messages #endif #ifdef USE_ZWAVE_PROXY static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_ZWAVE_PROXY static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); #endif #if defined(USE_IR_RF) || defined(USE_RADIO_FREQUENCY) @@ -4341,17 +4304,9 @@ static_assert(!std::is_polymorphic_v, "deco #endif #ifdef USE_SERIAL_PROXY static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_SERIAL_PROXY static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_SERIAL_PROXY static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_SERIAL_PROXY static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); -#endif -#ifdef USE_SERIAL_PROXY static_assert(!std::is_polymorphic_v, "decodable messages carry no vtable"); #endif #ifdef USE_BLUETOOTH_PROXY_CONNECTIONS diff --git a/esphome/components/api/proto.h b/esphome/components/api/proto.h index e48364885d..3ec3e6f436 100644 --- a/esphome/components/api/proto.h +++ b/esphome/components/api/proto.h @@ -728,8 +728,6 @@ class ProtoDecodableMessage : public ProtoMessage { /// Walk \p buffer and hand every field to \p field. The generated decode() passes the message's /// own decode_field, so decodable messages carry no vtable. static void decode_fields(void *msg, const uint8_t *buffer, size_t length, DecodeFieldFn field); - /// A decodable message without fields has nothing to read - void decode(const uint8_t *buffer, size_t length) {} /** * Count occurrences of a repeated field in a protobuf buffer. diff --git a/script/api_protobuf/api_protobuf.py b/script/api_protobuf/api_protobuf.py index 6067832ea6..b1e687ede8 100755 --- a/script/api_protobuf/api_protobuf.py +++ b/script/api_protobuf/api_protobuf.py @@ -2536,8 +2536,8 @@ def build_message_type( # Get source direction to determine if we need decode/encode methods source = message_source_map[desc.name] - needs_decode = source in (SOURCE_BOTH, SOURCE_CLIENT) - needs_encode = source in (SOURCE_BOTH, SOURCE_SERVER) + needs_decode = message_needs_decode(source) + needs_encode = message_needs_encode(source) # Add MESSAGE_TYPE method if this is a service message if message_id is not None: @@ -2846,6 +2846,23 @@ def get_field_opt( return field.options.Extensions[opt] +def message_needs_decode(source: int) -> bool: + return source in (SOURCE_BOTH, SOURCE_CLIENT) + + +def message_needs_encode(source: int) -> bool: + return source in (SOURCE_BOTH, SOURCE_SERVER) + + +def is_decodable_class(desc: descriptor.DescriptorProto, source: int) -> bool: + """Whether the generated class derives from ProtoDecodableMessage: decoded, and either on a + decodable base class or with at least one live field.""" + return message_needs_decode(source) and ( + get_base_class(desc) is not None + or any(not field.options.deprecated for field in desc.field) + ) + + def get_base_class(desc: descriptor.DescriptorProto) -> str | None: """Get the base_class option from a message descriptor.""" if not desc.options.HasExtension(pb.base_class): @@ -3402,7 +3419,7 @@ static void dump_bytes_field(DumpBuffer &out, const char *field_name, const uint continue s, c, dc = build_message_type(m, base_class_fields, message_source_map) - if message_source_map[m.name] in (SOURCE_BOTH, SOURCE_CLIENT): + if is_decodable_class(m, message_source_map[m.name]): decodable_messages.append((m.name, message_ifdef_map.get(m.name))) msg_ifdef = message_ifdef_map.get(m.name) @@ -3433,9 +3450,17 @@ static void dump_bytes_field(DumpBuffer &out, const char *field_name, const uint # decode() passes decode_field explicitly, so without the dump virtuals no decodable message # may carry a vtable; a build at any level below VERY_VERBOSE proves it cpp += "#ifndef HAS_PROTO_MESSAGE_DUMP\n" + assert_ifdef = None for name, msg_ifdef in decodable_messages: - line = f'static_assert(!std::is_polymorphic_v<{name}>, "decodable messages carry no vtable");' - cpp += "\n".join(wrap_with_ifdef(line, msg_ifdef)) + "\n" + if msg_ifdef != assert_ifdef: + if assert_ifdef is not None: + cpp += "#endif\n" + if msg_ifdef is not None: + cpp += _make_ifdef_line(msg_ifdef) + "\n" + assert_ifdef = msg_ifdef + cpp += f'static_assert(!std::is_polymorphic_v<{name}>, "decodable messages carry no vtable");\n' + if assert_ifdef is not None: + cpp += "#endif\n" cpp += "#endif\n" content += """\ diff --git a/tests/unit_tests/components/api/test_api_protobuf_generator.py b/tests/unit_tests/components/api/test_api_protobuf_generator.py index 094305dfe3..9b05e9bc2c 100644 --- a/tests/unit_tests/components/api/test_api_protobuf_generator.py +++ b/tests/unit_tests/components/api/test_api_protobuf_generator.py @@ -285,8 +285,9 @@ def test_a_fixed64_field_fails_at_generation_time() -> None: build_message_type(desc, {}, {"Wide": SOURCE_CLIENT}) -def test_message_gets_a_single_decode_field_override() -> None: - """All wire types of a decoded message land in one decode_field() switch.""" +def test_message_decodes_through_one_static_decode_field() -> None: + """All wire types of a decoded message land in one static decode_field() switch that the + inline decode() hands to the shared loop.""" desc = descriptor_pb2.DescriptorProto(name="Mixed") desc.field.add(name="name", number=1, type=STRING) desc.field.add(name="count", number=2, type=UINT32) @@ -308,3 +309,34 @@ def test_message_gets_a_single_decode_field_override() -> None: (3, "WIRE_TYPE_FIXED32"), ): assert f"case proto_tag({number}, {wire_type}):" in cpp, cpp + # The static body works on the cast message, never on this + assert "auto &msg = *static_cast(self);" in cpp + assert "this->" not in cpp + assert ( + header.count( + "ProtoDecodableMessage::decode_fields(this, buffer, length, &decode_field);" + ) + == 1 + ) + + +def test_fixed_vector_message_keeps_its_own_decode() -> None: + """A message that sizes a FixedVector first decodes through its own decode(), not the inline one.""" + desc = descriptor_pb2.DescriptorProto(name="Sized") + field = desc.field.add(name="values", number=1, type=UINT32) + field.label = descriptor_pb2.FieldDescriptorProto.LABEL_REPEATED + field.options.Extensions[pb.fixed_vector] = True + header, cpp, _ = build_message_type(desc, {}, {"Sized": SOURCE_CLIENT}) + assert ( + "ProtoDecodableMessage::decode_fields(this, buffer, length, &decode_field);" + not in header + ) + assert header.count("void decode(const uint8_t *buffer, size_t length);") == 1 + assert "void Sized::decode(const uint8_t *buffer, size_t length) {" in cpp + assert "ProtoDecodableMessage::count_repeated_field(buffer, length, 1)" in cpp + assert ( + cpp.count( + "ProtoDecodableMessage::decode_fields(this, buffer, length, &decode_field);" + ) + == 1 + )