Fail composition when a dangling override is rescued by an append - #3319
Conversation
A Defaults List override with no earlier Group Default to override correctly fails on its own, but a matching command line append made the composition succeed and silently replaced the appended value with the dangling override's. Stop registering a defaults list's own override when an externally appended group default of the same key is resolved. The appended entry now composes without consulting the dangling override, which is then reported unused, so the composition fails exactly as it does without the append. Command line overrides of appended groups are registered before traversal and still apply. Add regression coverage for the dangling override with and without the append. Fixes facebookresearch#3318
c93d809 to
f48ed4f
Compare
omry
left a comment
There was a problem hiding this comment.
Thanks for calling out the error message in the description. I agree that it should be improved in this PR.
The behavior is now correct, but with +db=mysql the current diagnostic is confusing:
Could not override 'db'.
Did you mean to override db?
The actual problem is the dangling override db: sqlite in config, not the command-line append. Please report config-sourced dangling overrides directly, for example:
In 'config': Invalid Defaults List override 'db: sqlite'.
No earlier Group Default for 'db' exists to override.
The diagnostic should be identical with and without +db=mysql. Please add regression coverage for both cases. Candidate suggestions for command-line overrides can retain their existing behavior.
|
One clarification to my review: a config-sourced override cannot always fail immediately when first encountered. A command-line selection can change an earlier subtree and thereby introduce the Group Default that makes the later override valid. Please add regression coverage for this full matrix:
The validity check therefore needs to use the effective depth-first Defaults List while distinguishing eligible earlier defaults from later command-line appends. |
An unused Defaults List override coming from a config is now reported as
In '<config>': Invalid Defaults List override '<key>: <value>'.
No earlier Group Default for '<full key>' exists to override.
instead of the command line oriented "Could not override" message with
candidate suggestions, which could suggest the very group the user just
appended. The diagnostic is identical with and without a command line append.
Command line overrides retain their existing messages and suggestions.
Validity is evaluated against the effective depth-first Defaults List, after
command line selections are applied: a config override is legal exactly when
an eligible earlier Group Default exists in the composed tree, so an earlier
selection that introduces the target makes the override valid, while later
command line appends never do. Add regression coverage for the full matrix.
|
Both points addressed in 3ca3035. Diagnostic. Config-sourced unused overrides now report directly, in your suggested form: The message is byte-identical with and without a command-line append — both regression tests assert the same string — and never lists candidates, so the appended group is no longer suggested. Command-line overrides keep their existing "Could not override / Did you mean" messages and the The matrix. All five cases are covered by the
On your clarification: the check indeed cannot run at first encounter, and the implementation doesn't — validity falls out of the traversal itself. An override is marked used when it applies to a Group Default during composition of the effective tree (after command-line selections resolve), and the reverse traversal visits appends before any config override registers, so appends can never mark one used.
|
Fixes #3318
Rebased onto
mainnow that #3315 has landed — single commit.Problem
Per #3318: a Defaults List override with no earlier Group Default correctly fails on its own, but a matching command line append made the composition succeed and silently discarded the appended value:
Fix
#3315 preserved this behavior deliberately (registering the list's own override when an externally appended group default of the same key resolves), on the principle that a precedence fix shouldn't change unrelated semantics. With #3318 ruling the behavior itself invalid, that registration path is removed. The appended entry now composes without consulting the dangling override, the override is reported unused, and composition fails exactly as it does without the append:
ensure_overrides_used), so+db=mysqlnever rescues the invalid Defaults List.+db=mysql db=postgres) are registered before traversal and still apply — covered by the existing suites.Per review, config-sourced dangling overrides are now reported directly instead of through the command-line-oriented candidate message:
The diagnostic is byte-identical with and without a command-line append and never suggests candidates. Command-line overrides keep their existing messages. Validity is evaluated against the effective depth-first Defaults List after command-line selections apply — an earlier selection that introduces the target makes a later config override valid, while later appends never do — covered by the five-case
dangling_nested_overridematrix from review.Tests
group_override_only++group1=file1→ConfigCompositionException. This test fails on Make later Defaults List overrides take precedence #3315 (where it composed), pinning the new semantics.group_override_onlywith no append → same failure as before the change, guarding against the fix drifting the base case.pytest tests/defaults_list tests/test_compose.py→ 423 passed. Full suite excludingtest_completion→ 2056 passed.