General override member(s) functionality #379

themkat · 2022-08-04T15:38:28Z

General info

Override any member that is currently not implemented in the current class. This includes open functions, functions with default implementations, open members, abstract stuff etc. The main difference here is that this is general override member functionality, NOT JUST abstract members like the quick fix does. This is super useful for cases where you need to override a default implementation (e.g toString, Thread run, lsp4j interface methods like in TextDocumentService etc.), NOT just the unimplemented abstract ones. For me it would be super useful for my day job using Kotlin 😄 Having to google api docs to find signatures are a pain in the ass just for doing simple stuff like this.

Two small issues:

Implicit Any/Object methods does not seem to work. I can't really figure out why. The methods from them show up if the class has at least one superclass. Unsure on how to solve this, there are a TODO in the code at a relevant place.
No support for empty class bodies (i.e, no brackets). Not sure how to solve that. Probably not super important either.

I think these are very minor. Just having this functionality in place would at least make me more productive using the language server 😄

The quick fix for abstract methods and this new functionality, share a lot of code. I've moved most of the code to the override-package, as that was the most logical place to put it.

Fixes #359

This is a new protocol extension, so each lsp-client will need to have an implementation to use it. I've only done one for Emacs so far, and speaking of...

Emacs screenshots

Running lsp-kotlin-override-members on MyClass (notice that multiple selections are possible!):

When we click enter:

(yes, it is the same sample project I often use for screenshot. A boring Spring Boot demo app)

Yes, it works for external interfaces and open classes as well, don't worry 🙂 There is an example of JDK Thread in the tests (in this PR, not in Emacs)

Emacs client implementation (lsp-mode):
https://github.com/themkat/lsp-mode/blob/kotlin_overrides/clients/lsp-kotlin.el#L224

Will make a PR for it once this PR has been merged. Just in case something changes.

~~Will see if I can find the time to make a VSCode version or not. If not, I will make an issue in the vscode-kotlin repo with enough details to get someone else started~~ 🙂 Really want to work on either Type hierarchy or Javadoc for Kotlin SDK and the JDK instead 😛

EDIT: Made a VSCode version, as I was bored after work yesterday: fwcd/vscode-kotlin#104

… method

fwcd

Looks good, a few style suggestions here and there, but nothing too major 👍

fwcd · 2022-08-12T13:57:06Z

server/src/main/kotlin/org/javacs/kt/overridemembers/OverrideMembers.kt

+import org.jetbrains.kotlin.types.typeUtil.asTypeProjection
+
+// TODO: see where this should ideally be placed
+private const val DEFAULT_TAB_SIZE = 4


We currently hardcode this in the formatter too, I think there was an open issue to make this configurable, but that's probably out-of-scope for this PR.

Hmm. That's a great point. I can't find the exact issue, but I suggest we do this as part of another PR. Like you said, that is a bit out of scope for this PR... I prefer to keep the PRs as to-the-point as possible, which helps me keep focus on each task. 🙂 Please link the issue if you find it 🙂

fwcd · 2022-08-12T13:59:04Z

server/src/main/kotlin/org/javacs/kt/overridemembers/OverrideMembers.kt

+fun listOverridableMembers(file: CompiledFile, cursor: Int): List<CodeAction> {
+    val kotlinClass = file.parseAtPoint(cursor)
+
+    if (kotlinClass is KtClass) {
+        return createOverrideAlternatives(file, kotlinClass)
+    }
+
+    return emptyList()
+}


I think we can rewrite this to be a bit more concise

Suggested change

fun listOverridableMembers(file: CompiledFile, cursor: Int): List<CodeAction> {

val kotlinClass = file.parseAtPoint(cursor)

if (kotlinClass is KtClass) {

return createOverrideAlternatives(file, kotlinClass)

}

return emptyList()

}

fun listOverridableMembers(file: CompiledFile, cursor: Int): List<CodeAction> =

when (file.parseAtPoint(cursor)) {

is KtClass -> createOverrideAlternatives(file, kotlinClass)

else -> emptyList()

}

