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

Remove no part checked. Fixes #303. #311

Closed
wants to merge 8 commits into from

Conversation

CrossEye
Copy link

This avoids the "No parts of should be checked" from the individual rules and adds a forAll intro section to explain what that means.

It is done using forall annotations on signatures. Could someone with deeper knowledge of H-M types double-check that I added it correctly?

@davidchambers
Copy link
Member

This is nice!

README.md Outdated

## `forAll`

[Parametric polymorphism][pp] is essential to these algebras. When the rules for any type are specified as applied to all values (`forall a b`) of the given generic types (here `a` and `b`), the rule must hold for any Javascript values unless otherwise specified. This implies, for instance, that code that behaves differently for the `null` or `undefined` value than for other values is unlikely to comply with the rule.
Copy link
Member

Choose a reason for hiding this comment

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

Could you wrap this paragraph at 79 characters?

Copy link
Member

Choose a reason for hiding this comment

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

Please change “Javascript” to “JavaScript” for consistency with the rest of the document.

Copy link
Author

Choose a reason for hiding this comment

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

Done and done.

README.md Outdated
@@ -363,7 +371,7 @@ method takes one argument:
#### `map` method

```hs
map :: Functor f => f a ~> (a -> b) -> f b
map :: forall a b . Functor f => f a ~> (a -> b) -> f b
Copy link
Member

Choose a reason for hiding this comment

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

PureScript appears not to include a space before the .. I suggest we follow suit.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

(Space is allowed in PS before the dot btw)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, allowed in Haskell as well, at least in some example code, but it's much more rare, it seems.

@@ -159,6 +159,14 @@ Identity type, for example, could provide `Id` as its type representative:
If a type provides a type representative, each member of the type must have
a `constructor` property which is a reference to the type representative.


## `forAll`
Copy link
Member

Choose a reason for hiding this comment

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

Might “Parametricity” be a better heading?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so. If someone doesn't understand this signature because of the forall clause:

map :: forall a b. Functor f => f a ~> (a -> b) -> f b

and tries to figure out what it is, a header that matches the confusing keyword is better than a term that -- given this confusion -- is likely unknown to them also.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Shall we use forall rather than forAll, though?

Copy link
Author

Choose a reason for hiding this comment

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

Absolutely. I'll fix that tonight.


[Parametric polymorphism][pp] is essential to these algebras. When the rules
for any type are specified as applied to all values (`forall a b`) of the
given generic types (here `a` and `b`), the rule must hold for any JavaScript
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering how can we make restrictions polymorphism implies more clear here.
What you think on something like this:(bold is for highlighting the addition):

Parametric polymorphism is essential to these algebras. When the rules
for any type are specified as applied to all values (forall a b) of the
given generic types (here a and b), the rule must hold for any possible
JavaScript value unless otherwise specified.
Since a parametrically polymorphic value does not "know" anything about the
type variables, it must behave the same regardless of its type. This is a somewhat
limiting but extremely useful property known as parametricity.
This implies, that a function should behave in same way for all values,
i.e. inspection of value is not allowed.
for instance, that code that
behaves differently for the null or undefined value than for other
values is unlikely to comply complying with the rule.

we can steal some ideas from https://wiki.haskell.org/Polymorphism or/and link to it too (has better explanation then wikipedia)

Copy link
Author

Choose a reason for hiding this comment

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

I would like to expand this section. These are good ideas. The "unless otherwise specified" has to do with things like f must be a binary function or b must be same Apply as a. In such cases, not every value is allowable.

"is unlikely to comply" was about examples such as return r === null ? Maybe.of(r) : Maybe.of(r). The only way these examples could work is if their differences are not observable, so perhaps we can skip it. But it was part of the motivation for this change.

@@ -552,11 +559,7 @@ method takes two arguments:
1. if `f` is not a function, the behaviour of `reduce` is unspecified.
2. The first argument to `f` must be the same type as `x`.
3. `f` must return a value of the same type as `x`.
4. No parts of `f`'s return value should be checked.

1. `x` is the initial accumulator value for the reduction
Copy link
Member

Choose a reason for hiding this comment

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

why remove this line?

Copy link
Author

Choose a reason for hiding this comment

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

I have no problem putting it back. I just didn't see it adding anything. Is there something that makes it clear that an "empty" foldable will return this value? Do you think that this text captures that?

Copy link
Member

@davidchambers davidchambers left a comment

Choose a reason for hiding this comment

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

Several type signatures are missing forall. Let's use forall everywhere it applies, not just for the methods whose descriptions currently include “no parts of […] should be checked”.

@joneshf
Copy link
Member

joneshf commented Feb 19, 2019

Since we were writing Haskell-esque type signatures, we were also inheriting Haskell's implicit forall for all type variables. I.e. it was already implied that things were parametrically polymorphic. The bit about no values being checked was still added in even though the signature explained that part. And we still get implementations that are not law abiding. The fact that we've moved to explicit forall doesn't seem like enough of a justification to me to also remove the bit about no values being checked. Adding an explicit forall is good because it is acting like a springboard to explain parametric polymorphism!

I'm not going to veto this PR, I think people making changes is good and I support it. But, I don't think this solves the underlying problem of people writing unlawful implementations.

Also, if we're now writing PureScript-esque type signatures (or Haskell-esque with GHC's ScopedTypeVariables extension), then we need to be consistent. All of the type variables should be introduced in the forall. E.g. map should be:

map :: forall a b f. Functor f => f a ~> (a -> b) -> f b

not

map :: forall a b. Functor f => f a ~> (a -> b) -> f b

Being inconsistent will lead to further explanations needed about how GHC's ScopedTypeVariables extension actually works and that's far off the beaten path of this spec.

@CrossEye
Copy link
Author

@joneshf:

The goal was not to add forall. It was explicitly to remove "no values should be checked". (See #303 for details.) Adding forall seemed to be a compromise for those cases. I have no problem removing forall everywhere and just depending upon an expanded section on parametricity. I actually think that would be better.

@CrossEye
Copy link
Author

@davidchambers: Rather than adding forall to the other signatures, let's look at removing all of them, and simply depending upon a parametricity section. I'll try to create a separate PR tonight or tomorrow.

@davidchambers
Copy link
Member

@CrossEye, if you don't intend to update this pull request, shall we close it?

@CrossEye CrossEye closed this Mar 2, 2019
@CrossEye
Copy link
Author

CrossEye commented Mar 2, 2019

I intend to create a replacement, but life has gotten in the way. Hopefully soon.

@puffnfresh
Copy link
Member

Add forall to everything

@xgbuils
Copy link
Contributor

xgbuils commented Mar 6, 2019

After @joneshf said that:

Since we were writing Haskell-esque type signatures, we were also inheriting Haskell's implicit forall for all type variables.

What 's the reason to add forall to everything? In my opinion, it generates noise in the law definitions. A section which explains that all laws are parametrically polymorphic is enough for me.

I think we have to get an agreement in #303 before doing the PR. Because it seems that no part to be checked is too much opened to interpretations:

  1. Does it mean parametric polymorphism?
  2. Does it mean inspection?
  3. Does it mean inspection with observable effect?

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.

6 participants