Fix handling of float default values#1033
Open
danieleades wants to merge 1 commit into
Open
Conversation
Two related bugs in float default handling:
- In has_default, the match arm intended to treat a 0.0 default on a
float property as the intrinsic default matched Integer instead of
Float (an unreachable duplicate of the preceding arm), so a
`{"type": "number", "default": 0.0}` property generated a bespoke
defaults function rather than a plain #[serde(default)]. The integer
arm now uses as_f64() == Some(0.0), which subsumes the previous
as_u64() check and also accepts an integer default written as 0.0.
- In validate_value, a non-zero float default returned
DefaultKind::Generic(DefaultImpl::I64), registering the shared
default_i64 helper even though default_fn has no Float arm and
generates a bespoke function anyway, leaving default_i64 emitted but
unused (and unusable for floats, since f64 does not implement
TryFrom<i64>). Non-zero float defaults now return
DefaultKind::Specific.
Related to oxidecomputer#442; follow-up to oxidecomputer#976.
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
1 participant
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.
Problem
Two related bugs in float default-value handling:
In
has_default(typify-impl/src/structs.rs) there were two consecutive match arms forTypeEntryDetails::Integerwith a numeric default — the second arm (guarded byn.as_f64() == Some(0.0), commented "implicit default: 0.0") was clearly intended to matchTypeEntryDetails::Floatbut matchedIntegerinstead, making it unreachable. As a result, a property like{"type": "number", "default": 0.0}was not recognized as having the intrinsic default, and a bespokedefaults::xxx()function returning0.0_f64was generated instead of a plain#[serde(default)].In
validate_value(typify-impl/src/defaults.rs), a non-zero float default returnedDefaultKind::Generic(DefaultImpl::I64). That registers the shareddefault_i64helper in the generateddefaultsmodule, butdefault_fnhas noFloatarm, so a bespoke function is generated anyway anddefault_i64is emitted dead/unused (it couldn't be used for floats regardless, sincef64: TryFrom<i64>doesn't hold). This shows up today as an unuseddefault_i64in thevega.outtest output.Fix
has_default: the first arm now matches integers with a zero default vian.as_f64() == Some(0.0)(subsuming the previousas_u64() == Some(0)check and also handling a default written as0.0), and the second arm now matchesFloatas intended.validate_value: non-zero float defaults now returnDefaultKind::Specific, matching howdefault_fnactually handles them, so the unusabledefault_i64helper is no longer registered.Regression coverage: a
MrDefaultFloatstype (one zero and one non-zero float default) added totypify/tests/schemas/types-with-defaults.json, plus atest_default_floatunit test intypify-impl/src/defaults.rs.Trade-offs
Previously-generated bespoke zero-default functions (e.g.
fn xxx_zfreq() -> f64 { 0.0_f64 }) disappear from output in favor of plain#[serde(default)], and the deaddefault_i64helper is no longer emitted when only float defaults required it — a cosmetic change to generated code with identical runtime behavior.Related to (but not fully fixing) #442, and a follow-up to #976.