Skip to content
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

Autoscoping for Table filter #9277

Closed
3 tasks
JaroslavTulach opened this issue Mar 5, 2024 · 13 comments
Closed
3 tasks

Autoscoping for Table filter #9277

JaroslavTulach opened this issue Mar 5, 2024 · 13 comments

Comments

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Mar 5, 2024

The Table filter method uses

@filter Widget_Helpers.make_filter_condition_selector

make sure the filter argument can use autoscoped constructors in the IDE.

Tasks

@JaroslavTulach JaroslavTulach converted this from a draft issue Mar 5, 2024
@JaroslavTulach JaroslavTulach moved this from ❓New to 📤 Backlog in Issues Board Mar 5, 2024
@JaroslavTulach
Copy link
Member Author

The IDE probably need to handle .. prefix for autoscoped constructors as prefix and not operator with missing lhs first to actually test the Table.filter behavior.

@4e6 4e6 moved this from 📤 Backlog to 🔧 Implementation in Issues Board Mar 11, 2024
@4e6
Copy link
Contributor

4e6 commented Mar 11, 2024

gui interprets the expression ..Constructor as a binary operation with the first argument not provided. The work on the parser is needed to recognize .. as a unary operator applied to constructor.

enso-autoscope-filter

@4e6
Copy link
Contributor

4e6 commented Mar 11, 2024

Also, the expression updates for autoscoped constructor calls don't contain the method pointer. The work on the engine side is also needed.

@4e6
Copy link
Contributor

4e6 commented Mar 11, 2024

The issue is that during the instrumentation, the result of the ..Constructor call is an UnresolvedConstructor object. To create a method pointer, the engine needs to intercept a node after the method resolution is done.
\cc @JaroslavTulach

@JaroslavTulach
Copy link
Member Author

The issue is that during the instrumentation, the result of the ..Constructor call is an UnresolvedConstructor object.

I see. We cannot create Method Call object from UnresolvedConstructor as Adam requested...

To create a method pointer, the engine needs to intercept a node after the method resolution is done.

However an UnresolvedConstructor gets converted to real constructor according to docs at the end.

  • the call just happens sometimes later
  • however that must be similar to suspended arguments
  • does IDE handle suspended arguments well?
  • if so, what do we need to do to send IDE enough information to also handle autoscoped constructors as well?

@enso-bot
Copy link

enso-bot bot commented Mar 12, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-03-11):

Progress: Started working on the task. Investigated how the autoscope constructors with arguments are displayed in the gui. Investigated how the autoscope calls are represented during the instrumentation. It should be finished by 2024-03-13.

Next Day: Next day I will be working on the #9277 task. Continue working on the task

@4e6
Copy link
Contributor

4e6 commented Mar 12, 2024

It seems that all the problems with autoscope are from the fact that the resolution is happening in runtime. The ..Constructor is supposed to mean "I'm too lazy to write the type name, you can figure it out yourself". And we indeed should be able to do that in the compile time. Given

type T
    A
    B
type S
    B
foo t:T|S = t
  • In the foo ..A call, the ..A expression is consumed by the method foo, which first argument has type T. This information should be enough to make a resolution and insert a T.A application node during the codegen.
  • The foo ..B should result in a compiler error indicating that the ..B resolution is ambiguous
  • The foo ..C should result in a compiler error indicating that the ..C cannot be resolved
  • the ..A call should result in a compiler error indicating that the ..A cannot be resolved because we don't have the context of the foo call providing the type information

This will require a kind of type inference (i.e. associating types with expressions) which the compiler cannot do right now. But I think it's a good opportunity to start by implementing this very isolated scenario \cc @radeusgd

Also, making the name resolution in compile time will not affect the runtime performance.

@JaroslavTulach
Copy link
Member Author

... autoscope ... resolution is happening in runtime. ... be able to do that in the compile time.

The possibility of having autoscoping based on static type inference has been discussed and rejected before the work started. If you believe static type inference is the only way to fix the problems for Table.filter, please continue the discussion in its discussion forum.

