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

Introduce polymorphic schemas #445

Closed
wants to merge 13 commits into from

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Aug 13, 2022

Adds polymorphic schemas with direct support for s/{de}fn etc.

My goal for this first pass is to represent the schema for sequence arities of clojure.core/map in one function. We'd need to (at least) look into #446 for the transducer arities.

@frenchy64
Copy link
Contributor Author

Seems pretty solid for s/defn. Going to test s/fn, s/letfn and others.

@frenchy64
Copy link
Contributor Author

Pretty happy with this now. Can't see anything extraneous.

x)
```

The actual value chosen as the "most general" depends on the polymorphic variables kind and should not be
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I don't understand from here down. Are you basing this design off an existing language or library that I can read about to get a better understanding? (I'm pretty familiar with Haskell and Scala's typesystems but less so with e.g. depenent types).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's a lot going on here and I prematurely jumped into too many details. I wrote about about some of it here, please take a look.

I will work on communicating better in the the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a bunch of readability improvements to the gist.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks -- that's very helpful context, and I think I understand it now.

I guess my initial reaction is that it's interesting, but I'm unsure about whether the utility of dotted types (and the corresponding generative testing stuff) pays for its complexity. I generally try to err on the side of keeping things simple and adding complexity only where it's clear there's high value for the user, and in this case I'm unsure both about how many people would use this functionality, and how many bugs would actually be caught by the testing. What are your thoughts about these questions?

I'll also mull it over and read some more. Not saying I'm necessarily opposed, but just want to make sure I understand the tradeoffs. Not sure if anyone else here has an opinion?

Copy link
Contributor Author

@frenchy64 frenchy64 Aug 15, 2022

Choose a reason for hiding this comment

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

Thanks, I know you're focussing on the utility dotted variable functionality, but I'll try and step back even further and give my feeling on all the features we've discussed.

how many people would use this functionality

It's a good question. I'm drawing on several data points.

The inspiration for the "just check a function against its schema" operation comes from clojure.spec, which exposes something similar called clojure.spec.test.alpha/check which will automatically generate test cases for your function based on its spec. This is popular functionality in private code IME and seemingly in open source projects on github.

I don't think something like clojure.spec.test.alpha/check exists for schema beyond schema.generators.fn-schema/check in #444. That alone I think would be a good addition to schema, even without polymorphism. Adding polymorphic types would make it more expressive, dotted variables more-so. Making schemas more expressive means that people can use check on more functions (I'm guessing that would go down well).

From personal experience, I've seen code bases that use schema that add polymorphic variables as comments to better explain what the function does. I'm guessing converting them to schemas and having them verified for "free" would be compelling.

how many bugs would actually be caught by the testing

It would be interesting to know how effective schema.generators.fn-schema/check (fuzz testing for s/defn functions) might be:

  1. without polymorphism
  2. with (non-dotted) polymorphism
  3. with dotted polymorphism

I'm guessing there are diminishing returns because the number of functions that require these features diminishes quickly (eg., 1 is any CRUD app fn---tons of those, 2 might be an internal library function---maybe 20x less, 3 might be a fleshed out and generalized library function released to the world---maybe 100x less).

But if we treat each function on its own merits, IME its very often that something that requires dotted polymorphism is a manually unrolled function with 4+ arities. Some of these expansions are monstrously complex (check out every-pred and some-fn). I'm not aware of any tool that can fuzz test something like that beyond writing a custom test.check expression. To me at least, that's a compelling use-case.

unsure about whether the utility of dotted types (and the corresponding generative testing stuff) pays for its complexity

It's also worth entertaining the possibility that these features go down great. It might invite an even longer list of complex feature requests for being able to check all kinds of functions (eg., return values per arity, transducers). Is that where we want schema to go? Personally, I think it would be awesome, but I'm not your average Clojure programmer. If I put my CRUD-developer hat on (that's my day job), then yes, perhaps this dotted polymorphic stuff is not that important and the state of things is good enough.

Copy link
Member

Choose a reason for hiding this comment

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

The inspiration for the "just check a function against its schema" operation comes from clojure.spec, which exposes something similar called clojure.spec.test.alpha/check which will automatically generate test cases for your function based on its spec.

Interesting, I didn't know about that, thanks. I love generative testing in general, but my intuition was that most of the value came from richer invariants between the input and output than can be expressed by types/schemas. For example, when testing every-pred I would want to make sure I actually returned the right value, not just the right type, over all the various arities; I have always assumed that turning on validation for a couple hand-specified unit tests gives most of the value you could hope for from simple schema validation.

Since I think the utility of this automatic validation is at the core of the discussion here, I'd like to try to get on the same page and dispel any misunderstandings I might have. I looked through the first few pages of search results you linked to and didn't find any examples that were particularly convincing to me. (One thing I did find is that spec supports invariants that relate the input and output, which definitely provides more power and could make for more meaningful generative testing). Do you remember any specific cases where it's felt especially valuable?

One more question, would it be practical to incubate this feature in a downstream library? That's generally the approach I've tried to take with non-core features like generators when possible. (The generators library might actually be a good fit for this functionality, actually). I know there are some interactions with richness of function schemas and syntax which could be tricky, happy to discuss that if this seems like a potentially viable path.

If you feel really strongly that we should add this here now, I don't want to block you. I agree this could go down great and am certainly excited about new ideas that could improve the quality of testing and enhance the schema ecosystem overall, but if possible I'd really prefer to have stronger understanding of the value proposition (e.g. from examples or community support) before adding complexity here.

Hope that makes sense, thanks for bearing with me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you remember any specific cases where it's felt especially valuable?

There aren't really situations where this feature (stest/check) competes with hand-crafted properties. They're complimentary. IMO stest/check shares similarities with static type checking: just write your code and magic happens to reveal some logical inconsistencies. The more specific your types/specs, the more detailed the checks, and the more you can "lean" on the tool.

It's also interesting to ponder a situation where the programmer is unfamiliar with test.check or even generative testing and just knows Clojure and clojure.spec. Right off the bat, without stest/check they wouldn't use generative testing very effectively (if at all).

But let's say they try stest/check because some tutorial told them to. Eventually, the generator for some argument needs tweaking, so they learn enough about generators to override a custom generator with clojure.spec.alpha/with-gen.

Maybe they use gen/generate to grab values and write one-off (non-shrinking) properties. Then, they might learn that using clojure.test.check/for-all gives you shrinking, so they learn that.

At this point, they may even use both stest/check and custom properties in tandem. They rely on a baseline beyond "it compiles", or even "it's instrumented": your function is fuzz tested with all combinations of (general) specs.

This is more the narrative I see. Certainly stest/check directly inspired me to learn how to create my own properties.

One more question, would it be practical to incubate this feature in a downstream library?

Yes I think so. The only drawback would be forcing users to use wrappers for s/defn and s/=> that provide the new syntax.

But the big advantage is that you could try out these features for yourself to get a better feel for them.

If you feel really strongly that we should add this here now, I don't want to block you.

No, I'd much rather collaborate with you to find something that matches the spirit of this library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fleshed out the idea of poly schemas + generative testing in a separate place. We can now see how this might look in action (you can try it by cloning the repo and lein repl).

Here's how I'd check clojure.core/comp: https://github.com/frenchy64/schema-incubator/blob/main/test/cljc/com/ambrosebs/schema_incubator/poly/validate_test.cljc#L220-L276

The basic idea is to enumerate all the possible arities and use quick-check to check a good subset of them.

Here's a WIP of checking that every-pred short-circuits with one arg: https://github.com/frenchy64/schema-incubator/blob/be7f31a16b41dc2d7c8a9fba15da0833c57a85d9/test/cljc/com/ambrosebs/schema_incubator/poly/validate_test.cljc#L169-L218

Same idea, but needs more work.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, I really appreciate your flexibility and understanding! Sorry I dropped out from this conversation, things got a little hectic at home but I will definitely take a look as soon as I get a chance.


(s/defn rest-args-mono :- s/Any
[& xs :- [{:a s/Any :b s/Any}]]
x)
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand on this example please? I'm not following what the dotted variables mean or when I would want to use them.

@frenchy64
Copy link
Contributor Author

@frenchy64 frenchy64 closed this Oct 1, 2022
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