Meta.get_annotation and extension methods #7000
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| package org.enso.interpreter.node.expression.builtin.meta; | ||
|
|
||
| import com.oracle.truffle.api.nodes.Node; | ||
| import org.enso.interpreter.dsl.AcceptsError; | ||
| import org.enso.interpreter.dsl.BuiltinMethod; | ||
| import org.enso.interpreter.runtime.EnsoContext; | ||
| import org.enso.interpreter.runtime.callable.atom.Atom; | ||
| import org.enso.interpreter.runtime.callable.atom.AtomConstructor; | ||
| import org.enso.interpreter.runtime.data.Array; | ||
| import org.enso.interpreter.runtime.type.TypesGen; | ||
|
|
||
| @BuiltinMethod( | ||
| type = "Meta", | ||
| name = "meta_builtin", | ||
| description = "Find the type and constructs a Meta instances.", | ||
| autoRegister = false) | ||
| public class MetaBuiltinNode extends Node { | ||
| Object execute(long index, @AcceptsError Object value, Array factories) { | ||
| if (index < 0) { | ||
| index = findIndex(value); | ||
| } | ||
| if (index < 0) { | ||
| return -index; | ||
| } | ||
| var factory = (AtomConstructor) factories.getItems()[(int) index]; | ||
| return factory.newInstance(value); | ||
| } | ||
|
|
||
| private int findIndex(Object value) { | ||
| if (TypesGen.isFunction(value)) { | ||
| return 0; | ||
| } | ||
| if (value instanceof Atom) { | ||
| return 1; | ||
| } | ||
| if (TypesGen.isAtomConstructor(value)) { | ||
| return 2; | ||
| } | ||
| if (TypesGen.isUnresolvedSymbol(value) || TypesGen.isUnresolvedConversion(value)) { | ||
| return 4; | ||
| } | ||
| if (TypesGen.isDataflowError(value)) { | ||
| return -5; | ||
|
There was a problem hiding this comment. These indices look rather magical, I think we should have some comment explaining where they come from. Why is the dataflow index the only negative one? There was a problem hiding this comment. This is the relevant commit to read: 133ba8d There was a problem hiding this comment. I think it would be good to have some comment explaining this logic, as it's not that easy to understand it. There was a problem hiding this comment. What is the result of IIRC it used to be |
||
| } | ||
| if (TypesGen.isType(value)) { | ||
| return 6; | ||
| } | ||
| var ctx = EnsoContext.get(this); | ||
| if (ctx.getEnvironment().isHostObject(value)) { | ||
| return 3; | ||
| } | ||
| // primitive value | ||
| return 7; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 many
typeinMeta- why don't we have a singleMetatype and many constructorsAtom,Constructor,Primitive, etc.? Is this just a heritage or is there a benefit of doing so?Vote +1 for me to attempt to rewrite to single
Metatype and constructors. Vote -1 for keeping status quo as much as possible. CCing @jdunkerley, @GregoryTravis, @radeusgdThere 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'm not strongly attached to either way, although I think the current state of things is fine.
I think the reason could have been that each 'variant' has a different set of operations available on it - an
Atommay have afieldsfield, but this does not make sense for aFunctionorPrimitive. Ofc we can just throw 'Illegal_State' for such wrong calls but IMO this does not look good - it's better when the API of each 'variant' is visible from its type not from for what it works and for what it throws.On the other hand, I'd appreciate some 'common parent type' for all these, but without class hierarchies / interfaces, I don't think we can have the cake (common type) and eat it too (clear, variant-specific API). Do you think it's somehow possible?
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, the common type is the problem. There is a really long signature and I'd love to shorten it to
meta : Any -> Meta.Right now we are getting
No_Such_Methoderrors. Rather than throwing the (or another) error, I'd just return some default, when sensible. If I rewrote toMetawith multiple constructors.