Let's leave this issue focused on solving the problem, but feel free to question the overall concept.

@enso-bot
Copy link

enso-bot bot commented Mar 13, 2024

Dmitry Bushev reports a new STANDUP for yesterday (2024-03-12):

Progress: Continue working on the task. Investigated the issue of why the method calls are not reported for the autoscope constructors. It should be finished by 2024-03-13.

Next Day: Next day I will be working on the #9277 task. Continue working on the task

@hubertp
Copy link
Contributor

hubertp commented Mar 25, 2024

Should probably be closed once #9259 is closed.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Apr 3, 2024

With commit 3a986f0 from #9452 I was able to get operator51546.filter 'Organisation' filter=(..Equal 'SpaceX') working with following change in Table:

diff --git distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso
index a42bc1dd8c..9aaf724a4f 100644
--- distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso
+++ distribution/lib/Standard/Table/0.0.0-dev/src/Data/Table.enso
@@ -1476,16 +1476,17 @@ type Table
     filter self column (filter : Filter_Condition | (Any -> Boolean) = Filter_Condition.Equal True) on_problems=Report_Warning = case column of
         _ : Column ->
             mask filter_column = Table.Value (self.java_table.filter filter_column.java_column)
-            case filter of
+            f = Panic.catch Any (filter:Filter_Condition) _->filter
+            case f of
                 _ : Filter_Condition ->
-                    resolved = (self:Table_Ref).resolve_condition filter
+                    resolved = (self:Table_Ref).resolve_condition f
                     mask (make_filter_column column resolved on_problems)
                 _ : Function -> Filter_Condition_Module.handle_constructor_missing_arguments filter <|
                     mask (column.map filter)
-        _ : Expression -> self.filter (self.evaluate_expression column on_problems) filter on_problems
+        _ : Expression -> @Tail_Call self.filter (self.evaluate_expression column on_problems) filter on_problems
         _ ->
             table_at = self.at column
-            self.filter table_at filter on_problems
+            @Tail_Call self.filter table_at filter on_problems
 

e.g. the inline type ascription (filter : Filter_Condition | (Any -> Boolean) is quite problematic. It is the same as (filter : Filter_Condition | Function) and autoscoped constructor has type Function. E.g. it matches the runtime type without any conversion! Moreover doing case filter of and _ : Filter_Condition pattern matching doesn't force the conversion of UnresolvedConstructor to real Filter_Condition - it just doesn't match. Hence the f = Panic.catch Any (filter:Filter_Condition) _->filter attempt to force a conversion. Nasty.

IDE knows Equal's argument

However the IDE seems to know ..Equal takes one argument and what kind of values it can offer for that argument.

@radeusgd
Copy link
Member

radeusgd commented Apr 5, 2024

e.g. the inline type ascription (filter : Filter_Condition | (Any -> Boolean) is quite problematic. It is the same as (filter : Filter_Condition | Function) and autoscoped constructor has type Function. E.g. it matches the runtime type without any conversion! Moreover doing case filter of and _ : Filter_Condition pattern matching doesn't force the conversion of UnresolvedConstructor to real Filter_Condition - it just doesn't match. Hence the f = Panic.catch Any (filter:Filter_Condition) _->filter attempt to force a conversion. Nasty.

Huh, that's an interesting edge case! The filter operator taking a function is indeed complicating things a bit - we also need special handling for to more nicely display info about missing args to Filter_Condition (see: unify_condition_or_predicate). I guess we could move this special 'hack' into unify_condition_or_predicate as well.

Hopefully it will not be needed in too many places - fortunately I don't think there's many other places that mix an autoscoped type with Function.

@4e6
Copy link
Contributor

4e6 commented Apr 8, 2024

#9293 (comment) verifies that after fixing the expression updates for autoscope constructors in #9452, the dropdowns with autoscope constructors work in gui the same way as normal constructors

@4e6 4e6 closed this as completed Apr 8, 2024
@github-project-automation github-project-automation bot moved this from 🔧 Implementation to 🟢 Accepted in Issues Board Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants