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

Correct expression updates of partially applied constructors #6939

Closed

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Jun 2, 2023

Pull Request Description

close #6903

Works this way:

  • Detect the call to the constructor on function enter
  • On the expression return, check if it was a partially applied constructor call. Then build the function info from the resulting function

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,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 2, 2023
@4e6 4e6 self-assigned this Jun 2, 2023
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written here:

I believe we are trying to tackle the problem from a wrong angle. What's the goal? When there is a partially applied function, then we want to know what arguments have been applied. That is not information about "a call" that has happened, but about a "future call" - we have a function value and the IDE needs to know what to do with it.

That means (in my opinion) that ProgramExecutionSupport.scala needs to do following when composing sendExpressionUpdate:

  • when the value.getType is Standard.Builtins.Main.Function
  • accompany the information with a MethodPointer to the function definition
  • use (value.value : runtime.Function).getSchema() to obtain hasPreApplied array and send it to the IDE somehow

I don't think the IdExecutionInstrument shall differentiate between constructors and other executables. The solution presented in this PR continues to push the solution in the wrong direction - at least according to my opinion. Or I am completely confused. In any case I cannot approve it.

@4e6
Copy link
Contributor Author

4e6 commented Jun 3, 2023

  • If we just detect that the expression returns a function (on the expression return), we don't have enough information to build a method pointer from it. It has to be captured (and stored) before (on the function call return)
  • Expression update contains information about the computed expression, not the expression that will be computed.
main =
    x1 = func1 1
    x1

func1 x = func2 x
func2 x y z = x + y + z

Following your logic, the expression update of the func1 1 will return arguments of the func2 call.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Frizi wrote:

If you are able to ... fill methodPointer (or a function schema for that matter) before actual execution, the IDE should be ready to accept it

Pawel also noted that the current implementation is a significant source of buggy behavior:

this behavior is also a cause of some issues during graph reevaluation, where new Pending updates are setting methodPointers to null, causing widgets and arguments to disappear for a moment

Based on that I request changes. We don't want any more workarounds in IdExecutionInstrument - we want to make sure that ProgramExecutionSupport.sendExecutionUpdates sends info about Function values to the IDE. Tracked as

@4e6
Copy link
Contributor Author

4e6 commented Jun 11, 2023

Pawel is talking about sending the method pointers in the compile time when the Pending updates are sent. It has nothing to do with this change.

Regarding his idea. The engine doesn't have sophisticated enough static analysis to do that right now. What IDE can do is preserve the information about the method pointer when it receives the Pending update instead of erasing all the information about the node. I assume erasing only the type information should be enough to gray out the nodes

@4e6
Copy link
Contributor Author

4e6 commented Jun 11, 2023

Superseded by #6957

@4e6 4e6 closed this Jun 11, 2023
@4e6 4e6 deleted the wip/db/6903-atom-constructor-is-using-two-function-calls branch June 11, 2023 09:37
@JaroslavTulach
Copy link
Member

Pawel is talking about sending the method pointers in the compile time when the Pending updates are sent.
It has nothing to do with this change.

Yeah, now I get it too. Enso doesn't have a static type checker. The only thing we can do is to deliver the information during runtime. We just have to make sure we deliver the right information as #6957 suggests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atom constructor is using two function calls
2 participants