From bdc4a54613d77d8ededc180070369f5135f17be4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 25 Mar 2026 15:10:10 -1000 Subject: [PATCH] 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 f65c45b2ea..c9a476622e 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 b4a5621e3e..fe4db23ae3 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 c150d2b3a8..6f3c422693 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 6e1abcef55..28eae4e8f0 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 e33c3ad84e..37779f23e6 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}"