fix: apply options_schema defaults at runtime, enforce component behaviour at compile time#123
Merged
Merged
Conversation
…haviour at compile time
Schema `default:` values declared in a component's `options_schema/0` were
never applied at runtime. The component servers called `callback_module.init/1`
with opts straight from `ParamResolution.resolve/2`, which only swaps
`ParamRef`s — it never ran `Spark.Options.validate/2`. The schema's
`default:` was exercised only by the compile-time verifier, which threw the
validated result away.
Symptom: a sensor like `BB.Sensor.BMI323` declared sensible defaults but
`init/1` crashed with `KeyError` whenever a defaulted key was omitted — even
though the schema said the default would be applied. Same bug bit every AHRS
filter in `bb_estimator_ahrs` (`Madgwick`, `Mahony`, `Complementary`):
`Keyword.fetch!(opts, :beta)` raised when `beta` was omitted.
## Changes
- New `BB.Component.OptionsSchema` helper with `validate/3` (splits framework
keys, runs `Spark.Options.validate/2` applying defaults, merges them back)
and `inject/2` (the schema-declaration machinery, shared by the six `use`
macros instead of being duplicated per behaviour).
- The six `use BB.X` macros (sensor / actuator / controller / estimator /
bridge / command) now delegate to the shared helper. `options_schema/0` is
always defined — `use BB.X` without a schema gets an overridable empty
`Spark.Options.new!([])` — so callers never need a `function_exported?/3`
guard.
- Providing both `options_schema:` to `use` *and* a hand-written
`def options_schema/0` is now a hard `CompileError` (via `@before_compile`).
Previously the keyword form silently won.
- The five component servers (sensor / actuator / controller / estimator /
command) now validate resolved opts through the helper before calling
`init/1`, and again on the `handle_options/2` param-change path, so schema
defaults flow through at runtime and live param changes stay consistent.
Bridges are unchanged (they have no intermediary server).
- Command is included: a command that passes options must now declare an
`options_schema/0`. The two test commands in `test/bb/command/options_test.exs`
got schemas to match.
- New `BB.Dsl.ValidateChildSpecBehavioursTransformer` enforces that every
module wired into a component slot in the topology DSL actually implements
the matching BB component behaviour. Spark's built-in `{:behaviour, X}`
schema type only validates `is_atom`, not behaviour conformance, so a bare
`use GenServer` module could otherwise be wired in as e.g. an actuator and
pass DSL compilation only to crash at runtime. Lives in a transformer
(raises) rather than a verifier (would only warn — `@after_verify` does
not tolerate errors).
- Command's stored schema is now a compiled `%Spark.Options{}` like every
other component, instead of a raw keyword list. Only the verifier consumes
it and `Spark.Options.validate/2` accepts both forms.
- New regression tests:
- `test/bb/component/options_schema_test.exs` — `validate/3` applies
defaults, preserves framework keys, errors on unknown/missing required
keys; double-declaration raises `CompileError`; missing schema yields
an empty default.
- `test/bb/dsl/validate_child_specs_test.exs` — non-conformant modules
raise `Spark.Error.DslError` from the transformer for actuator,
sensor, and controller slots.
## Downstream impact
This is the first release where commands' `options_schema/0` becomes
load-bearing at runtime. Any command in a dependent package that passes
options without declaring a schema will fail at startup with
`unknown options ...`. Sweep before release.
`bb_sensor_bmi323` and `bb_estimator_ahrs` are confirmed fixed against
`BB_VERSION=local`.
`Code.ensure_loaded/1` returns `{:error, :nofile}` when a referenced module
hasn't been written to disk yet — which happens during the same compilation
pass when an in-tree fixture (e.g. `bb_sensor_bmi323/test/support/test_bot.ex`)
references a sensor defined in the same package's `lib/`. The transformer
was halting compilation in that case even though the referenced module is
perfectly conformant.
Use `Code.ensure_compiled/1` so Mix waits for the dependency, and if even
that fails (genuinely missing module, or cross-package matrix where the
referenced module isn't loadable from the transformer's vantage point),
fall through to `:ok` and let the runtime component server reject a
non-conformant module when it tries to start.
Hand-rolled modules in the SAME source tree (like the previously-broken
`MockActuator` in `bb_ik_fabrik`) are still caught at compile time — they
are loadable but lack the behaviour.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Schema
default:values declared in a component'soptions_schema/0were never applied at runtime. The component servers calledcallback_module.init/1with opts straight fromParamResolution.resolve/2, which only swapsParamRefs — it never ranSpark.Options.validate/2. The schema'sdefault:was exercised only by the compile-time verifier, which threw the validated result away.Symptom:
BB.Sensor.BMI323declared sensible defaults butinit/1crashed withKeyErrorwhenever a defaulted key was omitted — even though the readme said the default would be applied. Same bug bit every AHRS filter inbb_estimator_ahrs(Madgwick,Mahony,Complementary):Keyword.fetch!(opts, :beta)raised whenbetawas omitted.BB.Sensor.OpenLoopPositionEstimatoronly worked because it re-stated every default defensively viaMap.get(opts, :easing, :linear)— a duplication smell this fix retires.Approach
Two interlocking concerns:
Concern A — unify schema declaration. Two ways to declare the schema existed (
use BB.X, options_schema: [...]vsdef options_schema/0) and could silently fight. They now share a single helper, and providing both is a hard compile error. Keyword form stays for self-contained literal schemas; the callback form remains the escape hatch for schemas that reference module attributes / helpers / sigils (which aren't in scope atuseexpansion). With no schema declared, an empty default is injected, sooptions_schema/0is always defined and callers never need afunction_exported?guard.Concern B — apply defaults at runtime. The five component servers (sensor / actuator / controller / estimator / command) now split off framework-injected keys, run
Spark.Options.validate/2againstmodule.options_schema/0(applying defaults), and merge framework keys back — both ininitand on thehandle_options/2param-change path. Bridges are unchanged (they have no intermediary server).Changes
BB.Component.OptionsSchemahelper:validate/3— runtime split / validate / merge.inject/2— the declaration machinery used by the sixusemacros (was duplicated per behaviour).use BB.Xmacros now delegate to the shared helper. Double declaration of keyword + callback raisesCompileErrorvia@before_compile.initandhandle_options.options_schema/0. The two test commands inBB.Command.OptionsTestwere given schemas to match.%Spark.Options{}like the other components (was a raw keyword list). Low-risk: only the verifier consumes it andSpark.Options.validate/2accepts both forms.BB.Dsl.ValidateChildSpecBehavioursTransformerenforces that every module wired into a component slot actually implements the matching behaviour. Spark's built-in{:behaviour, X}schema type only validatesis_atom, not conformance, so a bareuse GenServermodule could otherwise be wired in as e.g. an actuator, passing DSL compilation only to crash at runtime when the server expected callbacks (options_schema/0,init/1, etc.) that weren't defined. Implemented as a transformer (raises) rather than a verifier (would only warn —@after_verifydoesn't tolerate errors). Runs afterUniquenessTransformerso name conflicts are reported first.test/bb/component/options_schema_test.exs—validate/3applies defaults, preserves framework keys, errors on unknown/missing required keys; double declaration raisesCompileError; missing schema yields an empty default.test/bb/dsl/validate_child_specs_test.exs— non-conformant modules raiseSpark.Error.DslErrorfor actuator / sensor / controller slots.Downstream impact
This is the first release where a command's
options_schema/0becomes load-bearing at runtime — a command that passes options without declaring a schema will fail at startup with Spark'sunknown options ...error. A sweep across the workspace (bb_example_so101,bb_example_wx200,bb_ik_dls,bb_ik_fabrik,bb_jido,bb_kino,bb_mcp,bb_reactor) found 19 command modules; none declare a schema, but none are wired with tuple-formhandler({Module, opts})— they all use barehandler(Module), so empty opts validate against the empty default schema. Confirmed empirically againstBB_VERSION=local:bb_kino6/0,bb_mcp88/0,bb_reactor28/0,bb_jido37/0. No downstream changes required.Verification
mix check --no-retryclean (compiler, credo, dialyzer, ex_doc, ex_unit, formatter, mix_audit, reuse, spark_cheat_sheets, spark_formatter, unused_deps). 1057 tests / 0 failures.BB_VERSION=localproves the fix onbb_sensor_bmi323(40/0) andbb_estimator_ahrs(42/0).mix check --no-retrysweep across all 20 workspace repos against this branch passes everywhere that's currently green; the residual failures (bb_servo_pca9685,bb_servo_robotis) are a pre-existingBB.Igniter.set_robot_opts/3API drift addressed in companion PRs (chore(deps): bump github/codeql-action from 3.31.10 to 4.31.10 #39 and feat: addBB.Sensor.Mimicfor mechanically-linked joints #34 respectively).Open follow-ups (left for review feedback)
BB.Sensor.OpenLoopPositionEstimator's defensiveMap.get(opts, key, default)duplication now that the server applies the schema defaults? It would assert that callbackinit/1may assume server pre-validation — that affects the test which currently callsinit/1directly with partial opts. Worth its own change if you want to make that contract explicit.