-
Notifications
You must be signed in to change notification settings - Fork 334
Fix special handling of static method calls on Any #13939
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
base: develop
Are you sure you want to change the base?
Fix special handling of static method calls on Any #13939
Conversation
Request for comments from @AdRiley and @JaroslavTulach : Do you agree with the new semantics described in 38df5ef? |
``` | ||
|
||
the method call `obj.method` is equivalent to `T.method obj`, which is | ||
equivalent to `T.method self=obj`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two things to consider when invoking a method in Enso:
- lookup of the method
- passing of
self
(and maybeSelf
) argument
The lookup is always done by name in some Map<String, Function>
"pool". The number of pools, their inheritance is not likely to change by this or #11686 changes.
I like the idea of defaulted/implicit self argument. It should allow us to make sure that all methods/functions in the pools are always of following FunctionSchema
:
fn self[=T] arg1[=def1] arg2[=def2] arg3[=def3] ...
- if
fn
is an instance method, then the first argument isself
and it doesn't have default value - if
fn
is a static method, then the first argument isself
and it has default value set to the defining typeT
By adhering to this logic, we avoid the duplication of functions (described in #11686) - each function will be defined just once. It will be looked up and based on the type of invocation either implicit self=obj
will be pre-applied or not.
PS: Sounds reasonable, but ... it would be good to formalize it with some strict apparatus, otherwise it is clear we'll miss some nuances (as usually).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with more formalization, but do you have any suggestions on how such "strict apparatus" may look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ask the public... meanwhile we should rely on simple, well organized tests. This is my current test case:
import Standard.Base.Any.Any
import Standard.Base.Panic.Panic
type Single
popis self = "Jsem single"
type Val
private Value x
popis self = "Jsem to "+self.x
Any.popis self = "Kdokoli"
test_instance val:Val =
i1 = Single.popis # yields Kdokoli WRONG: #13892
i2 = Val.popis # yields Kdokoli
i3 = val . popis # yields Jsem to já
[ i1, i2, i3 ]
test_static val:Val =
s1 = Any.popis val # yields Kdokoli
s2 = Any.popis self=val # yields Kdokoli
s3 = Panic.recover Any <| Val.popis val # errors with Not_Invokable.Error 'Kdokoli' WRONG!?
s4 = Panic.recover Any <| Val.popis self=val # errors with Not_Invokable.Error 'Kdokoli' WRONG!?
[ s1, s2, s3, s4 ]
main =
val = Val.Value "já"
t1 = test_instance val
t2 = test_static val
[t1, t2]
there are two errors as far as I can say. Are there any tests related to this instance/static behavior already? Are are they runtime-integration-tests
? Can we group them next to each other, add this new one and constantly add new and new?
docs/types/dynamic-dispatch.md
Outdated
algorithm can be generally described as follows: | ||
|
||
- Is the method called statically? For example like `Any.method ...` or | ||
`T.method ...`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does one find out that the "method is called statically"? By receiver being a Type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Added this sentence to docs in e426eaf
Fixes #13892
Pull Request Description
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.