ponyc icon indicating copy to clipboard operation
ponyc copied to clipboard

Default arguments aren't handled correctly on method calls of unions

Open jasoncarr0 opened this issue 2 years ago • 12 comments

If a method call is valid for all members of a union, it should be possible to call on the union. This isn't true if the methods have a different number of parameters due to optional parameters. For instance, calling .string() on a JsonType fails with an error:

Error:
/example/main.pony:12:12: a member of the union type has an incompatible method signature
        x.string()
           ^
    Info:
    /somelocation/lib/pony/0.42.0/packages/json/json_type.pony:97:3: methods have a different number of parameters
      fun string(indent: String = "", pretty_print: Bool = false): String =>
      ^
    /somelocation/lib/pony/0.42.0/packages/builtin/real.pony:711:3: methods have a different number of parameters
      fun string(): String iso^ =>

Even though the call .string() is valid on all members of the union

jasoncarr0 avatar Mar 26 '22 01:03 jasoncarr0

Can you give a minimal example?

SeanTAllen avatar Mar 26 '22 01:03 SeanTAllen

I think I understand what you are saying and I don't think the example I believe you are giving is an error. A minimal example would be good.

SeanTAllen avatar Mar 26 '22 01:03 SeanTAllen

Sure: https://playground.ponylang.io/?gist=d0c848aa83aeac85e93dfe5c11bd10ad

class A
  fun string(): String => "A"
class B
  fun string(excited: Bool = false): String =>
    "B" + (if excited then "!" else "" end)

actor Main
  new create(env: Env) =>
    // fine
    let a: A = A
    env.out.print(a.string())
    // fine
    let b: B = B
    env.out.print(b.string())
    // error!
    let ab: (A | B) = A
    env.out.print(ab.string())

jasoncarr0 avatar Mar 26 '22 01:03 jasoncarr0

So this works and I think rightly so:

class Foo
  fun string(x: String = "foo"): String iso^ =>
    x.clone()
    
class Bar
  fun string(x: String): String iso^ =>
    x.clone()
    
actor Main
  new create(env: Env) =>
    let x: (Foo | Bar) = Foo
    env.out.print(x.string("fred"))

and this doesn't:

class Foo
  fun string(x: String = "foo"): String iso^ =>
    x.clone()
    
class Bar
  fun string(): String iso^ =>
    "bar".clone()
    
actor Main
  new create(env: Env) =>
    let x: (Foo | Bar) = Foo
    env.out.print(x.string())

And I think also rightly so. One is string() the other is string(string) and those are not the same method even if string(string) has a default value.

SeanTAllen avatar Mar 26 '22 01:03 SeanTAllen

yeah @jasoncarr0 I don't think this is a bug. I think this is correct. A.string() and B.string() aren't same method. They have different signatures. I wouldn't consider those to have a subtype relationship.

SeanTAllen avatar Mar 26 '22 01:03 SeanTAllen

If it was just subtyping I would be more amenable to agree (but still strongly prefer allowing it). But here it's a simple method call so it's far more surprising that we need to do this:

match x
| let f: Foo => f.string()
| let b: Bar => b.string()
end

jasoncarr0 avatar Mar 26 '22 01:03 jasoncarr0

I'd say it's still odd that something which supports .string() isn't Stringable. But in any case we'll discuss in sync

jasoncarr0 avatar Mar 26 '22 01:03 jasoncarr0

It doesn't support string() it supports string(string).

When something with a default value is called, it is still string with 1 parameter. not string with no parameters. The sharing of a name does not make them the same. Return type needs to be the same, as does the number of parameters.

Here's example:

class Foo
  fun string(x: String = "foo"): String iso^ =>
    x.clone()
    
class Bar
  fun string(): String iso^ =>
    "bar".clone()
    
actor Main
  new create(env: Env) =>
    let x: (Foo | Bar) = Foo
    env.out.print(x.string("foo"))

Foo.string is a fundamentally different than Bar.string. I think no one would argue that the example above should compile.

SeanTAllen avatar Mar 26 '22 01:03 SeanTAllen

So it is again a default so really your example is:

match x
| let f: Foo => f.string("foo")
| let b: Bar => b.string()
end

Which I don't think is at all surprising. The existence of defaults do not change the method signature, they allow the writer to elide them from their code and have the compiler insert it for them.

SeanTAllen avatar Mar 26 '22 01:03 SeanTAllen

I would be very much in favor of enhancing the tutorial to explain that default values in methods do not change the signature.

SeanTAllen avatar Mar 26 '22 01:03 SeanTAllen

In the sync call, I agreed with Jason that conceptually the function with default arguments should be a subtype of the function without arguments. Sean does not agree.

Making it work as Jason and I expect is tricky, because default arguments are injected at the call site. We'd likely need to create a virtual table entry that takes zero arguments, but acts as a shim function that calls the real function and injects the default arguments within the virtual shim.

Then we'd need to work through updating the selector painting logic to correctly assign the right virtual table index for this case vs the concrete case without the shim.

So it would be non-trivial to make this work, and it's somewhat controversial at the moment (we don't yet agree that we'd like to implement this).

In the short term everyone agrees that we should update the Pony tutorial to document the limitation so that it doesn't catch people by surprise - it's just not agreed whether the limitation is a bug or an intentional limitation.

jemc avatar Apr 26 '22 18:04 jemc

Small update #4088 is non-controversial and requires the same work as this to make it work so if someone wants to take on this work, it will be accepted. The question will be "what are the resolution rules".

SeanTAllen avatar Oct 07 '23 20:10 SeanTAllen