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

Update inheritance.md - covariance and contravariance #276

Merged
merged 2 commits into from Aug 13, 2018

Conversation

KCErb
Copy link
Contributor

@KCErb KCErb commented Aug 9, 2018

Not sure about linking to an issue in the gitbook ... but otherwise, how's this for a crack at crystal-lang/crystal#6507 (comment)?


## Covariance and Contravariance

One place this can get a little tricky is with arrays. We have to be careful when declaring an array of objects where inheritance is used. For example, consider the following
Copy link
Contributor

Choose a reason for hiding this comment

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

please be specific about what "this" is.

@KCErb
Copy link
Contributor Author

KCErb commented Aug 11, 2018

Thanks @RX14 for the review. I updated this to inheritance.

@RX14 RX14 requested a review from bcardiff August 12, 2018 19:59
@mverzilli mverzilli merged commit 2560fb2 into crystal-lang:master Aug 13, 2018
@mverzilli
Copy link

Thank you @KCErb!

@KCErb KCErb deleted the patch-1 branch August 13, 2018 13:46
Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

Late review into the party 🎉

@KCErb let me know what you think about the review and if you are interested in making further changes in this section.

Thanks for pushing this into the crystal-book.

end
```

because in the initialize the default type for @arr is `Array(Bar)` but the required type is `Array(Foo)`. You can solve this by specifying the type explicitly:
Copy link
Member

Choose a reason for hiding this comment

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

default type for @arr

This is not concept of the type system. I would suggest explaining that the type of the [Bar.new] expression is Array(Bar) and that is not assignable to an Array(Foo) instance var.

Then it follows from the previous example why the expression [Bar.new] of Foo works.

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, I can see how my language is a bit confusing / imprecise here. I'll reword a bit and see what you think.

end
```

The way Crystal handles the bigger topic of [covariance and contravariance](https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)) in general, has more tricks and pitfalls to it, so you may be interested in [this issue / discussion](https://github.com/crystal-lang/crystal/issues/3803) for more reading.
Copy link
Member

Choose a reason for hiding this comment

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

I would not link issues from the repo. It does not help to read the guide as a book.

Also,

has more tricks and pitfalls to it
adds no explanation or description current limitations.

I know the Covariance and Contravariance is not fully supported since the user can't express fully it's expectation in generic classes, nor the compiler infers what happens. Given that, maybe examples should cover: tuples (or named tuples), arrays, custom generic classes.

Another aspect mentioned is that method matching does not uses the notion of substitutability to determine if a definition should be used or not. In this PR only assignments were used, hence substitutability principle is used.

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'd be happy to remove this link, seems a little strange to me. :)

As for some of the examples you mentioned, is this the best place or is it potentially growing the article too much? If we really want to address covariance and contravariance as a topic in the gitbook, I might not be the best person, it's a little over my head at the moment (though I could catch up with a bit of reading it seems).

I can think of a few roads:

  1. Small: just remove the link and the stuff about "pitfalls" to keep it to one simple example of an array and link to the wikipedia article.
  2. Medium: add in an example using a Tuple here or put one on Generics over in the generics inheritance section and link to it.
  3. Large: consider doing a separate page on the topic of covariance and contravariance to fully document the expected behavior in a wide range of cases, link to that in places that mention inheritance such as this article or the one I just mentioned on generics.

It's only that third option that I wouldn't feel totally qualified to write up properly. Which direction do you (folks) lean?

@KCErb
Copy link
Contributor Author

KCErb commented Aug 13, 2018

Sure I'd be happy to work on this a bit more. Would it be better to restore the branch and push more changes to it or make a new branch?

@bcardiff
Copy link
Member

Great! Thanks. Opening a new PR is fine. This PR is an improvement by itself :-)

@KCErb
Copy link
Contributor Author

KCErb commented Aug 13, 2018

OK cool, will do next chance

KCErb added a commit to KCErb/crystal-book that referenced this pull request Sep 27, 2018
bcardiff pushed a commit that referenced this pull request Oct 2, 2019
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.

None yet

4 participants