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

ADTs defined using S functions are compatible only with types known to S #262

Closed
davidchambers opened this issue Oct 1, 2016 · 8 comments

Comments

@davidchambers
Copy link
Member

In resolving conflicts between #216 and #232 I discovered a problem. Consider this example:

//# Identity#fantasy-land/traverse :: Applicative f => Identity a ~> (a -> f b, c -> f c) -> f (Identity b)
Identity.prototype['fantasy-land/traverse'] = function(f, of) {
  return S.map(Identity, f(this.value));
};

Seems fine, doesn't it? Yes, but for the fact that by using S.map we're limiting the types with which this method is compatible to the types known to S.

This is very much related to sanctuary-js/sanctuary-def#74.

As things stand, I see two workarounds:

  • use S.create({checkTypes: false, env: []}).map in place of S.map; or
  • provide a create :: { checkTypes :: Boolean, env :: Array Type } -> Module function which returns an Identity constructor compatible with the desired set of types.
@davidchambers
Copy link
Member Author

Another option would be to use Z.map, which does not perform type checking.

@Avaq
Copy link
Member

Avaq commented Jan 15, 2017

Using Z.map is perhaps the best work-around for this case, but there are plenty of Sanctuary functions one might use that don't have unchecked equivalents available out of the box. I have several thoughts on the matter:

  • Is this really an issue? By using sanctuary you opt in to using a set environment outside of which nothing can exist. This is just a consequence of that choice.
  • Why does S.create take two options? Doesn't the provision of an environment imply that types should be checked? What else are we providing the environment for? To continue that thought: Why did we ever move away from S.unchecked? An API like this would be easier to use: S = S.safe(S.env), S.unsafe needs no environment. Un-checked map would be available as S.unsafe.map(...).
  • I think we will never be able to provide a convenient way to define an environment which can then be used across multiple modules that build on top of Sanctuary, unless we consider object mutation as a way to extend the environment. We would not even need S.safe(S.env). We would just have S.env.push(MyType) and S would include the type everywhere it's being used (even across multiple modules using compatible Sanctuary versions). It would also be a much easier transition for people who use Sanctuary throughout their code-base and realize they are using a type not in the environment. They can continue to require('sanctuary'). I realize it has other drawbacks, but they don't seems as inconvenient to deal with. I might come back on this thought, need to think it over.

@davidchambers
Copy link
Member Author

Is this really an issue? By using sanctuary you opt in to using a set environment outside of which nothing can exist. This is just a consequence of that choice.

This is a perfectly reasonable decision to make at the application level. An algebraic data type, though, should be usable in applications which do not use Sanctuary (in addition to those which do). Were I to use several FL-compatible ADTs in an application I would expect them to work together without first introducing them to one another. This suggests to me that methods of ADTs should not perform type checking.

Why does S.create take two options? Doesn't the provision of an environment imply that types should be checked?

We could certainly change the type from

create :: { checkTypes :: Boolean, env :: Array Type } -> Module

to

create :: Maybe (Array Type) -> Module

which is undoubtedly more elegant. It may be less convenient, though:

-const S = create({
-  checkTypes: process.env.NODE_ENV !== 'production',
-  env: env.concat([IdentityType]),
-});
+const S = create(process.env.NODE_ENV === 'production' ?
+                   Nothing :
+                   Just(env.concat([IdentityType])));

Why did we ever move away from S.unchecked?

Prior to #206 we supported require('sanctuary') and require('sanctuary').unchecked. The problem was that the environment was fixed by Sanctuary, so S.I(Future.of(1)) was a type error.

An API like this would be easier to use: S = S.safe(S.env), S.unsafe needs no environment.

I don't think we considered this API. We decided the behaviour of require('sanctuary') should remain unchanged (i.e. it should evaluate to a Sanctuary module with type checking enabled).

I'm happy to reconsider the API.

I think we will never be able to provide a convenient way to define an environment which can then be used across multiple modules that build on top of Sanctuary, unless we consider object mutation as a way to extend the environment.

This option scares me, I must admit. 😜

@Avaq
Copy link
Member

Avaq commented Jan 16, 2017

I would expect [third-party algebraic types] to work together without first introducing them to one another

I see your point. That's rather painful. Disabling type-checking was a feature intended to increases performance. It would be a shame to need it elsewhere. Would it be possible to reintroduce "allowing foreign types" as an option?

