Meta.get_annotation and extension methods #7000

JaroslavTulach · 2023-06-09T11:37:02Z

Pull Request Description

Attempt to fix #6955. Writing a test to check behavior of Meta.get_annotation and extension methods. The idea is to deprecate Meta.get_attribute and rather than that obtain the annotation for function itself.

That however requires to pass function as a handle thru the visualizations. The problem there is that saturated functions like to turn themselves into into results and we shall prevent that. Hence 5d87c9c introduces Meta.Function with its get_annotation parameter_name method.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
All code follows the
Scala,
Java,
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

JaroslavTulach · 2023-06-09T11:41:17Z

test/Tests/src/Semantic/Meta_Spec.enso

+
+        Test.specify "verify calling everything works" <|
+            describe_anno = Meta.get_annotation it "describe" "which"
+            describe_anno it . should_equal 13


This is very "verbose", @jdunkerley! One has to:

a lambda function @which (self -> self.a) as annotation

gets a lambda function via Meta.get_annotation

needs to invoke it

I was hoping Meta.get_annotation actually invokes the code in @which self.a by itself internally! E.g. one could reference self in the code after @which and it evaluation would happen by itself too.

With the current state of affairs, there is no reason why Meta.get_annotation shall take self argument! It rather needs a Function!

67fb583 let's anyone "resolve" a function and then pass such function as a handle around. Then everyone can query for annotations associated with that function.

Agree on the get_annotation having to unwind to get the value is annoying.

I wrapped it in the visualization layer and haven't thought of it since!

JaroslavTulach · 2023-06-12T14:22:13Z

distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso

@@ -217,6 +218,20 @@ type Polyglot
        lang_str = get_polyglot_language self.value
        if lang_str == "java" then Language.Java else Language.Unknown

+type Function


There are many type in Meta - why don't we have a single Meta type and many constructors Atom, 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 Meta type and constructors. Vote -1 for keeping status quo as much as possible. CCing @jdunkerley, @GregoryTravis, @radeusgd

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 Atom may have a fields field, but this does not make sense for a Function or Primitive. 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?

Yes, the common type is the problem. There is a really long signature and I'd love to shorten it to meta : Any -> Meta.

an just throw 'Illegal_State' for such wrong calls

Right now we are getting No_Such_Method errors. Rather than throwing the (or another) error, I'd just return some default, when sensible. If I rewrote to Meta with multiple constructors.

jdunkerley

This looks like a good workaround to the issue,

So to be clear. The IDE would call: Widgets.get_widget_json value value.my_function ["arg1", "arg3"] ....

We will need to amend the Visualization layer and the rust code. We must include the ... to shutdown auto evaluation of the defaulted argument.

jdunkerley · 2023-06-13T10:52:18Z

test/Tests/src/Semantic/Meta_Spec.enso

+
+        Test.specify "verify calling everything works" <|
+            describe_anno = Meta.get_annotation it "describe" "which"
+            describe_anno it . should_equal 13


Agree on the get_annotation having to unwind to get the value is annoying.

I wrapped it in the visualization layer and haven't thought of it since!

radeusgd · 2023-06-13T11:14:24Z

So to be clear. The IDE would call: Widgets.get_widget_json value value.my_function ["arg1", "arg3"] ....

Wouldn't it have to be

Widgets.get_widget_json value (value.my_function ...) ["arg1", "arg3"]

instead?

