-
-
Notifications
You must be signed in to change notification settings - Fork 415
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
Default arguments aren't handled correctly on method calls of unions #4073
Comments
Can you give a minimal example? |
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. |
Sure: https://playground.ponylang.io/?gist=d0c848aa83aeac85e93dfe5c11bd10ad
|
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. |
yeah @jasoncarr0 I don't think this is a bug. I think this is correct. |
If it was just subtyping I would be more amenable to agree (but still strongly prefer allowing it).
|
I'd say it's still odd that something which supports |
It doesn't support When something with a default value is called, it is still string with 1 parameter. not string with no 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. |
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. |
I would be very much in favor of enhancing the tutorial to explain that default values in methods do not change the signature. |
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. |
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". |
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:Even though the call .string() is valid on all members of the union
The text was updated successfully, but these errors were encountered: