[core] Enforce the preferences contracts with concepts (#18191)

esphome/core/defines.h

@@ -157,9 +157,17 @@
 #define USE_OUTPUT_FLOAT_POWER_SCALING
 #define USE_POWER_SUPPLY
 #define USE_PREFERENCES_SYNC_EVERY_LOOP
-// Only defined by key-lookup preference backends (esp32, libretiny, host, zephyr);
+// Only defined by key-lookup preference backends; the slot-based platforms
-// slot-based platforms (esp8266, rp2040) never set it in generated builds
+// (esp8266, rp2040) never set it in generated builds, and their preferences
+// managers do not provide load_from_key(), so the PreferencesKeyLookupContract
+// assert would fail their clang-tidy environments. Written as a deny-list so
+// the no-platform analysis configuration (whose Preferences stub provides
+// load_from_key()) keeps covering the key-lookup code paths, and so a future
+// slot-based platform fails the assert loudly instead of silently losing
+// analysis coverage.
+#if !defined(USE_ESP8266) && !defined(USE_RP2)
 #define USE_PREFERENCE_KEY_LOOKUP
+#endif
 #define USE_PROVISIONING
 #define USE_QR_CODE
 #define USE_SAFE_MODE_BOOT_IS_GOOD_ON_SHUTDOWN

esphome/core/preference_backend.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <concepts>
 #include <cstdint>
 
 #include "esphome/core/defines.h"
@@ -30,6 +31,15 @@
 
 namespace esphome {
 
+// The PreferenceBackend method surface, asserted on the alias each platform
+// header binds. save() persists len bytes; load() fills dest only when the
+// stored data exists and matches len. Both report success as their return.
+template<typename T>
+concept PreferenceBackendContract = requires(T backend, const uint8_t *src, uint8_t *dest, size_t len) {
+  { backend.save(src, len) } -> std::same_as<bool>;
+  { backend.load(dest, len) } -> std::same_as<bool>;
+};
+
 #if !defined(USE_ESP32) && !defined(USE_ESP8266) && !defined(USE_RP2) && !defined(USE_LIBRETINY) && \
     !defined(USE_HOST) && !(defined(USE_ZEPHYR) && defined(CONFIG_SETTINGS))
 // Stub for static analysis when no platform is defined.
@@ -40,6 +50,8 @@ struct PreferenceBackend {
 #endif
 
 using ESPPreferenceBackend = PreferenceBackend;
+static_assert(PreferenceBackendContract<PreferenceBackend>,
+              "The platform's preference backend is missing part of the PreferenceBackend surface");
 
 class ESPPreferenceObject {
  public:
@@ -68,6 +80,39 @@ class ESPPreferenceObject {
   PreferenceBackend *backend_{nullptr};
 };
 
+// The preferences manager method surface, asserted in esphome/core/preferences.h
+// on the ESPPreferences alias each platform's preferences.h binds through
+// DECLARE_PREFERENCE_ALIASES. Semantics beyond the signatures:
+// - make_preference: the two-argument form applies the platform's historic
+//   default storage; in_flash=false may fall back to flash where the platform
+//   has no faster storage.
+// - sync: commit pending writes to flash, true on success.
+// - reset: forget unsaved changes and re-initialize the permanent storage
+//   (usually followed by a restart), true on success.
+// The template forms are what component call sites use; PreferencesMixin
+// supplies them, but the derived class's non-template overloads hide them
+// unless it also declares `using PreferencesMixin<X>::make_preference;`, so
+// the concept pins those too.
+template<typename T>
+concept PreferencesContract = requires(T prefs, size_t len, uint32_t type, bool in_flash) {
+  { prefs.make_preference(len, type, in_flash) } -> std::same_as<ESPPreferenceObject>;
+  { prefs.make_preference(len, type) } -> std::same_as<ESPPreferenceObject>;
+  { prefs.template make_preference<uint32_t>(type, in_flash) } -> std::same_as<ESPPreferenceObject>;
+  { prefs.template make_preference<uint32_t>(type) } -> std::same_as<ESPPreferenceObject>;
+  { prefs.sync() } -> std::same_as<bool>;
+  { prefs.reset() } -> std::same_as<bool>;
+};
+
+// Key-lookup platforms additionally provide load_from_key(), a one-shot read
+// of a stored preference by key that migrate_preference() relies on; see the
+// key-lookup note at the top of this file. Not part of PreferencesContract,
+// so it is asserted in preferences.h only where USE_PREFERENCE_KEY_LOOKUP
+// is set.
+template<typename T>
+concept PreferencesKeyLookupContract = requires(T prefs, uint32_t type, uint8_t *data, size_t len) {
+  { prefs.load_from_key(type, data, len) } -> std::same_as<bool>;
+};
+
 /// CRTP mixin providing type-safe template make_preference<T>() helpers.
 /// Platform preferences classes inherit this to avoid duplicating these templates.
 template<typename Derived> class PreferencesMixin {

esphome/core/preferences.h

@@ -45,8 +45,18 @@ extern ESPPreferences *global_preferences;  // NOLINT(cppcoreguidelines-avoid-no
 }  // namespace esphome
 #endif
 
+namespace esphome {
+static_assert(PreferencesContract<ESPPreferences>,
+              "The platform's preferences manager is missing part of the ESPPreferences surface "
+              "(esphome/core/preference_backend.h)");
+}  // namespace esphome
+
 #ifdef USE_PREFERENCE_KEY_LOOKUP
 namespace esphome {
+static_assert(PreferencesKeyLookupContract<ESPPreferences>,
+              "This platform emits USE_PREFERENCE_KEY_LOOKUP but its preferences manager does not provide "
+              "load_from_key() (esphome/core/preference_backend.h)");
+
 /// Copy preference data stored under old_key into new_pref (created for new_key) if the keys
 /// differ and new_pref has no data yet. scratch must hold at least size bytes.
 /// Returns true when scratch holds the entity's current data (loaded or just migrated).

tests/component_tests/preferences/config/bk72xx.yaml

@@ -0,0 +1,5 @@
+esphome:
+  name: preftest
+
+bk72xx:
+  board: generic-bk7252

tests/component_tests/preferences/config/esp32.yaml

@@ -0,0 +1,5 @@
+esphome:
+  name: preftest
+
+esp32:
+  board: esp32dev

tests/component_tests/preferences/config/esp8266.yaml

@@ -0,0 +1,5 @@
+esphome:
+  name: preftest
+
+esp8266:
+  board: esp01_1m

tests/component_tests/preferences/config/host.yaml

@@ -0,0 +1,4 @@
+esphome:
+  name: preftest
+
+host:

tests/component_tests/preferences/config/nrf52.yaml

@@ -0,0 +1,6 @@
+esphome:
+  name: preftest
+
+nrf52:
+  board: adafruit_itsybitsy_nrf52840
+  bootloader: adafruit_nrf52_sd140_v6

tests/component_tests/preferences/config/rp2.yaml

@@ -0,0 +1,5 @@
+esphome:
+  name: preftest
+
+rp2:
+  board: rpipicow

tests/component_tests/preferences/test_key_lookup_gate.py

@@ -0,0 +1,39 @@
+"""Every preferences platform either emits USE_PREFERENCE_KEY_LOOKUP from
+codegen (key-lookup backends) or must not (slot-based backends, whose managers
+have no load_from_key()). Run each platform's real codegen and assert the
+emission, mirroring the split the deny-list in esphome/core/defines.h assumes
+for static analysis.
+
+The fixtures cover every distinct preferences backend today: ln882x and
+rtl87xx route through libretiny (bk72xx stands in for the family), rp2040 is
+an alias of rp2, and nrf52 exercises zephyr. A seventh backend needs a new
+fixture here."""
+
+from collections.abc import Callable
+from pathlib import Path
+
+import pytest
+
+from esphome.core import CORE
+
+
+@pytest.mark.parametrize(
+    ("fixture", "emits"),
+    [
+        ("esp32.yaml", True),
+        ("bk72xx.yaml", True),  # libretiny
+        ("host.yaml", True),
+        ("nrf52.yaml", True),  # zephyr
+        ("esp8266.yaml", False),
+        ("rp2.yaml", False),
+    ],
+)
+def test_key_lookup_define_matches_the_platform_backend(
+    fixture: str,
+    emits: bool,
+    generate_main: Callable[[str | Path], str],
+    component_config_path: Callable[[str], Path],
+) -> None:
+    generate_main(component_config_path(fixture))
+    defines = {define.name for define in CORE.defines}
+    assert ("USE_PREFERENCE_KEY_LOOKUP" in defines) is emits

tests/components/core/test_preference_contract.cpp

@@ -0,0 +1,91 @@
+// Pins the preferences contract concepts so the surface they enforce cannot
+// drift unnoticed: a minimal conforming type must satisfy each concept, and a
+// type missing a method or returning the wrong type must not.
+
+#include <gtest/gtest.h>
+
+#include "esphome/core/preference_backend.h"
+
+namespace esphome::core::testing {
+
+struct MinimalBackend {
+  bool save(const uint8_t *, size_t) { return true; }
+  bool load(uint8_t *, size_t) { return true; }
+};
+static_assert(PreferenceBackendContract<MinimalBackend>);
+
+struct BackendMissingLoad {
+  bool save(const uint8_t *, size_t) { return true; }
+};
+static_assert(!PreferenceBackendContract<BackendMissingLoad>);
+
+struct BackendWrongReturn {
+  void save(const uint8_t *, size_t) {}
+  bool load(uint8_t *, size_t) { return true; }
+};
+static_assert(!PreferenceBackendContract<BackendWrongReturn>);
+
+struct MinimalPreferences : public PreferencesMixin<MinimalPreferences> {
+  using PreferencesMixin<MinimalPreferences>::make_preference;
+  ESPPreferenceObject make_preference(size_t, uint32_t, bool) { return {}; }
+  ESPPreferenceObject make_preference(size_t, uint32_t) { return {}; }
+  bool sync() { return true; }
+  bool reset() { return true; }
+};
+static_assert(PreferencesContract<MinimalPreferences>);
+
+struct PreferencesMissingTwoArgForm : public PreferencesMixin<PreferencesMissingTwoArgForm> {
+  using PreferencesMixin<PreferencesMissingTwoArgForm>::make_preference;
+  ESPPreferenceObject make_preference(size_t, uint32_t, bool) { return {}; }
+  bool sync() { return true; }
+  bool reset() { return true; }
+};
+static_assert(!PreferencesContract<PreferencesMissingTwoArgForm>);
+
+struct PreferencesMissingReset : public PreferencesMixin<PreferencesMissingReset> {
+  using PreferencesMixin<PreferencesMissingReset>::make_preference;
+  ESPPreferenceObject make_preference(size_t, uint32_t, bool) { return {}; }
+  ESPPreferenceObject make_preference(size_t, uint32_t) { return {}; }
+  bool sync() { return true; }
+};
+static_assert(!PreferencesContract<PreferencesMissingReset>);
+
+struct PreferencesWrongSyncReturn : public PreferencesMixin<PreferencesWrongSyncReturn> {
+  using PreferencesMixin<PreferencesWrongSyncReturn>::make_preference;
+  ESPPreferenceObject make_preference(size_t, uint32_t, bool) { return {}; }
+  ESPPreferenceObject make_preference(size_t, uint32_t) { return {}; }
+  void sync() {}
+  bool reset() { return true; }
+};
+static_assert(!PreferencesContract<PreferencesWrongSyncReturn>);
+
+// Forgot `using PreferencesMixin<X>::make_preference;`, so the derived
+// overloads hide the template forms (see the PreferencesContract note in
+// preference_backend.h); the concept must reject the class.
+struct PreferencesForgotUsingDeclaration : public PreferencesMixin<PreferencesForgotUsingDeclaration> {
+  ESPPreferenceObject make_preference(size_t, uint32_t, bool) { return {}; }
+  ESPPreferenceObject make_preference(size_t, uint32_t) { return {}; }
+  bool sync() { return true; }
+  bool reset() { return true; }
+};
+static_assert(!PreferencesContract<PreferencesForgotUsingDeclaration>);
+
+struct MinimalKeyLookup {
+  bool load_from_key(uint32_t, uint8_t *, size_t) { return true; }
+};
+static_assert(PreferencesKeyLookupContract<MinimalKeyLookup>);
+
+struct KeyLookupMissingMethod {};
+static_assert(!PreferencesKeyLookupContract<KeyLookupMissingMethod>);
+
+TEST(PreferenceContract, NullBackendRefusesBothOperations) {
+  // ESPPreferenceObject forwards to whichever backend the platform binds; a
+  // default-constructed object has no backend and must refuse both operations
+  // instead of crashing.
+  ESPPreferenceObject without_backend;
+  uint32_t value = 42;
+  EXPECT_FALSE(without_backend.save(&value));
+  EXPECT_FALSE(without_backend.load(&value));
+}
+
+}  // namespace esphome::core::testing