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

Make all types and coercions inlinable #10

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

haarg
Copy link
Contributor

@haarg haarg commented Feb 16, 2017

This is incomplete, but it is an attempt to make all types and coercions inlinable, even if they don't have an inline implementation. It does this by storing the type in the environment hash that is returned by the various functions. There are a few issues:

  • Documentation and tests haven't been updated
  • inline_check_always and inline_coercion_always are terrible names.
  • It makes the can_inline_* methods misleading.

The docs specify that there will not be a trailing semicolon on the
return value of inline_coercion and inline_coercion_and_check.  Adjust
the code to match the docs.
These functions are similar to inline_coercion and inline_check, but
they will work even if can_be_inlined is false.  They return a code
string and an environment hash.
Allow the inline_coercion, inline_coercion_and_check, and inline_assert
methods to work on non-inlinable types and coercions.  Non-inlinable
types will be stored in the environment hash.
@autarch
Copy link
Member

autarch commented Feb 19, 2017

I think it might make more sense to just have the existing methods always inline and to change things like can_inline_* to has_explicit_inline_* or something like that.

AFAIK, I'm the only downstream consumer of this module. The only thing I'd want to check is that Moose works properly with any changes along these lines. I know that Moose expects a can_be_inlined method to be present. Always returning true for that method should be fine, since the faux inlining you're doing here is really not any different than what Moose ends up doing itself for non-inlinable constraints.

@haarg
Copy link
Contributor Author

haarg commented Feb 20, 2017

I think having the C<inline_coerce> and C<inline_check> always support inlining would make the most sense, but it would require changing their return from a single string to a string + hash.

Would we also want a method for the just the inlinable string like the methods return now?

@haarg
Copy link
Contributor Author

haarg commented Feb 20, 2017

It's worth noting that Type::Tiny also provides an inline_check method, with similar limitations. If we could come up with a suitable name for a new method, it could be implemented in both Specio and Type::Tiny.

@autarch
Copy link
Member

autarch commented Feb 25, 2017

I think having the C<inline_coerce> and C<inline_check> always support inlining would make the most sense, but it would require changing their return from a single string to a string + hash.

Would we also want a method for the just the inlinable string like the methods return now?

There's no inline_coerce method, just inline_coercion, which already returns 2 elements.

I think for inline_check having it return 2 elements would make sense. The way I'm using it right now is to get the inlining for a parent constraint while creating inlining for a child. I think I'd add an inline_check_no_env method that dies if the check requires environment.

The big issue is that anyone using this in the wild will encounter some breakage if they're using inline_check like the builtin libraries do now. That said, I don't think Specio has gotten any huge adoption, and it's better to fix these issues now.

Type::Tiny doesn't support environment variables at all, except just to return an empty hashref for the benefit of Moose. It'd be nice if it supported the same API as Specio, inasmuch as is feasible.

@haarg
Copy link
Contributor Author

haarg commented Feb 26, 2017

I got the name wrong, but I was referring to Specio::Coercion::inline_coercion.

There aren't many users of Specio, but DateTime is an important one, and it will break with this proposed change. That means someone upgrading Params::ValidationCompiler (after it is updated to use the new API) will break DateTime. Obviously there are things like Dist::CheckConflicts to help with that, but it isn't a perfect solution. They are your modules though, so you'll have to make that call.

Type::Tiny doesn't support environment as part of its type API, but it would be pretty easy to add. Its eval_closure implementation supports it. Even if it doesn't use the environment for the type internally, having a method that returns an environment along with the string would make sense.

@autarch
Copy link
Member

autarch commented Feb 26, 2017

Would DateTime break? This only breaks PVC for coercions that can be inlined which are attached to types which cannot be inlined. All the types used in DateTime should be inlinable and none of the types have coercions.

@haarg
Copy link
Contributor Author

haarg commented Feb 26, 2017

So have the method only return a single item if the environment it would include is empty?

@autarch
Copy link
Member

autarch commented Feb 26, 2017

It's going to be a very rare case where this breaks PVC. That said, I could even special case and check for the caller and PVC version or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants