Implement special handling of subapplications when collecting MethodCallInfo #9677
Conversation
When collecting MethodCallInfo
app/gui2/src/util/callTree.ts
Outdated
| notAppliedArguments: correctedNotAppliedArguments.sort().slice(appliedArgs), | ||
| }, | ||
| suggestion: info.suggestion, | ||
| partiallyApplied: info.partiallyApplied, |
There was a problem hiding this comment.
On self-review I noticed that I’m not doing anything with this field. The issue is that I don’t quite understand its purposes and how it should be changed for outer expressions when it’s coming from inner ones.
app/gui2/src/util/callTree.ts
Outdated
| return { | ||
| methodCall: { | ||
| ...info.methodCall, | ||
| notAppliedArguments: correctedNotAppliedArguments.sort().slice(appliedArgs), |
There was a problem hiding this comment.
I don't understand why is sort necessary here.
There was a problem hiding this comment.
Do we have a guarantee that the list coming from the engine side will always be sorted? There is no such thing in the documentation, although it seems to be the current behavior.
app/gui2/src/util/callTree.ts
Outdated
| // No info, continue recursion to the next sub-application AST. | ||
| if (ast instanceof Ast.App) { | ||
| if (ast.argumentName) | ||
| return get(ast.function, appliedArgs, [...appliedNamedArgs, ast.argumentName.code()]) | ||
| return get(ast.function, appliedArgs + 1, appliedNamedArgs) | ||
| } | ||
| } | ||
| return get(ast, 0, []) |
There was a problem hiding this comment.
This function could easily be a loop. That should be easy to transform, since currently it is always a tail recursion.
| { | ||
| description: '1 named & 1 unnamed args.', | ||
| code: 'Aggregate_Column.Sum as=x y', | ||
| subapplicationIndex: 2, | ||
| notAppliedArguments: [0, 1], | ||
| expectedNotAppliedArguments: [], | ||
| }, | ||
| ] as TestCase[])( |
There was a problem hiding this comment.
Make sure to also add a test case with notAppliedArguments being "incorrectly" empty, which afaik is the actual response we get from engine for constructors.
| if (prevFunctionState != null && info?.partiallyApplied === true && ast instanceof Ast.Ident) { | ||
| return Score.Mismatch | ||
| } |
There was a problem hiding this comment.
The purpose of that condition and partiallyApplied property was to avoid rendering the argument lists twice in specific case of calling partially applied function from another node:
node1 = some_func arg3=baz
node2 = node1 foo bar
Since node1 inside node2 expression itself has a Function type and presents a list of non-applied arguments, those used to be rendered again. With your changes and the prefixCalls test above, this condition might no longer be necessary. If that's true, the whole partiallyApplied info could be completely removed as well. That would also save us from calling getMethodCallInfo in the score function, which is really neat.
There was a problem hiding this comment.
Thank you for explanation. My testing showed that partiallyApplied check is indeed redundant for us. I don’t think we can get rid of getMethodCallInfo – otherwise we would not build a function widget in case of node2 = node1. Below is behavior after my changes:
When no arguments applied to the second node (node2 = node1):
When arguments applied to the second node (node2 = node1 path):
I think it looks correct.
| @@ -521,7 +521,7 @@ const baseMockNode = { | |||
| conditionalPorts: new Set(), | |||
| } satisfies Partial<Node> | |||
|
|
|||
| function mathodCallEquals(a: MethodCall | undefined, b: MethodCall | undefined): boolean { | |||
| export function mathodCallEquals(a: MethodCall | undefined, b: MethodCall | undefined): boolean { | |||
There was a problem hiding this comment.
Why export? This doesn't seem to be used anywhere else.
There was a problem hiding this comment.
Yeah, but I still think it can become useful in the future, so I don’t want to remove it. export is a way of disabling unused linter warning.
There was a problem hiding this comment.
In general I would prefer to remove unused code, unless you have a specific use in the future in mind for it.
vitvakatu
added
CI: Ready to merge
Pull Request Description
Fixes on of the issues in #9354
Stale method call info for inner sub-application was causing additional argument placeholders on the node for certain expressions. Now it is fixed:
subapplications.mp4
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.
./run ide build.