Skip to content

Fix unsigned integer inference for nonnegative ranges #1102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TvdW wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix unsigned integer inference for nonnegative ranges #1102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 155 additions & 46 deletions typify-impl/src/convert.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ impl TypeSpace {
break;
}
}
let (type_entry, _) = self.convert_schema(type_name, &new_schema).unwrap();
let (type_entry, _) = self.convert_schema(type_name, &new_schema)?;
Ok((type_entry, metadata))
}

Expand Down Expand Up @@ -985,15 +985,15 @@ impl TypeSpace {
let (mut min, mut max, multiple) = if let Some(validation) = validation {
let min = match (&validation.minimum, &validation.exclusive_minimum) {
(None, None) => None,
(None, Some(value)) => Some(value + 1.0),
(None, Some(value)) => Some(value.floor() + 1.0),
(Some(value), None) => Some(*value),
(Some(min), Some(emin)) => Some(min.max(emin + 1.0)),
(Some(min), Some(emin)) => Some(min.max(emin.floor() + 1.0)),
};
let max = match (&validation.maximum, &validation.exclusive_maximum) {
(None, None) => None,
(None, Some(value)) => Some(value - 1.0),
(None, Some(value)) => Some(value.ceil() - 1.0),
(Some(value), None) => Some(*value),
(Some(max), Some(emax)) => Some(max.min(emax - 1.0)),
(Some(max), Some(emax)) => Some(max.min(emax.ceil() - 1.0)),
};
(min, max, validation.multiple_of)
} else {
Expand Down Expand Up @@ -1082,9 +1082,10 @@ impl TypeSpace {
.iter()
.find(|(int_format, _, _, _, _)| int_format == format)
{
let valid_min = min.is_none() || min.map(|fmin| fmin.ge(imin)).unwrap_or(false);
let valid_max = max.is_none() || max.map(|fmax| fmax.le(imax)).unwrap_or(false);
if multiple.is_none() && valid_min && valid_max {
let valid_min = min.is_none_or(|min| *imin <= min && min <= *imax);
let valid_max = max.is_none_or(|max| *imin <= max && max <= *imax);
let valid_range = min.zip(max).is_none_or(|(min, max)| min <= max);
if multiple.is_none() && valid_min && valid_max && valid_range {
// If there's a default value and it's either not a number
// or outside of the range for this format, return an
// error.
Expand All @@ -1106,10 +1107,10 @@ impl TypeSpace {
}
}

if min.is_none() {
if min.is_none() && max.is_none_or(|max| *imin <= max) {
min = Some(*imin);
}
if max.is_none() {
if max.is_none() && min.is_none_or(|min| min <= *imax) {
max = Some(*imax);
}
}
Expand Down Expand Up @@ -1166,16 +1167,32 @@ impl TypeSpace {
(None, None) => None,
};

// TODO we should do something with `multiple`
if let Some(ty) = maybe_type {
Ok((TypeEntry::new_integer(ty), metadata))
} else {
// TODO we could construct a type that itself enforces the various
// bounds.
// TODO failing that, we should find the type that most tightly
// matches these bounds.
Ok((TypeEntry::new_integer("i64"), metadata))
let ty = maybe_type.unwrap_or_else(|| {
if min.is_some_and(|min| min >= 0.) {
"u64".to_string()
} else {
"i64".to_string()
}
});

// NonZero types share signed/unsigned rows; use their unsigned limits.
let (_, _, _, imin, imax) = formats
.iter()
.rev()
.find(|(_, int_ty, nz_ty, _, _)| ty == *int_ty || ty == *nz_ty)
.unwrap();
if min.is_some_and(|min| min < *imin || min > *imax)
|| max.is_some_and(|max| max < *imin || max > *imax)
|| min.zip(max).is_some_and(|(min, max)| min > max)
{
return Err(Error::InvalidSchema {
type_name: metadata.as_ref().and_then(|m| m.title.clone()),
reason: "integer bounds do not fit any supported integer type".to_string(),
});
}

// TODO we should do something with `multiple`
Ok((TypeEntry::new_integer(ty), metadata))
}

fn convert_number<'a>(
Expand Down Expand Up @@ -2117,9 +2134,7 @@ mod tests {
};

#[track_caller]
fn int_helper<T: JsonSchema>(type_name: &'static str) {
let schema = schema_for!(T);

fn int_helper(schema: RootSchema, type_name: &str) {
let mut type_space = TypeSpace::default();
type_space
.add_ref_types(schema.definitions.clone())
Expand All @@ -2134,15 +2149,15 @@ mod tests {
let output = ty.type_name(&type_space);
let actual = output.split("::").last().unwrap().trim();
let expected = type_name.split("::").last().unwrap();
assert_eq!(actual, expected);
assert_eq!(actual, expected, "{schema:?}");
}

macro_rules! int_test {
($t:ty) => {
paste! {
#[test]
fn [<test_int_ $t:lower>]() {
int_helper::<$t>(stringify!($t))
int_helper(schema_for!($t), stringify!($t))
}
}
};
Expand All @@ -2161,6 +2176,110 @@ mod tests {
int_test!(NonZeroU32);
int_test!(NonZeroU64);

#[test]
fn test_integer_fallback() {
for (bounds, expected) in [
(json!({"minimum": 0, "maximum": 100}), "u64"),
(json!({"minimum": 2, "maximum": 100}), "u64"),
(
json!({"minimum": 2, "maximum": 9223372036854775808u64}),
"u64",
),
(
json!({"minimum": 1, "maximum": 100}),
"::std::num::NonZeroU64",
),
(json!({"minimum": -2, "maximum": 100}), "i64"),
(json!({"minimum": -128, "maximum": 127}), "i8"),
(json!({"minimum": 0, "maximum": 255}), "u8"),
(json!({"minimum": 2}), "u64"),
(json!({"minimum": 2, "multipleOf": 2}), "u64"),
(json!({"format": "uint8", "minimum": 1000}), "u64"),
(json!({"format": "int8", "maximum": -1000}), "i64"),
(
json!({"exclusiveMinimum": 0, "exclusiveMaximum": 1.5}),
"::std::num::NonZeroU64",
),
(
json!({"exclusiveMinimum": 5, "exclusiveMaximum": 6.5}),
"u64",
),
(
json!({"exclusiveMinimum": -1.5, "exclusiveMaximum": -0.5}),
"i64",
),
(
json!({"minimum": 6, "exclusiveMinimum": 5.1, "maximum": 7, "exclusiveMaximum": 6.5}),
"u64",
),
(json!({}), "i64"),
(
json!({"format": "int32", "minimum": 2, "maximum": 100}),
"i32",
),
(
json!({"minimum": -9223372036854775808i64, "maximum": 9223372036854775807i64}),
"i64",
),
(
json!({"format": "uint64", "minimum": 0, "maximum": 18446744073709551615u64}),
"u64",
),
] {
let mut schema = bounds;
schema["type"] = json!("integer");
int_helper(serde_json::from_value(schema).unwrap(), expected);
}
}

#[test]
fn test_integer_unsupported_ranges() {
for bounds in [
json!({"minimum": 2, "maximum": 36893488147419103232.0}),
json!({"minimum": 1, "maximum": 36893488147419103232.0}),
json!({"format": "uint64", "minimum": 0, "maximum": 36893488147419103232.0}),
json!({"minimum": -2, "maximum": 9223372036854777856u64}),
json!({"minimum": -18446744073709551616.0}),
json!({"minimum": 36893488147419103232.0}),
json!({"exclusiveMinimum": 2, "exclusiveMaximum": 3}),
json!({"format": "int8", "minimum": 20, "maximum": 10}),
json!({"format": "uint64", "minimum": -1}),
] {
let mut schema = bounds;
schema["type"] = json!("integer");
let schema: RootSchema = serde_json::from_value(schema).unwrap();
let result = TypeSpace::default().add_type(&schema.schema.clone().into());
assert!(
matches!(result, Err(Error::InvalidSchema { ref reason, .. })
if reason.contains("supported integer type")),
"{schema:?}: {result:?}"
);
}
}

#[test]
fn test_integer_unsupported_reference_bounds() {
let schema: RootSchema = serde_json::from_value(json!({
"title": "Config",
"type": "object",
"definitions": {"Base": {"type": "integer"}},
"properties": {
"n": {
"$ref": "#/definitions/Base",
"minimum": 2,
"maximum": 36893488147419103232.0
}
}
}))
.unwrap();
let result = TypeSpace::default().add_root_schema(schema);
assert!(
matches!(result, Err(Error::InvalidSchema { ref reason, .. })
if reason.contains("supported integer type")),
"{result:?}"
);
}

#[test]
fn test_redundant_types() {
#[derive(JsonSchema)]
Expand Down Expand Up @@ -2224,28 +2343,18 @@ mod tests {

#[test]
fn test_low_default() {
let schema = SchemaObject {
instance_type: Some(InstanceType::Integer.into()),
format: Some("uint".to_string()),
metadata: Some(
Metadata {
default: Some(json!(-1i32)),
..Default::default()
}
.into(),
),
number: Some(NumberValidation::default().into()),
..Default::default()
};

let mut type_space = TypeSpace::default();
match type_space.convert_schema_object(
Name::Unknown,
&schemars::schema::Schema::Object(schema.clone()),
&schema,
) {
Err(Error::InvalidValue) => (),
_ => panic!("unexpected result"),
for schema in [
json!({"type": "integer", "format": "uint", "default": -1}),
json!({"type": "integer", "minimum": 2, "default": 1}),
] {
let schema: SchemaObject = serde_json::from_value(schema).unwrap();
let original_schema = schemars::schema::Schema::Object(schema.clone());
let result = TypeSpace::default().convert_schema_object(
Name::Unknown,
&original_schema,
&schema,
);
assert!(matches!(result, Err(Error::InvalidValue)), "{schema:?}");
}
}

Expand Down
25 changes: 25 additions & 0 deletions typify-test/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,31 @@ fn main() {
ipnetwork::IpNetwork::add(&mut type_space);
TriplePattern::add(&mut type_space);

let mut integer_range_schema: serde_json::Value = serde_json::from_str(
r#"{
"title": "Config",
"type": "object",
"properties": {
"n": {
"type": "integer",
"minimum": 2,
"maximum": 9223372036854775808
}
},
"required": ["n"]
}"#,
)
.unwrap();
type_space
.add_root_schema(serde_json::from_value(integer_range_schema.clone()).unwrap())
.unwrap();
integer_range_schema["title"] = serde_json::json!("SignedConfig");
integer_range_schema["properties"]["n"]["minimum"] = serde_json::json!(i64::MIN);
integer_range_schema["properties"]["n"]["maximum"] = serde_json::json!(i64::MAX);
type_space
.add_root_schema(serde_json::from_value(integer_range_schema).unwrap())
.unwrap();

let contents =
prettyplease::unparse(&syn::parse2::<syn::File>(type_space.to_stream()).unwrap());

Expand Down
22 changes: 22 additions & 0 deletions typify-test/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,28 @@ include!(concat!(env!("OUT_DIR"), "/codegen.rs"));

fn main() {}

#[test]
fn test_integer_range_above_i64_max() {
for (input, expected) in [
(r#"{"n": 2}"#, 2),
(r#"{"n": 9223372036854775808}"#, 9223372036854775808),
] {
let config: Config = serde_json::from_str(input).unwrap();
assert_eq!(config.n, expected);
}
}

#[test]
fn test_integer_full_i64_range() {
for (input, expected) in [
(r#"{"n": -9223372036854775808}"#, i64::MIN),
(r#"{"n": 9223372036854775807}"#, i64::MAX),
] {
let config: SignedConfig = serde_json::from_str(input).unwrap();
assert_eq!(config.n, expected);
}
}

#[test]
fn test_with_set() {
// Validate that a set is currently represented as a Vec
Expand Down