-
Notifications
You must be signed in to change notification settings - Fork 318
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
Send info about function values #7168
Send info about function values #7168
Conversation
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.
Overall LGTM, minor questions/comments
engine/polyglot-api/src/main/scala/org/enso/polyglot/runtime/Runtime.scala
Outdated
Show resolved
Hide resolved
public static int[] collectNotAppliedArguments(Function function) { | ||
FunctionSchema functionSchema = function.getSchema(); | ||
Object[] preAppliedArguments = function.getPreAppliedArguments(); | ||
boolean isStatic = preAppliedArguments[0] instanceof Type; |
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.
Is this reliable? We could have an argument which is a type but the function is not a static?
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, to resolve the function the self argument has to be present. And at this point, the function is resolved.
If the function is not static, the self argument will be something like UnboxingAtom
I believe.
...time-instrument-common/src/main/java/org/enso/interpreter/instrument/IdExecutionService.java
Outdated
Show resolved
Hide resolved
…untime.scala Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
…reter/instrument/IdExecutionService.java Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
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.
I'd like to know what will happen when schema changes? Will updates be delivered? Is that a covered test case?
} | ||
|
||
public static int[] collectNotAppliedArguments(Function function) { | ||
FunctionSchema functionSchema = function.getSchema(); | ||
Object[] preAppliedArguments = function.getPreAppliedArguments(); |
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.
What if the preAppliedArguments
array has zero size? Will the next line not throw an ArrayIndexOutOfBoundsException
?
Option( | ||
ctx.executionService.toDisplayString(warnings(0).getValue) | ||
val warnings = | ||
Option.when( |
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.
Replacing if
with Option.when
! I can only say: I am not fan of such aggressive functional rewrites.
@@ -953,6 +963,109 @@ class RuntimeServerTest | |||
) | |||
} | |||
|
|||
it should "send method pointer updates of partially applied type method returning a method" in { |
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.
What will happen when the result is a function and remains a function yet its schema changes on subsequent evaluation of the result? Will we deliver an update to value/schema?
I am envisioning following test case:
f a b c = a + b + c
fn what = case what of
0 -> f
1 -> f 3
2 -> f 3 8
_ -> f 3 8 9
what will happen if some code invoke fn 1
first and then the code get changed and invokes fn 2
- will the IDE be notified about the schema change?
Pull Request Description
close #6957
Extend
ExpressionUpdate
message and send a function schema if the returned value is a function.Important Notes
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.