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

Fix the inconsistent type of "env" #123

Closed
Avaq opened this issue Feb 23, 2017 · 14 comments
Closed

Fix the inconsistent type of "env" #123

Avaq opened this issue Feb 23, 2017 · 14 comments

Comments

@Avaq
Copy link
Member

Avaq commented Feb 23, 2017

@Avaq
Copy link
Member Author

Avaq commented Feb 23, 2017

One of the ideas was to replace all type constructors (Type... -> Type) in env with types that have already been applied with Unknowns, eg: env.concat([Maybe(Unknown), Either(Unknown, Unknown)]).

My concern is that currently, type constructors might actually be used by sanctuary-def in order to create concrete types for values with variable types, otherwise, what's the value of having these type constructors in the environment in the first place?

@davidchambers
Copy link
Member

Thanks for opening this issue, Aldwin.

otherwise, what's the value of having these type constructors in the environment in the first place?

So that values such as [1, 2, 3] and Just('foo') are recognized. For example:

const a = $.TypeVariable('a');

$.create({checkTypes: true, env: [$.Number]})('id', {}, [a, a], x => x)([1, 2, 3]);
// ! TypeError: Type-variable constraint violation
//
//   id :: a -> a
//         ^
//         1
//
//   1)  [1, 2, 3] ::
//
//   Since there is no type of which all the above values are members, the type-variable constraint has been violated.

@Avaq
Copy link
Member Author

Avaq commented Feb 23, 2017

So that values such as [1, 2, 3] and Just('foo') are recognized

But wouldn't adding Maybe(Unknown) to the env instead of Maybe make the type-checker think that Just(1) and Just('foo') are consistent? Or is some other mechanism used to determine consistency over multiple occurrences of the same type-var?

@davidchambers
Copy link
Member

$.Unknown is special. Search the code for UNKNOWN and you'll find several occurrences.

$.Unknown is used internally when the actual type cannot be completely determined. The type of [], for example, is Array Unknown. $.Array($.Unknown) constrains the outer type without constraining the inner type.

$.Unknown is a bit like a stem cell. :)

@Avaq
Copy link
Member Author

Avaq commented Feb 23, 2017

Oh, hold on a second! The type constructors are applied to Unknown the moment they are passed into Sanctuary Def!

That clears everything up. Sanctuary Def simply doesn't use them.

@davidchambers
Copy link
Member

Yes. What I'm suggesting is that it should be the caller's responsibility to do something like S.EitherType($.Unknown, $.Unknown) rather than for sanctuary-def to take care of it.

@Avaq
Copy link
Member Author

Avaq commented Feb 23, 2017

Totally get that now. This change wouldn't even impact the user much, as most of the changes would happen in the internals. It would be a major version for sanctuary-def, but a minor or even patch for sanctuary.

Come to think of it. It would be a patch for sanctuary-def as well, if we wouldn't publish #121!

@Avaq
Copy link
Member Author

Avaq commented Feb 23, 2017

We would simply remove the applyParameterizedTypes line from test, which makes it compatible with the signature originally assigned to it before #121. Nothing changes on the surface.

@Avaq
Copy link
Member Author

Avaq commented Feb 23, 2017

And then it's just S.env that needs a quick patch.

@davidchambers
Copy link
Member

Okay. Let's remove applyParameterizedTypes, release sanctuary-def@1.0.0, then open a Sanctuary pull request to update its sanctuary-def dependency and S.env. Does this sound good?

@Avaq
Copy link
Member Author

Avaq commented Feb 23, 2017

Sounds good. I suppose we can close #121 then. Haha, my eagerness to get a patch out actually caused me to have to wait longer.

@Avaq
Copy link
Member Author

Avaq commented Feb 23, 2017

Oh wait. If you're going to release this as major, we may as well merge #121 so that people will not get 🐛 bitten in the current version. I don't know.

@davidchambers
Copy link
Member

Oh wait. If you're going to release this as major, we may as well merge #121 so that people will not get 🐛 bitten in the current version.

This seems reasonable.

I'm about to put my head down. I'll come back to these issues and pull requests on Saturday. :)

@Avaq
Copy link
Member Author

Avaq commented Feb 24, 2017

After collecting my thoughts, this is what I think we should do:

  • Patch sanctuary to export an env which is strictly Array Type, immediately fixing the issue at hand.
  • Close Correct type of $.test #121
  • Open PRs in sanctuary-def to:
    1. Remove the unnecessary call to applyParameterizedTypes
    2. Document how to apply parameterized types in order to create a valid env
  • Possibly release the aforementioned changes to sanctuary-def under a new major version. There's no hurry anymore since we patched the problem in sanctuary.

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