const {create, env} = require('sanctuary');
const {map} = create({
  env: env.concat(myTypes), //I have some custom types
  checkTypes: process.env.NODE_ENV === 'development', //My user-base likes this ;)
  allowForeign: true //Since I'm making a lib, I should expect types outside of my env
});

This would allow library authors to build on top of Sanctuary, providing its fantastic type-checking for known types, without having to burden users with the creation of an environment or the concept of unknown types.

AMENDMENT: I just realized I'm looking at it from my own perspective as a library author. The question from Sanctuary's perspective would be: "Would Sanctuary allowForeign by default?". If the answer is "yes", then we're back to sanctuary-js/sanctuary-def#77, if the answer is "no", then we haven't solved the problem. As it stands I think your proposed workaround (taking type-checking away from specific functions for the benefit of interoperability) is a better trade-off.

We decided the behaviour of require('sanctuary') should remain unchanged

It would, in my proposal. S.safe() would only exist to provide a customized environment (in the absence of mutation), and S is defined by S.safe(S.env).

[Mutation of environment] scares me, I must admit

Me too. And I think the ideas presented in sanctuary-js/sanctuary-def#74 are much more elegant. However, I think the complexity of wiring up the Sanctuary env is quite daunting and it keeps bothering me that library authors have to pass along the env if they wish to make meaningful use of Sanctuary. Though using mutation might give us more problems than it solves. :\

@Avaq
Copy link
Member

Avaq commented Jan 16, 2017

Would it be possible to reintroduce "allowing foreign types" as an option?

Actually, this might be a bad idea. It's going to enforce developers to "cut off access" to extending the internal env they are using in favor of a simple API. Disabling "power users" to extend the env with environments from other modules and have better type-checking.

@Avaq
Copy link
Member

Avaq commented Jan 16, 2017

So to conclude:

  • I have no better ideas than your proposed work-around
  • It's probably better to leave foreign types support out
  • The only question that remains is whether to expose a more convenient way to access unsafe Sanctuary functions, eg. S.unsafe.map. I think that should be possible, since unsafe functions don't need an env, nor to know whether to check types. Essentially we're adding a second preconfigured Sanctuary, the first being S itself.

@davidchambers
Copy link
Member Author

We decided the behaviour of require('sanctuary') should remain unchanged

It would, in my proposal. S.safe() would only exist to provide a customized environment (in the absence of mutation), and S is defined by S.safe(S.env).

Oh, I see. :)

The only question that remains is whether to expose a more convenient way to access unsafe Sanctuary functions, eg. S.unsafe.map. I think that should be possible, since unsafe functions don't need an env, nor to know whether to check types. Essentially we're adding a second preconfigured Sanctuary, the first being S itself.

I do like the idea of reinstating S.unchecked—I prefer "unchecked" to "unsafe"—for ergonomics:

const S = require('sanctuary').unchecked;
const S = require('sanctuary').create({checkTypes: false, env: []});

Were we to do so, the checkTypes member of the { checkTypes :: Boolean, env :: Array Type } record would become redundant, as you noted. S.create would only be necessary for creating type-checked modules with custom environments, so we could change its type to Array Type -> Module. There are two minor drawbacks of this approach: S.create would no longer mirror $.create, and switching based on process.env.NODE_ENV or similar would be slightly messier.

I'm keen to reinstate S.unchecked if you think it's a good idea. We may or may not decide to change S.create while we're at it.

@davidchambers
Copy link
Member Author

Using sanctuary-type-classes is absolutely the right approach. ADT libraries are foundational so should not depend on high-level libraries such as Sanctuary. Sanctuary is large and has many dependencies.

+---------------------------------------------------------+
|                                                         |
|                       application                       |
|                                                         |
+---------------------------------------------------------+

+---------------------------------------------------------+
|                        sanctuary                        |
+---------------------------------------------------------+

+---------------+   +-----------------+   +---------------+
| sanctuary-map |   | sanctuary-maybe |   | sanctuary-set |
+---------------+   +-----------------+   +---------------+

+---------------------------------------------------------+
|                 sanctuary-type-classes                  |
+---------------------------------------------------------+

+---------------------------------------------------------+
|                                                         |
|                                                         |
|        JavaScript's built-in types and functions        |
|                                                         |
|                                                         |
+---------------------------------------------------------+

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

No branches or pull requests

2 participants