From bdc4a54613d77d8ededc180070369f5135f17be4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Mar 2026 15:10:10 -1000 Subject: [PATCH 1/5] Address review: const ref args, revert lock migration - TriggerForwarder::operator() now takes const Ts&... to avoid copies of non-trivial types (e.g., std::string). - Revert lock to trigger-based pattern: LockStateForwarder holds two pointers (automation + lock), exceeding sizeof(void*) which would cause Callback::create to heap-allocate. - Remove forwarder_extra_args from API since lock was the only user. --- esphome/automation.py | 21 +++++---------------- esphome/components/lock/__init__.py | 21 ++++----------------- esphome/components/lock/automation.h | 10 ---------- esphome/core/automation.h | 2 +- tests/unit_tests/test_automation.py | 18 ++++++------------ 5 files changed, 16 insertions(+), 56 deletions(-) diff --git a/esphome/automation.py b/esphome/automation.py index f65c45b2ea0..c9a476622e4 100644 --- a/esphome/automation.py +++ b/esphome/automation.py @@ -672,7 +672,6 @@ async def build_callback_automation( args: TemplateArgsType, config: ConfigType, forwarder: MockObjClass | None = None, - forwarder_extra_args: list | None = None, ) -> None: """Build an Automation and register it as a callback on the parent. @@ -680,7 +679,9 @@ async def build_callback_automation( automation's trigger() directly as a callback on the parent component. Uses template forwarder structs so the compiler deduplicates the operator() - body across all call sites with the same signature. + body across all call sites with the same signature. The forwarder must be + pointer-sized (single Automation* field) to fit inline in Callback::ctx_ + and avoid heap allocation. :param parent: The component object (e.g., button, sensor). :param callback_method: Name of the callback method (e.g., "add_on_press_callback"). @@ -688,11 +689,7 @@ async def build_callback_automation( :param config: The automation config dict. :param forwarder: Optional forwarder type to use instead of the default TriggerForwarder. Pass any struct type whose aggregate init takes - an Automation pointer as the first field (e.g., TriggerOnTrueForwarder, - or a custom component-defined forwarder). - :param forwarder_extra_args: Optional list of extra MockObj args to pass to the - forwarder after the automation pointer in aggregate init. For example, - a lock forwarder needs the lock entity pointer: [lock_var]. + a single Automation pointer (e.g., TriggerOnTrueForwarder). """ arg_types = [arg[0] for arg in args] templ = cg.TemplateArguments(*arg_types) @@ -708,12 +705,4 @@ async def build_callback_automation( if arg_types else TriggerForwarder.template() ) - init_args = str(obj) - if forwarder_extra_args: - extra = ", ".join(str(a) for a in forwarder_extra_args) - init_args = f"{init_args}, {extra}" - cg.add( - getattr(parent, callback_method)( - cg.RawExpression(f"{forwarder}{{{init_args}}}") - ) - ) + cg.add(getattr(parent, callback_method)(cg.RawExpression(f"{forwarder}{{{obj}}}"))) diff --git a/esphome/components/lock/__init__.py b/esphome/components/lock/__init__.py index b4a5621e3eb..fe4db23ae3a 100644 --- a/esphome/components/lock/__init__.py +++ b/esphome/components/lock/__init__.py @@ -35,7 +35,6 @@ LockLockTrigger = lock_ns.class_("LockLockTrigger", automation.Trigger.template( LockUnlockTrigger = lock_ns.class_("LockUnlockTrigger", automation.Trigger.template()) LockState = lock_ns.enum("LockState") -LockStateForwarder = lock_ns.class_("LockStateForwarder") LOCK_STATES = { "LOCKED": LockState.LOCK_STATE_LOCKED, @@ -95,23 +94,11 @@ def lock_schema( @setup_entity("lock") async def _setup_lock_core(var, config): for conf in config.get(CONF_ON_LOCK, []): - await automation.build_callback_automation( - var, - "add_on_state_callback", - [], - conf, - forwarder=LockStateForwarder.template(LockState.LOCK_STATE_LOCKED), - forwarder_extra_args=[var], - ) + trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) + await automation.build_automation(trigger, [], conf) for conf in config.get(CONF_ON_UNLOCK, []): - await automation.build_callback_automation( - var, - "add_on_state_callback", - [], - conf, - forwarder=LockStateForwarder.template(LockState.LOCK_STATE_UNLOCKED), - forwarder_extra_args=[var], - ) + trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) + await automation.build_automation(trigger, [], conf) if mqtt_id := config.get(CONF_MQTT_ID): mqtt_ = cg.new_Pvariable(mqtt_id, var) diff --git a/esphome/components/lock/automation.h b/esphome/components/lock/automation.h index c150d2b3a85..6f3c4226933 100644 --- a/esphome/components/lock/automation.h +++ b/esphome/components/lock/automation.h @@ -66,14 +66,4 @@ template class LockStateTrigger : public Trigger<> { using LockLockTrigger = LockStateTrigger; using LockUnlockTrigger = LockStateTrigger; -/// Forwarder that triggers an Automation<> when a Lock reaches a specific state. -template struct LockStateForwarder { - Automation<> *automation; - Lock *lock; - void operator()() const { - if (this->lock->state == State) - this->automation->trigger(); - } -}; - } // namespace esphome::lock diff --git a/esphome/core/automation.h b/esphome/core/automation.h index 6e1abcef55a..28eae4e8f07 100644 --- a/esphome/core/automation.h +++ b/esphome/core/automation.h @@ -495,7 +495,7 @@ template class Automation { /// One operator() instantiation per Automation signature, shared across all call sites. template struct TriggerForwarder { Automation *automation; - void operator()(Ts... args) const { this->automation->trigger(args...); } + void operator()(const Ts &...args) const { this->automation->trigger(args...); } }; /// Callback forwarder that triggers an Automation<> only when the bool arg is true. diff --git a/tests/unit_tests/test_automation.py b/tests/unit_tests/test_automation.py index e33c3ad84e2..37779f23e68 100644 --- a/tests/unit_tests/test_automation.py +++ b/tests/unit_tests/test_automation.py @@ -187,7 +187,6 @@ def _build_forwarder( automation_name: str, args: list[tuple[str, str]], forwarder: MockObj | None = None, - extra_args: list[str] | None = None, ) -> str: """Build a trigger forwarder expression the same way build_callback_automation does. @@ -202,10 +201,7 @@ def _build_forwarder( cg.TemplateArguments(*arg_types) if arg_types else cg.TemplateArguments() ) forwarder = TriggerForwarder.template(templ) - init_args = str(obj) - if extra_args: - init_args += ", " + ", ".join(extra_args) - return f"{forwarder}{{{init_args}}}" + return f"{forwarder}{{{obj}}}" def test_trigger_forwarder_no_args() -> None: @@ -253,10 +249,8 @@ def test_trigger_forwarder_string_arg() -> None: assert result == "TriggerForwarder{auto_1}" -def test_trigger_forwarder_custom_with_extra_args() -> None: - """Lock on_lock: custom forwarder with extra args for entity pointer.""" - lock_forwarder = MockObj("LockStateForwarder", "") - result = _build_forwarder( - "auto_1", [], forwarder=lock_forwarder, extra_args=["lock_var"] - ) - assert result == "LockStateForwarder{auto_1, lock_var}" +def test_trigger_forwarder_custom_type() -> None: + """Custom forwarder type passed directly.""" + custom = MockObj("MyForwarder", "") + result = _build_forwarder("auto_1", [], forwarder=custom) + assert result == "MyForwarder{auto_1}" From 73ae0dd7398475573673dc5374f5b7322f1b5303 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Mar 2026 15:17:05 -1000 Subject: [PATCH 2/5] Migrate event component (EventTrigger) --- esphome/components/event/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/esphome/components/event/__init__.py b/esphome/components/event/__init__.py index 300902b8cad..82b4cc76e16 100644 --- a/esphome/components/event/__init__.py +++ b/esphome/components/event/__init__.py @@ -92,8 +92,9 @@ def event_schema( @setup_entity("event") async def setup_event_core_(var, config, *, event_types: list[str]): for conf in config.get(CONF_ON_EVENT, []): - trigger = cg.new_Pvariable(conf[CONF_TRIGGER_ID], var) - await automation.build_automation(trigger, [(cg.StringRef, "event_type")], conf) + await automation.build_callback_automation( + var, "add_on_event_callback", [(cg.StringRef, "event_type")], conf + ) cg.add(var.set_event_types(event_types)) From a90a2b1d62401b5cc81f0c722447493b7ca2bc14 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Mar 2026 15:20:36 -1000 Subject: [PATCH 3/5] Address review: simplify template branch, add constructor comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove unnecessary TriggerForwarder.template() branch — TemplateArguments() already handles the empty case. - Add comment on Automation default constructor explaining its purpose for the forwarder pattern. --- esphome/automation.py | 6 +----- esphome/core/automation.h | 1 + 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/esphome/automation.py b/esphome/automation.py index c9a476622e4..0958e862a8d 100644 --- a/esphome/automation.py +++ b/esphome/automation.py @@ -700,9 +700,5 @@ async def build_callback_automation( # one operator() per forwarder type; different automation pointers are just # data in the struct. if forwarder is None: - forwarder = ( - TriggerForwarder.template(templ) - if arg_types - else TriggerForwarder.template() - ) + forwarder = TriggerForwarder.template(templ) cg.add(getattr(parent, callback_method)(cg.RawExpression(f"{forwarder}{{{obj}}}"))) diff --git a/esphome/core/automation.h b/esphome/core/automation.h index 28eae4e8f07..35f9e9bf387 100644 --- a/esphome/core/automation.h +++ b/esphome/core/automation.h @@ -470,6 +470,7 @@ template class ActionList { template class Automation { public: + /// Default constructor for use with TriggerForwarder (no Trigger object needed). Automation() = default; explicit Automation(Trigger *trigger) { trigger->set_automation_parent(this); } From bd69737878834f3918919d5cbad486d841388208 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Mar 2026 15:21:55 -1000 Subject: [PATCH 4/5] Add comment explaining RawExpression usage --- esphome/automation.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/esphome/automation.py b/esphome/automation.py index 0958e862a8d..996181f50b3 100644 --- a/esphome/automation.py +++ b/esphome/automation.py @@ -701,4 +701,7 @@ async def build_callback_automation( # data in the struct. if forwarder is None: forwarder = TriggerForwarder.template(templ) + # RawExpression for aggregate init — both forwarder and obj are codegen + # MockObjs (not user input), and there's no Expression type for positional + # aggregate initialization (StructInitializer uses named fields). cg.add(getattr(parent, callback_method)(cg.RawExpression(f"{forwarder}{{{obj}}}"))) From 150e0775b9fe6ffb042e5796d35e03d8d7283b15 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Mar 2026 15:36:08 -1000 Subject: [PATCH 5/5] Add static_asserts for forwarder size, widen type hint - static_assert that forwarders are pointer-sized and trivially copyable so future field additions can't silently cause heap allocation in Callback::create(). - Widen forwarder parameter type hint to MockObj | MockObjClass. --- esphome/automation.py | 2 +- esphome/core/automation.h | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/esphome/automation.py b/esphome/automation.py index 996181f50b3..7b1d6ceca13 100644 --- a/esphome/automation.py +++ b/esphome/automation.py @@ -671,7 +671,7 @@ async def build_callback_automation( callback_method: str, args: TemplateArgsType, config: ConfigType, - forwarder: MockObjClass | None = None, + forwarder: MockObj | MockObjClass | None = None, ) -> None: """Build an Automation and register it as a callback on the parent. diff --git a/esphome/core/automation.h b/esphome/core/automation.h index 35f9e9bf387..fc2cad99be0 100644 --- a/esphome/core/automation.h +++ b/esphome/core/automation.h @@ -494,12 +494,14 @@ template class Automation { /// Callback forwarder that triggers an Automation directly. /// One operator() instantiation per Automation signature, shared across all call sites. +/// Must stay pointer-sized to fit inline in Callback::ctx_ without heap allocation. template struct TriggerForwarder { Automation *automation; void operator()(const Ts &...args) const { this->automation->trigger(args...); } }; /// Callback forwarder that triggers an Automation<> only when the bool arg is true. +/// Must stay pointer-sized to fit inline in Callback::ctx_ without heap allocation. struct TriggerOnTrueForwarder { Automation<> *automation; void operator()(bool state) const { @@ -509,6 +511,7 @@ struct TriggerOnTrueForwarder { }; /// Callback forwarder that triggers an Automation<> only when the bool arg is false. +/// Must stay pointer-sized to fit inline in Callback::ctx_ without heap allocation. struct TriggerOnFalseForwarder { Automation<> *automation; void operator()(bool state) const { @@ -517,4 +520,13 @@ struct TriggerOnFalseForwarder { } }; +// Ensure forwarders fit in Callback::ctx_ (pointer-sized inline storage). +// If these fail, the forwarder would heap-allocate in Callback::create(). +static_assert(sizeof(TriggerForwarder<>) <= sizeof(void *)); +static_assert(sizeof(TriggerOnTrueForwarder) <= sizeof(void *)); +static_assert(sizeof(TriggerOnFalseForwarder) <= sizeof(void *)); +static_assert(std::is_trivially_copyable_v>); +static_assert(std::is_trivially_copyable_v); +static_assert(std::is_trivially_copyable_v); + } // namespace esphome