mirror of
https://github.com/esphome/esphome.git
synced 2026-09-11 23:37:34 +00:00
[core] Review cleanup: docstring accuracy, rationale comments, unsafe+no_split test
- Fix the ComponentMarker docstring's incomplete 'either placement-news or mutates a global' claim — acknowledge that function-local patterns also exist and note the bare-local detection covers them. - Document that _emits_bare_local's RawExpression detection is intentionally safety-biased: false negatives break compilation, false positives just keep a slightly larger IIFE. Note the CallExpression(..., RawExpression) negative case explicitly. - Explain the mutable-list-flag pattern in cpp_main_section — dataclass would read cleaner but the pattern is localized. - Add regression test for a group with BOTH IIFEUnsafeStatement and a bare-local: unsafe wins (flat emission) because a return inside any IIFE, even a single big one, only exits the lambda.
This commit is contained in:
@@ -544,7 +544,14 @@ def _emits_bare_local(exp: "Statement") -> bool:
|
||||
declare a function-local whose lifetime extends past the current
|
||||
statement. Components that emit any such statement must not be
|
||||
sub-split — later references within the same ``to_code`` would land
|
||||
in a different IIFE and fail to compile."""
|
||||
in a different IIFE and fail to compile.
|
||||
|
||||
The detection is intentionally safety-biased: false negatives cause
|
||||
silent broken C++, false positives just keep a component in one
|
||||
slightly larger IIFE. Any ``cg.add(RawExpression(...))`` disables
|
||||
sub-splitting for its group regardless of whether the raw text
|
||||
actually references a local, because the chunker can't introspect
|
||||
arbitrary raw text."""
|
||||
from esphome.cpp_generator import (
|
||||
AssignmentExpression,
|
||||
ExpressionStatement,
|
||||
@@ -561,7 +568,11 @@ def _emits_bare_local(exp: "Statement") -> bool:
|
||||
# `time::ParsedTimezone tz{}` or `tz.field = ...`. CORE.add wraps
|
||||
# a passed Expression in an ExpressionStatement; when the inner is
|
||||
# a RawExpression the author is emitting uninterpreted text that
|
||||
# may reference a local declared elsewhere in the same block.
|
||||
# may reference a local declared elsewhere in the same block. A
|
||||
# RawExpression passed as a CallExpression argument does NOT land
|
||||
# here (its ExpressionStatement's .expression is the CallExpression),
|
||||
# so value-pass patterns like `var.set_program(RawExpression("&foo"))`
|
||||
# continue to sub-split normally.
|
||||
if isinstance(exp, ExpressionStatement) and isinstance(
|
||||
exp.expression, RawExpression
|
||||
):
|
||||
@@ -1113,6 +1124,12 @@ class EsphomeCore:
|
||||
# sub-split so the declaration and any later references stay
|
||||
# together.
|
||||
prefix: list[str] = []
|
||||
# Flags are stored as 1-element lists so they can be mutated by
|
||||
# reference after the tuple has been pushed into ``components``
|
||||
# (assignment to a local ``unsafe_flag`` inside the loop would
|
||||
# rebind the local without updating the stored tuple entry).
|
||||
# A small dataclass would read cleaner but the pattern is
|
||||
# localized to this one property.
|
||||
components: list[tuple[list[str], list[bool], list[bool]]] = []
|
||||
current: list[str] = prefix
|
||||
unsafe_flag: list[bool] = [False] # unused for prefix
|
||||
|
||||
@@ -460,8 +460,12 @@ class ComponentMarker(Statement):
|
||||
|
||||
Grouping is best-effort: ``flush_tasks`` can interleave coroutines
|
||||
on ``await``, so a component's later statements may land in another
|
||||
component's chunk. Safe because every statement either placement-
|
||||
news into static storage or mutates a file-scope global."""
|
||||
component's chunk. This is safe for the dominant codegen patterns
|
||||
(placement-new into static storage, assignment to a file-scope
|
||||
global); patterns that depend on function-local state within the
|
||||
IIFE scope (cg.variable, with_local_variable, raw bare locals)
|
||||
are kept together by the bare-local detection in cpp_main_section
|
||||
so they aren't split across sibling lambdas."""
|
||||
|
||||
__slots__ = ("name",)
|
||||
|
||||
|
||||
@@ -1107,6 +1107,24 @@ def test_cpp_main_section_typed_assignment_disables_sub_split() -> None:
|
||||
assert out.count("[]() {") == 1
|
||||
|
||||
|
||||
def test_cpp_main_section_iife_unsafe_wins_over_no_split() -> None:
|
||||
# A group that triggers BOTH flags (IIFEUnsafeStatement present AND
|
||||
# a bare-local emission) must still be emitted flat — the unsafe
|
||||
# flag wins because a `return` inside any IIFE, even a single big
|
||||
# one, only exits the lambda.
|
||||
target = core.EsphomeCore()
|
||||
target.main_statements = [
|
||||
ComponentMarker("safe_mode_with_local"),
|
||||
RawStatement("{"), # would trigger no_split
|
||||
RawStatement("new_foo();"),
|
||||
RawStatement("}"),
|
||||
IIFEUnsafeStatement(RawExpression("if (cond) return")),
|
||||
]
|
||||
out = target.cpp_main_section
|
||||
assert "[]() {" not in out
|
||||
assert "if (cond) return;" in out
|
||||
|
||||
|
||||
def test_cpp_main_section_iife_unsafe_statement_emits_component_flat() -> None:
|
||||
# A component that emits IIFEUnsafeStatement (e.g. safe_mode with
|
||||
# `if (...) return;`) must be emitted flat — a `return` inside an
|
||||
|
||||
Reference in New Issue
Block a user