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

Lookup method pointers in IDE #5578

Merged
merged 18 commits into from
Feb 8, 2023

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Feb 7, 2023

Pull Request Description

Closes #5036

Move the logic that looks up method pointers from the language server to IDE. This way we can keep the suggestion updates and expression updates async, otherwise it will hurt the initial startup time of LS.
Fixes the issue when some expression updates does not contain the method pointer.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@4e6 4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 7, 2023
@4e6 4e6 self-assigned this Feb 7, 2023
Comment on lines 244 to 245
let method_pointer = node_info.method_call.as_ref().ok_or(NoResolvedMethod(node))?;
Ok(Rc::new(method_pointer.clone()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to avoid this clone: let's store the method pointer in the ComputedValueInfo under Rc.

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.

Changes on the JVM side look sane. Using method pointer in the protocol is more human readable and still exact enough to identify the right method. The conversion of method pointer on the IDE side doesn't look complex either.

I like the change.

@@ -324,7 +324,7 @@ interface ExpressionUpdate {
/**
* The updated pointer to the method call.
*/
methodPointer?: SuggestionId;
methodPointer?: MethodPointer;
Copy link
Member

Choose a reason for hiding this comment

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

This is the (major) API change: Instead of using SuggestionId to identify a method, we are using MethodPointer.


}
.pipeTo(sessionRouter)
val computedExpressions = expressionUpdates.map { update =>
Copy link
Member

Choose a reason for hiding this comment

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

The code is simplified. Instead of trying to find a suggestion id we just send a tripple (module, type, methodname)

* @param profilingInfo profiling information about the expression
* @param fromCache whether or not the expression's value came from the cache
* @param payload an extra information about the computed value
*/
case class ExpressionUpdate(
expressionId: UUID,
`type`: Option[String],
methodPointer: Option[Long],
methodPointer: Option[MethodPointer],
Copy link
Member

Choose a reason for hiding this comment

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

This will probably increase the size of ExpressionUpdate messages - instead of a number we send the tripple with three strings, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the MethodPointer is a triple containing module, self type, and method name

@4e6 4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Feb 7, 2023
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Engine changes look sane

}
}

def toProtocolMethodPointer(methodPointer: Api.MethodPointer): MethodPointer =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't ContextEventsListener just make that method public (or protected, if sufficient)?
That would eliminate duplicate method.

@mergify mergify bot merged commit 53d5487 into develop Feb 8, 2023
@mergify mergify bot deleted the wip/db/expression-method-pointer-184381624 branch February 8, 2023 00:57
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. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expressions will not have methodPointer assigned if they are updated before suggestion database.
4 participants