I think we need to use ... to suspend defaults on my_function, not on get_widget_json (the latter I don't think should have any default arguments).

Frizi · 2023-06-14T08:17:05Z

I don't know how the IDE is supposed to accomplish that by using the Visualizations API. Right now it is not possible to refer to multiple existing expressions in a single visualization call. See https://github.com/orgs/enso-org/discussions/6832 for more details.

Right now, the self argument's expression is passed using the expression ID (effectively passing in its evaluation result), but the method name is passed in as a string literal. If we extend the visualizations to support referring to multiple expressions (see discussion linked above), I could pass in the actual method expression as well. But then, adding ... to it would not be possible anyway.

GitHub
Widget Visualizations with multiple input expressions · enso-org · Discussion #6832
Currently, the data for dynamic node widgets is requested using language server's Visualization API. That allows us to resolve method annotations using the data from evaluated method call's target....

…ystem of get_annotation

Frizi · 2023-06-14T08:54:12Z

app/gui/src/controller/graph/widget.rs

@@ -392,7 +392,7 @@ impl QueryData {
    /// Generate visualization metadata for this query.
    fn visualization_metadata(&self) -> Metadata {
        let arguments: Vec<Code> = vec![
-            Self::escape_visualization_argument(&self.method_name).into(),
+            format!("(self.{}...)", &self.method_name).into(),


It is surprising to me that this could work. I assumed that the arguments passed to the visualization preprocessor were treated as expressions in method call argument position. In that case, self would not refer to the right thing. Looks like my assumption was incorrect.

So, to be clear, is it true that the passed-in "arguments" are treated effectively as expressions within the BODY of the visualization preprocessor method?

I am not sure it works. Clearly we are able to compute get_annotation for the select_into_database_table..., but only when computing the nodes. The primary_key drop down remains empty showing Nothing...

While it will surely work when sketching out that query flow in the IDE itself, the question is if it will be resolved as intended through the visualization API. You would need to check the log of visualization requests and responses to be sure it behaves as expected.

The IDE is able to come up with a decent set of widgets purely based on data from suggestion database that it already has, even when the visualization query fails. So inspecting the graph visually for dropdowns is not enough to assess that it works.

It doesn't work. Evaluation fails with unknown/wrong self. Is there a way to obtain the name of the self node? In this case operator18? It might be available... No it doesn't seem to be readily available in EvalNode.parseExpression as the LocalScope is empty.

Can we at least find the type of operator18 out, @Frizi?

How does the self value get in, @4e6? There is:

ctx.executionService.callFunctionWithInstrument( visualisation.cache, visualisation.module, visualisation.callback, expressionValue +: visualisation.arguments: _* )

in ProgramExecutionSupport.scala and expressionValue is self while the rest is defined by the visualization.

The visualisation.module is an interesting object - update: no, it is not - the last interesting module identification is in IdExecutionInstrument just before calling onExpressionReturn when we can get node.getRootNode().getModuleScope() to be the ModuleScope of the user module.

To continue experiments I'd like to change the IDE to invoke the WIDGET_VISUALIZATION_METHOD with one more argument - reference (FQN name is sufficient) to the module that is being edited. Help needed - nothing in widget.rs file seems to have a reference to the module name as far as I can tell.

Given that we already send the expression ID into the visualization, can't the used module be computed from that on the engine side?

... already send the expression ID into the visualization, can't the used module be
computed from that on the engine side?

You send it to a completely different part of the engine. It needs to be available when evaluating the visualization. Engine knows nothing about Widgets.get_widget_json - that is a contract between IDE and Standard.Visualization - I shouldn't try to interpret the arguments to Widgets.get_widget_json function in the engine - better if the IDE assembles them properly.

However IDE cannot specify the name of the module that requests the visualization, as the IDE doesn't know it. According to @Frizi, following might help:

Interesting change: https://github.com/enso-org/enso/pull/7028/files#diff-4b79b29711f3cafb4bc4dbbd0882b63066d1de06e338bc8f86bf7c03a3e2e936R45
The project name will now actually be available in the graph.

@Frizi - that commit helped a lot. I am able to send the name of the edited module to the engine and evaluate the visualization properly in the context of such visualisation_module. Please help me with reviews and QA in #7115

radeusgd · 2023-06-15T09:35:47Z

...runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/MetaBuiltinNode.java

+      return 4;
+    }
+    if (TypesGen.isDataflowError(value)) {
+      return -5;


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?

This is the relevant commit to read: 133ba8d

I think it would be good to have some comment explaining this logic, as it's not that easy to understand it.

What is the result of Meta.meta (Error.throw "Foo")?

IIRC it used to be Error.Value, but how I understand the current code, I'd guess it to be Primitive.Value (for Text). Do I understand it correctly?

xvcgreg · 2023-06-19T11:21:10Z

@JaroslavTulach What's the status here?

JaroslavTulach · 2023-06-20T13:42:39Z

@JaroslavTulach What's the status here?

Still struggling and I will continue to struggle for the next iteration.

JaroslavTulach · 2023-06-23T12:02:48Z

Will be fixed by #7115 - no need for the changes in this PR anymore.

JaroslavTulach added 2 commits June 9, 2023 13:01

Multiple modules to verify Meta.get_annotation and extension methods

5fded8b

Meta_Spec cannot see the method while Meta_Define_Usage can

4100e7e

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 9, 2023

JaroslavTulach requested review from hubertp, 4e6 and Akirathan June 9, 2023 11:37

JaroslavTulach requested a review from jdunkerley as a code owner June 9, 2023 11:37

JaroslavTulach self-assigned this Jun 9, 2023

JaroslavTulach requested review from radeusgd and GregoryTravis as code owners June 9, 2023 11:37

JaroslavTulach commented Jun 9, 2023

View reviewed changes

GregoryTravis approved these changes Jun 9, 2023

View reviewed changes

Pass fn as a handle and query its annotations from anywhere

67fb583

enso-bot bot mentioned this pull request Jun 10, 2023

@widgets for extension functions of Table may not be found #6955

Closed

hubertp approved these changes Jun 12, 2023

View reviewed changes

Akirathan approved these changes Jun 12, 2023

View reviewed changes

Special Meta.Function to work as a handle to Function

5d87c9c

JaroslavTulach commented Jun 12, 2023

View reviewed changes

Removing Meta.is_checks and adding Meta.meta_builtin factory

133ba8d

JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jun 13, 2023

jdunkerley approved these changes Jun 13, 2023

View reviewed changes

JaroslavTulach requested a review from Frizi June 13, 2023 18:54

Resolve the function as soon as possible and only then pass it thru s…

83152a8

…ystem of get_annotation

JaroslavTulach requested review from MichaelMauderer, wdanilo, farmaazon, mwu-tow and kazcw as code owners June 14, 2023 08:46

Frizi reviewed Jun 14, 2023

View reviewed changes

enso-bot bot mentioned this pull request Jun 15, 2023

Calculating sum is 10x slower using Statistics #5067

Closed

radeusgd reviewed Jun 15, 2023

View reviewed changes

JaroslavTulach closed this Jun 23, 2023

farmaazon deleted the wip/jtulach/MetaGetAnnotation_6955 branch May 9, 2024 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meta.get_annotation and extension methods #7000

Meta.get_annotation and extension methods #7000

JaroslavTulach commented Jun 9, 2023 •

edited

Loading

JaroslavTulach Jun 9, 2023

JaroslavTulach Jun 9, 2023

jdunkerley Jun 13, 2023

JaroslavTulach Jun 12, 2023

radeusgd Jun 12, 2023

JaroslavTulach Jun 12, 2023

jdunkerley left a comment

jdunkerley Jun 13, 2023

radeusgd commented Jun 13, 2023

Frizi commented Jun 14, 2023 •

edited

Loading

Frizi Jun 14, 2023

JaroslavTulach Jun 14, 2023

Frizi Jun 14, 2023

JaroslavTulach Jun 14, 2023 •

edited

Loading

JaroslavTulach Jun 14, 2023 •

edited

Loading

JaroslavTulach Jun 19, 2023 •

edited

Loading

Frizi Jun 19, 2023

JaroslavTulach Jun 20, 2023

JaroslavTulach Jun 20, 2023

JaroslavTulach Jun 24, 2023

radeusgd Jun 15, 2023

JaroslavTulach Jun 16, 2023 •

edited

Loading

radeusgd Jun 16, 2023

radeusgd Jun 16, 2023

xvcgreg commented Jun 19, 2023

JaroslavTulach commented Jun 20, 2023

JaroslavTulach commented Jun 23, 2023

Meta.get_annotation and extension methods #7000

Meta.get_annotation and extension methods #7000

Conversation

JaroslavTulach commented Jun 9, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdunkerley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd commented Jun 13, 2023

Frizi commented Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Jun 14, 2023 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xvcgreg commented Jun 19, 2023

JaroslavTulach commented Jun 20, 2023

JaroslavTulach commented Jun 23, 2023

JaroslavTulach commented Jun 9, 2023 •

edited

Loading

Frizi commented Jun 14, 2023 •

edited

Loading

JaroslavTulach Jun 14, 2023 •

edited

Loading

JaroslavTulach Jun 14, 2023 •

edited

Loading

JaroslavTulach Jun 19, 2023 •

edited

Loading

JaroslavTulach Jun 16, 2023 •

edited

Loading