Skip to content
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.

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

Safe oneOf #94

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Safe oneOf #94

merged 3 commits into from
Dec 16, 2024

Conversation

hudson-ai
Copy link
Collaborator

Adds a method to check for "verifiable" schema disjointness (returns false if we're not really sure) and uses it to coerce oneOf to anyOf whenever it is provably safe to do so. This includes cases where objects have required properties that are themselves provably disjoint, covering discriminated unions.

We could probably take this one step farther and actually compute the difference between some schemas (e.g. Schema::Boolean.subtract(Schema::LiteralBoolean{ true }) => Schema::LiteralBoolean{ false }), but simply checking for disjointness covers some important cases.

Closes (or at least contributes significant progress towards) #77

@hudson-ai hudson-ai requested review from mmoskal and Copilot December 13, 2024 21:05

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@hudson-ai
Copy link
Collaborator Author

This is fun.

This compiles:

{"oneOf": [{"enum": [1,2,"foo"]}, {"enum": [2,3,"bar"]}], "type": "string"}

This does not:

{"oneOf": [{"enum": [1,2,"foo"]}, {"enum": [2,3,"bar"]}]}

Copy link
Collaborator

@mmoskal mmoskal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

}),
_ => {
// Except for in the cases above, it should suffice to check that the types are different
mem::discriminant(self) != mem::discriminant(other)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! I didn't know this existed!

Actually, on that note, maybe in future, Schema::Boolean should have value: Optional and we could drop LiteralBool? The same way numbers and strings are treated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, duh. That's a good idea. 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mmoskal I think we really need to implement refs here (could be in the next PR), but we'll definitely have to be careful about ref cycles causing infinite loops. Maybe setting a max ref depth on this method would be an ok solution

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.s. matching on RegexAst::Literal can replace the const_string check in your const_compile PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to match on const anything - we just always compile strings as regexes and or them together with other options in AnyOf

@hudson-ai hudson-ai merged commit a821d67 into guidance-ai:main Dec 16, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants