UnresolvedSymbol is now accepted by Vector.sort #6334
Conversation
`Vector.sort` does some custom method dispatch logic which always expected a function as `by` and `on` arguments. At the same time, `UnresolvedSymbol` is treated like a (to be resolved) `Function` and under normal circumstances there would be no difference between `_.foo` and `.foo` provided as arguments. Rather than adding an additional phase that does some form of eta-expansion, to accomodate for this custom dispatch, this change only fixes the problem locally. We accept `Function` and `UnresolvedSymbol` and perform the resolution on the fly. Ideally, we would have a specialization on the latter but again, it would be dependent on the contents of the `Vector` so unclear if that is better.
hubertp
requested review from
jdunkerley,
radeusgd,
4e6 and
JaroslavTulach
as code owners
hubertp
added
the
CI: No changelog needed
There was a problem hiding this comment.
I assume the primary goal is to make the (on = .b) syntax work. Approving.
However I'd be also interested in making it run (reasonably) fast. Or (at least) not to do such wild things where a Node is held and used by a non-node object. There should be a clear separation between "code" (represented by Node hierarchy) and "data" - however with Compare I am not sure where to draw the line - it feels like a mixture of both.
...time/src/main/java/org/enso/interpreter/node/expression/builtin/ordering/SortVectorNode.java
Outdated
Show resolved
Hide resolved
| * as the comparator function, which has to be first resolved before it | ||
| * can be used to compare values. | ||
| */ | ||
| private abstract class Compare { |
There was a problem hiding this comment.
The "Truffle way" of doing this would be to have a class Compare extends Node and insert the right one (or more) into the AST somehow.
There was a problem hiding this comment.
Will revisit in future work, as discussed.
|
|
||
| @Override | ||
| boolean hasFunctionSelfArgument(Object definedOn) { | ||
| if (function.getSchema().getArgumentsCount() > 0) { |
There was a problem hiding this comment.
The function.getSchema() is a candidate for speculating that it is a constant. However done this way, it is certainly not going to be a constant.
There was a problem hiding this comment.
Will revisit in future work, as discussed.
| @Override | ||
| boolean hasFunctionSelfArgument(Object definedOn) { | ||
| if (function.getSchema().getArgumentsCount() > 0) { | ||
| return function.getSchema().getArgumentInfos()[0].getName().equals("self"); |
There was a problem hiding this comment.
Can equals be used on fast-path? Probably this code isn't on fast-path, right?
There was a problem hiding this comment.
Right, this is not on fast-path. It is called from sortGeneric specialization that is behind boundary. If it would be on fast-path, native-image build test should fail, I believe.
...time/src/main/java/org/enso/interpreter/node/expression/builtin/ordering/SortVectorNode.java
Outdated
Show resolved
Hide resolved
| private class CompareFromUnresolvedSymbol extends Compare { | ||
|
|
||
| private final UnresolvedSymbol unresolvedSymbol; | ||
| private final MethodResolverNode methodResolverNode; |
There was a problem hiding this comment.
We should avoid having references to Node from a "data". Or is Compare supposed to be "code"? Then it should be Node... This feels like mixing apples and oranges...
You could also go the slow-path route and use MethodResolverNode.getUncached() at usage sites of methodResolverNode.
There was a problem hiding this comment.
This, indeed, feels weird. I think that optimally, this whole Compare class hierarchy should work like "InvokeCompareFunctionNode". The method resolution from the UnresolvedSymbol could be done straight away in GenericSortComparator constructor.
There was a problem hiding this comment.
The method resolution from the UnresolvedSymbol could be done straight away in GenericSortComparator constructor.
Not really. Constructor has no knowledge (yet) of the values on which one should dispatch the unresolved symbol on.
There was a problem hiding this comment.
Sorry, I misinterpreted that. You are right. I guess that is fine then.
Thanks.
I assume that given that |
Well, the code is a bit complex. During recent discussions we realized that there is a However it is a deeper problem and it can wait for another PR. |
Yeap, agreed. Thanks for the pointers regarding Truffle. I will create a follow up ticket. |
Pull Request Description
Vector.sortdoes some custom method dispatch logic which always expected a function asbyandonarguments. At the same time,UnresolvedSymbolis treated like a (to be resolved)Functionand under normal circumstances there would be no difference between_.fooand.fooprovided as arguments.Rather than adding an additional phase that does some form of eta-expansion, to accomodate for this custom dispatch, this change only fixes the problem locally. We accept
FunctionandUnresolvedSymboland perform the resolution on the fly. Ideally, we would have a specialization on the latter but again, it would be dependent on the contents of theVectorso unclear if that is better.Closes #6276,
Important Notes
There was a suggestion to somehow modify our codegen to accomodate for this scenario but I went against it. In fact a lot of name literals have
isMethodflag and that information is used in the passes but it should not control how (late) codegen is done. If we were to make this more generic, I would suggest maybe to add separate eta-expansion pass. But it could affect other things and could be potentially a significant change with limited potential initially, so potential future work item.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.