Add method call info for infix operators #7090
Conversation
4e6
requested review from
jdunkerley,
radeusgd,
GregoryTravis,
JaroslavTulach,
hubertp and
Akirathan
as code owners
| moduleName = ensoRootNode.getModuleScope().getModule().getName(); | ||
| typeName = null; | ||
| functionName = rootNode.getName(); | ||
| case BuiltinRootNode builtinRootNode -> { |
There was a problem hiding this comment.
I am surprised the case for EnsoRootNode is removed and nothing gets broken!?
There was a problem hiding this comment.
EnsoRootNode does not contain enough information to build the method pointer. We can simply skip it
| @@ -1692,6 +1692,187 @@ class RuntimeServerTest | |||
| ) | |||
| } | |||
|
|
|||
| it should "send method pointer updates of a builtin method" in { | |||
There was a problem hiding this comment.
What is the builtin method? The + operator?
| @@ -12,6 +13,29 @@ protected BuiltinRootNode(EnsoLanguage language) { | |||
| super(language); | |||
| } | |||
|
|
|||
| protected QualifiedName moduleName = null; | |||
There was a problem hiding this comment.
Why protected? There is public getter, public setter. Usually such fields are made private then.
|
|
||
| /** Set the module name where the builtin is defined. */ | ||
| public void setModuleName(QualifiedName moduleName) { | ||
| this.moduleName = moduleName; |
There was a problem hiding this comment.
I don't like mutability of things that should be immutable. I'd prefer to remove the setters completely. If that is not possible, please throw exception if the value is already set and somebody wants to redefine it.
There was a problem hiding this comment.
Ok, I'll update the setters in the next PR
| val builtinRootNode = | ||
| m.getFunction.getCallTarget.getRootNode | ||
| .asInstanceOf[BuiltinRootNode] | ||
| builtinRootNode.setModuleName(moduleScope.getModule.getName) |
There was a problem hiding this comment.
Can't this information be passed to the factory that creates the node?
There was a problem hiding this comment.
There's not enough information on that stage. The BuiltinMethod annotation contains an unqualified method name. There is also no information on whether the method is static or not (to derive the self-type). Although the BuiltinType annotation contains a qualified type name. But I'm not sure if we can correctly match builtin methods and types because there's not always a 1-1 correspondence (i.e. like with numbers subtypes)
There was a problem hiding this comment.
CCing @hubertp as the creator of these annotations and author of Builtins::readBuiltinTypes and Buitins::readBuiltinMethodsMethods.
I see that BuiltinTypes.metadata contains
Date_Time:org.enso.interpreter.node.expression.builtin.date.DateTime:Standard.Base.Data.Time.Date_Time.Date_Time:EnsoDateTime
which represents:
- Enso type name
- fully qualified Java class name
- fully qualified name as available in stdlib
- underlying type name of the builtin
then I also see that BuiltinMethods.metadata contains:
Date.month:org.enso.interpreter.node.expression.builtin.date.MonthDateMethodGen:false:true
which represents:
- the language-level name of this method
- fully qualified Java class name
- is static information
- is autoregistered information
not enough information on that stage
Given the combined content of the BuiltinMethods.metadata and BuiltinTypes.metadata I am a believer in having all (most of) the needed information already available...
no information on whether the method is static or not
...that information seems to be present in BuiltinMethods.metadata.
Should any information be missing in BuiltinMethods.metadata and BuiltinTypes.metadata files, then I'd rather add such information there than trying to supply it later via setters in IrToTruffle stage. Immutability creates better systems that are easier to reason about. Having half-initialized objects floating around the system is just asking for troubles.
Please accept my apology for not providing this review sooner than this PR got merged.
There was a problem hiding this comment.
I'm not sure if this will cover static wrappers. But yes, in general that information is ready in the metadata files that are being read in static initializers.
There was a problem hiding this comment.
So if I understand that correctly, instead of providing the moduleName and typeName here via setters on BuiltinRootNode, you are suggesting to tweak Builtins$LoadedBuiltinMethod::toFunction method (and any other affected locations in the code) such that it will create a BuiltinRootNode with already initialized moduleName and typeName? That sounds like a good idea.
Immutability creates better systems that are easier to reason about.
Indeed. Let me also add a note that setters are twice as much problematic in the Truffle environment than in pure Java. What if BuiltinRootNode.setModuleName is used during runtime? Should we manually invalidate any assumptions? Is it safe not to call CompilerDirectives.transferToInterpreterAndInvalidate() in these setters? Should the moduleName and typeName fields be marked as @CompilationFinal? (They probably should, theoretically, BuiltinRootNode can be shared among multiple contexts and can be a compilation constant; therefore, these fields can also be compilation constants). etc ...
TL;DR; I agree with Jaroslav, and I favor a follow-up PR that removes these setters (or tracking it in a separate issue). The sooner we do it, the better.
Pull Request Description
close #6374
In order to provide the method pointer information, the
IrToTrufflepass sets the module name and the type name of the builtin node.Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.