We could in theory rewrite this to use a when-expression like above (with modifications), but we need the kotlinClass-reference for the createOverrideAlternatives-call. That means the suggestion above won't compile due to unknown reference to kotlinClass. If we still need that variable, I don't think it improves much just using a when-expression instead of the if-version. Feel free to disagree 🙂 (or tell me if I'm missing something!)

server/src/main/kotlin/org/javacs/kt/overridemembers/OverrideMembers.kt

fwcd · 2022-08-12T14:20:58Z

server/src/main/kotlin/org/javacs/kt/overridemembers/OverrideMembers.kt

+// Checks if two functions have matching parameters
+private fun parametersMatch(
+        function: KtNamedFunction,
+        functionDescriptor: FunctionDescriptor
+): Boolean {


Perhaps we could rewrite this into a big functional zip? I haven't checked on the specifics though, maybe there's too much specialized logic to do that.

A bit unsure about this. We loop through two different lists; value parameters and type parameters. Unsure on how much it would improve in this case, but I may miss something...

…ptor

themkat · 2022-08-25T18:20:59Z

@fwcd , do you have time to look at this again soon? Would be good to get it done while the details are still semi-fresh in my memory. Sorry for nagging, know you are probably busy 🙁

themkat · 2022-09-08T19:04:01Z

@fwcd , bump?

fwcd · 2022-09-13T16:34:00Z

Lgtm, sorry for the delay. Thanks!

themkat · 2022-09-13T16:36:09Z

@fwcd , no problem 🙂 Sorry for nagging you. Just very eager about this project, so I can get a little impatient 🙁

themkat added 6 commits August 2, 2022 20:10

Skeleton of new general override members implementation. fwcd#359

5d49b80

Implement most of the general override functionality. Stubborn equals…

5207d57

… method

Fix weird issue with overriding equals

52f80e9

Add testcase for jdk superclass and fix minor issues

095ddaf

Minor tweaks and indentation fix

965f844

Removed todo comment

74136b4

themkat mentioned this pull request Aug 6, 2022

General override member(s) functionality - VSCode version fwcd/vscode-kotlin#104

Merged

fwcd reviewed Aug 12, 2022

View reviewed changes

themkat added 3 commits August 12, 2022 21:42

Remove unused variable

c5c4f51

Simplify code based upon PR comments

ed1d4ad

Simplify expression based upon earlier refactoring using MemberDescri…

596c6a3

…ptor

themkat requested a review from fwcd August 21, 2022 10:27

fwcd merged commit eef14bd into fwcd:main Sep 13, 2022

themkat deleted the GH-359 branch September 13, 2022 16:35

themkat mentioned this pull request Sep 13, 2022

Kotlin override/implement member functionality emacs-lsp/lsp-mode#3719

Merged

themkat mentioned this pull request Sep 25, 2022

Override/Implement suggestions #31

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

General override member(s) functionality #379

General override member(s) functionality #379

Uh oh!

themkat commented Aug 4, 2022 •

edited

Loading

Uh oh!

fwcd left a comment

Uh oh!

fwcd Aug 12, 2022

Uh oh!

themkat Aug 12, 2022

Uh oh!

fwcd Aug 12, 2022

Uh oh!

themkat Aug 12, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fwcd Aug 12, 2022

Uh oh!

themkat Aug 13, 2022

Uh oh!

themkat commented Aug 25, 2022 •

edited

Loading

Uh oh!

themkat commented Sep 8, 2022

Uh oh!

fwcd commented Sep 13, 2022

Uh oh!

themkat commented Sep 13, 2022

Uh oh!

Uh oh!

General override member(s) functionality #379

General override member(s) functionality #379

Uh oh!

Conversation

themkat commented Aug 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General info

Emacs screenshots

Uh oh!

fwcd left a comment

Choose a reason for hiding this comment

Uh oh!

fwcd Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

themkat Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

fwcd Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

themkat Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fwcd Aug 12, 2022

Choose a reason for hiding this comment

Uh oh!

themkat Aug 13, 2022

Choose a reason for hiding this comment

Uh oh!

themkat commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

themkat commented Sep 8, 2022

Uh oh!

fwcd commented Sep 13, 2022

Uh oh!

themkat commented Sep 13, 2022

Uh oh!

Uh oh!

themkat commented Aug 4, 2022 •

edited

Loading

themkat Aug 12, 2022 •

edited

Loading

themkat commented Aug 25, 2022 •

edited

Loading