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

Add trait support and its test. #188

Merged
merged 12 commits into from
Jul 24, 2015
Merged

Add trait support and its test. #188

merged 12 commits into from
Jul 24, 2015

Conversation

albertnetymk
Copy link
Contributor

No description provided.

@EliasC
Copy link
Contributor

EliasC commented Jul 14, 2015

I would like the following code to compile

trait Mappable<v>
  require value : v
  require next : Mappable<v>
  def map(f : v -> v) : void{
    this.value = f(this.value);
    if this.next != null
    then this.next.map(f)
  }

passive class List<v> : Mappable<v>
  value : v
  next : List<v>

  def init(value : v, next : List<v>) : void{
    this.value = value;
    this.next = next;
  }

  def add(value : v) : void
    if this.next == null
    then this.next = new List<v>(value, null)
    else this.next.add(value)

but the following code to fail

trait T
  require f : Foo
  def setF(f : Foo) : void
    this.f = f

passive class C : T
  f : C

The problem with the second example is that the trait T assigns to a field whose exact type is unknown, meaning it could write something that is not a C to the field, destroying type safety for other traits (or classes). We talked briefly about this yesterday, but I was not very clear I think. Any trait that writes to a field must see the same type as the class sees, but as long as the field is only read (including calling methods) there can be a subtype relation between the type that the trait and the class sees. You should be able to use filter from Util.hs to find all the field assignments easily.

@EliasC
Copy link
Contributor

EliasC commented Jul 14, 2015

There is a regression in the quality of general error messages. Some examples:
Before:

No method 'baz' in class 'C'

Class 'C<t>' expects 1 type parameters. 
Type 'C' has 0

Method 'foo' of class 'C' expects 0 arguments. Got 1

Now:

No methad 'baz' in class 'C'

'C' expects 0 type arguments, but 'C<int>' has 1

Method 'foo' expect 0 but got 1

(note misspelled "methad" in first example)

The middle one might not count as a regression, but that these error messages have changed at all makes me a bit worried that there are changes in this PR that changes other things than adding trait support.

@albertnetymk
Copy link
Contributor Author

Not very clear about the two snippets you posted above. It seems typechecking error to me. Added a commit to see if I understand it correctly. As for the error msg, some "refactoring" is done to accommodate traits, and it's likely that some visible effects are leaked out. Would address them in later commits.

@EliasC
Copy link
Contributor

EliasC commented Jul 14, 2015

Another name clash bug: There needs to be a check for traits and classes with the same name (otherwise there's an error from clang).

@EliasC
Copy link
Contributor

EliasC commented Jul 14, 2015

We've had this discussion before, but I don't like that there are some changes that do not follow the naming conventions of the file they're in. Mostly it's a question of camelCase vs. snake_case but not always. For example, in AST.hs the name of a class is accessed through the field cname, the name of a method through mname, the name of a function through funname, but the name of a trait through trait_name. In Environment.hs, a field is looked up using fieldLookup, but a trait method is looked up using trait_method_lookup. When writing code in these files, I don't want to have to think about which case to use when I'm using a name (just remembering all the names is hard enough).

@EliasC
Copy link
Contributor

EliasC commented Jul 14, 2015

A lot of nice refactorings in this commit, and the traits seem to be working like they should (at least as long as you use them like you're supposed to)! Will keep this conversation going as new commits are added. Let me know if you want me to fix something!

@EliasC
Copy link
Contributor

EliasC commented Jul 21, 2015

It's a bit late now, but our own guidelines actually says we should develop larger features in a feature branch, not directly in master. Let's remember this for the next larger addition to the language. Also, see the section on respecting conventions ;)

@EliasC
Copy link
Contributor

EliasC commented Jul 24, 2015

If the xLookup' are renamed to something that signals their unsafety (I vote for xLookupUnsafe) I'm (finally) ready to merge this PR. Any other discussed changes are subjective and can be postponed to later PRs.

EliasC added a commit that referenced this pull request Jul 24, 2015
Add trait support and its test.
@EliasC EliasC merged commit 0a65b67 into parapluu:master Jul 24, 2015
@albertnetymk albertnetymk deleted the trait branch July 27, 2015 12:02
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.

2 participants