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

Implemented support for requiring methods #281

Merged
merged 3 commits into from
Dec 9, 2015
Merged

Implemented support for requiring methods #281

merged 3 commits into from
Dec 9, 2015

Conversation

EliasC
Copy link
Contributor

@EliasC EliasC commented Nov 16, 2015

A trait can now require methods just as they can require fields. The
syntax is like that of normal method definition but with require
instead of def:

trait T
  require id(x : int) : int

  def foo(x : int) : int
    this.id(x)

class C : T
  def id(x : int) : int
    x

Required methods are covariant in their return types (the actual method
can have a return type that is more specific than the required method)
and invariant in their parameter types, following Java interfaces.

This commit also includes a refactoring to unify the concept of a
function header. Functions, methods, stream methods and required methods
all share the same kind of header containing their names, parameters and
return types. This reduces a duplication but also has the effect of
appearing as many changed lines.

@EliasC
Copy link
Contributor Author

EliasC commented Nov 24, 2015

Rebased and ready for review!

@EliasC
Copy link
Contributor Author

EliasC commented Nov 24, 2015

Rebased (again) and ready for review (again)!

@albertnetymk
Copy link
Contributor

The insertion in CCodeNames.hs cuts into an existing comment and function pair. After pondering on it for a while, it seems to make sense with method_id naming, for it has nothing to do with msg. The original comment has rotted, IMO.

Why keeping the commented code in ClassDecl.hs?

The FunctionHeader declaration in AST.hs is vulnerable to indentation change caused by refactoring, so are some other datas.

@EliasC
Copy link
Contributor Author

EliasC commented Nov 27, 2015

The insertion in CCodeNames.hs cuts into an existing comment and function pair. After pondering on it for a while, it seems to make sense with method_id naming, for it has nothing to do with msg. The original comment has rotted, IMO.

Fixed!

Why keeping the commented code in ClassDecl.hs?

Not my intention, fixed now!

The FunctionHeader declaration in AST.hs is vulnerable to indentation change caused by refactoring, so are some other datas.

Good catch! Fixed!

A trait can now require methods just as they can require fields. The
syntax is like that of normal method definition but with `require`
instead of `def`:

```
trait T
  require id(x : int) : int

  def foo(x : int) : int
    this.id(x)

class C : T
  def id(x : int) : int
    x
```

Required methods are covariant in their return types (the actual method
can have a return type that is more specific than the required method)
and invariant in their parameter types, following Java interfaces.

This commit also includes a refactoring to unify the concept of a
function header. Functions, methods, stream methods and required methods
all share the same kind of header containing their names, parameters and
return types. This reduces a duplication but also has the effect of
appearing as many changed lines.
@albertnetymk
Copy link
Contributor

Since you are modifying them, it's good to fix the indentation for Requirement, MethodDecl.

The single commit contains style change (aligning =), method renaming (mname -> methodName), and the real changes. Breaking them into separate commit would have made the review much easier, IMO.

@EliasC
Copy link
Contributor Author

EliasC commented Dec 7, 2015

Since you are modifying them, it's good to fix the indentation for Requirement, MethodDecl.

Done!

The single commit contains style change (aligning =), method renaming (mname -> methodName), and the real changes. Breaking them into separate commit would have made the review much easier, IMO.

I tried to only do aligning in places where I was already doing changes. The renaming is part of the real changes (mname is a record selector, methodName is a function that gets the name from the header of a method) so that could not be separated. Will try to think about it next time!

@albertnetymk
Copy link
Contributor

What's the intended use case for extractTypes? I can't find where it's used.

I think printf is more readable than concat for the case in TypeError.hs.

There's no doc for co/contra variance for required methods other than the description in this PR.

@EliasC
Copy link
Contributor Author

EliasC commented Dec 9, 2015

What's the intended use case for extractTypes? I can't find where it's used.

I can't find anywhere it is used either (it's not new for this PR, but has been around for a long time). I have som vague recollection that it had something to do with the old implementation of closures. It seems like it is not strictly needed right now.

I think printf is more readable than concat for the case in TypeError.hs.

You were the one who started using concat for error messages :)

I've never had a problem with "Expression '" ++ show (ppExpr e) ++ "' of type '" ++ show t ++ "' is awesome", but I guess using printf would also work.

There's no doc for co/contra variance for required methods other than the description in this PR.

Ah, documentation, my Achilles heel! I'll add this now!

@albertnetymk
Copy link
Contributor

I think it should be removed, then. This or some other PRs.

Indeed, originally, it's using ++, changed to concat (which is arguably better), due to my lack of knowledge of printf.

I think this PR is OK, if the author doesn't intend to address the previous points.

Another note on the requires keywords for methods; since methods in interface/trait without a body are required implicitly, having the requires keyword is redundant. Surely, this is very subjective.

@EliasC
Copy link
Contributor Author

EliasC commented Dec 9, 2015

I found this:

bash-3.2$ git log -SextractTypes
commit b50b3608a742e438ab412440dffd3c0275804519
Author: EliasC <elias.castegren@gmail.com>
Date:   Mon Jun 9 16:27:39 2014 +0200

    A utitlity function for extracting all the types of an AST

    Written for a scrapped closure implementation, but might come in handy
    some day...

Since it hasn't been handy for a year, I also think it can be removed (especially since it's saved in the Git history). I don't think this PR should remove it though.

Another note on the requires keywords for methods; since methods in interface/trait without a body are required implicitly, having the requires keyword is redundant. Surely, this is very subjective.

This is mostly to make parsing simpler, but it also makes some kind of sense since def is used to define a method, and require to require it.

Unless other bugs are uncovered I do not plan to change anything else in this PR.

@albertnetymk
Copy link
Contributor

OK, merge in an hour, if no objections.

albertnetymk added a commit that referenced this pull request Dec 9, 2015
Implemented support for requiring methods
@albertnetymk albertnetymk merged commit 28199f2 into parapluu:features/plenary Dec 9, 2015
@EliasC EliasC deleted the features/require-methods branch December 21, 2016 10:45
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.

3 participants