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 #311 #312

Merged
merged 1 commit into from
Dec 21, 2015
Merged

Fix #311 #312

merged 1 commit into from
Dec 21, 2015

Conversation

EliasC
Copy link
Contributor

@EliasC EliasC commented Dec 19, 2015

Fixed broken coercion that would lose type parameters when casting:

trait T

passive class C<c> : T

class Main
  def main() : void
    let x = new C<int> : T in
      ()

@EliasC
Copy link
Contributor Author

EliasC commented Dec 19, 2015

We should probably go over the whole typechecker and make sure we're not doing any double work (right now the string "does not match expect type" appears nine times in Typechecker.hs, suggesting that we are duplicating work). There are also many different things going on that I think could be streamlined, like matching arguments, matching types, checking subtyping, coercing null and coercing other things.

@albertnetymk
Copy link
Contributor

In what condition would the first guard of coerce be hit, a snippet?

@EliasC
Copy link
Contributor Author

EliasC commented Dec 20, 2015

@albertnetymk: The simplest case I could think of is

class C
class Main
  def main() : void
    new C : C -- Needs to check if 'new C' is a 'C'. 

Coercion is in practice used to figure out the type of Nothing I believe. Nothing has the type Maybe Bottom, where Bottom is the type that no value has. When type casting it, Bottom is coerced into the type of the cast:

Nothing : Just int -- Maybe Bottom --> Maybe int

Not sure if it is currently possible to construct an example where the type parameter of a ref type is unknown and needs to be coerced, but if we had type inference of constructors one could imagine a

class C<t>
  f : t
  def init(x : t) : void
    this.f = x

that was instantiated as

new C(Nothing)

Here, we cannot know what t should be (we would only be able to infer C<Maybe Bottom>, so we would need to add a cast

new C(Nothing) : C<Maybe int>

which in the first case of coerce would see that C and C are both class types, meaning that their type parameters also need to match. The zipWithM coerce would then coerce Maybe Bottom into Maybe int, making the resulting type valid.

@albertnetymk
Copy link
Contributor

Wouldn't new C(Nothing) causes an error like C expects 1 type var, but found zero?

@supercooldave
Copy link

The whole idea of using Nothing to do coercion baffles me – I'll put it in the list of big fixes need.

@EliasC
Copy link
Contributor Author

EliasC commented Dec 21, 2015

@albertnetymk

Wouldn't new C(Nothing) causes an error like C expects 1 type var, but found zero?

Not sure if it is currently possible to construct an example where the type parameter of a ref type is unknown and needs to be coerced, but if we had type inference of constructors one could imagine...

@EliasC
Copy link
Contributor Author

EliasC commented Dec 21, 2015

@supercooldave

The whole idea of using Nothing to do coercion baffles me

I'm not sure what you mean by "using Nothing". Nothing on its own has the type Maybe Bottom which needs to be coerced, i.e. implicitly cast to a different type, to make the compiler happy in certain cases. For type checking alone, it would often be enough to say "Maybe Bottom is a subtype of Maybe Foo, so this expression is valid", but for code generation to work, the AST-node generally needs to have the "correct" type (i.e. not Bottom), hence the coercion.

With that said, I still think the type checker would benefit from a do-over.

@supercooldave
Copy link

I mean all that coercion-bottom stuff.

@albertnetymk
Copy link
Contributor

OK, IOW, it's impossible to hit the first clause currently, then.

Assuming we had constructor type inference, why is the isClassType && isTraitType case ruled out by the first guard?

passive class C<t> : T<t>

...
  new C(Nothing) : T<Maybe int>

@EliasC
Copy link
Contributor Author

EliasC commented Dec 21, 2015

OK, IOW, it's impossible to hit the first clause currently, then.

No, see my first example (new C : C). It just doesn't do anything useful (as far as I can see) at the moment.

Assuming we had constructor type inference, why is the isClassType && isTraitType case ruled out by the first guard?

It's not. Your example wouldn't compile (although it should). But rather than trying to tack that case on (which doesn't have a use case yet), I would rather see a complete revamp of the type checking procedure.

@albertnetymk
Copy link
Contributor

Yes, you are right. I forgot the new C : C case.

The first clause of coerce should deal with but not limited to cases listed below:

new C(Nothing) : C<Maybe int>
new C(Nothing) : T<Maybe int>

so that type inference could exploit the type annotation. Currently, both the guard and body of the first clause fail to meet the requirement, and fixing them completely probably stipulates a full-scale improvement in TC, so maybe it's best to remove the first clause totally to avoid false sense of security and get fast failure if at all.

@EliasC
Copy link
Contributor Author

EliasC commented Dec 21, 2015

maybe it's best to remove the first clause totally to avoid false sense of security and get fast failure if at all.

Agreed! Pushed now

@albertnetymk
Copy link
Contributor

In spite of my lobbying effort, there's still +2 changed made it into the repo. Merging after lunch, if no objections.

Fixed broken coercion that would lose type parameters when casting:
```
trait T

passive class C<c> : T

class Main
  def main() : void
    let x = new C<int> : T in
      ()
```
albertnetymk added a commit that referenced this pull request Dec 21, 2015
@albertnetymk albertnetymk merged commit 1724b5b into parapluu:development Dec 21, 2015
@kikofernandez kikofernandez deleted the fix/typecasting branch December 21, 2015 17:19
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.

4 participants