Skip to content

Support definitions on JDK symbols #339

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

Merged
merged 6 commits into from
Apr 5, 2022
Merged

Conversation

daplf
Copy link
Contributor

@daplf daplf commented Mar 21, 2022

Related to #308

Provides support for definitions on JDK symbols.

Some notes:

  • I refactored our KLS URIs logic to allow fetching the JDK and its sources. This is achieved by extracting the JDK source files from ${JAVA_HOME}/lib/src.zip. Note that this will only work for JDK 9+, since the previous versions stored the source code in other locations. We could support those as well in the future (although I'm guessing most people working with Kotlin are already using JDK 11+).
  • The current mechanism just goes straight to the source code, rather than the bytecode of the JDK. In the future, we could provide access to the actual bytecode if we wanted (although I see little benefit in that). We could even decompile that code with Fernflower, although integrating with it might be trickier, since I'm not sure it supports JRT URLs or anything of the like. In any case, it would be doable with some effort.
  • referenceAtPoint currently fails to find declarations for the JDK. I spent some time investigating this, but I couldn't find the root cause. I'll explain my findings below anyway (perhaps someone knows why). In the meantime, I implemented a workaround (referenceExpressionAtPoint) that works pretty well for definitions. It doesn't work for other things (like completions and such), but we can keep using referenceAtPoint for now in those situations. When I have some more time, I'll try to investigate the issue some more, so we can hopefully use referenceAtPoint for everything.
  • I changed the compiler to expose the ModuleDescriptor directly, instead of the ComponentProvider since we were only using the latter to fetch the former.

So, why does referenceAtPoint fail? context.getSliceContents(BindingContext.REFERENCE_TARGET) returns an empty list on JDK symbols. This list is populated by the compiler, so it's difficult to understand what exactly leads to this, especially since it's a huge codebase with little documentation. As far as I can tell, the problem happens because we are using a fake file and we'not doing a full compilation of the file (referenceExpressionAtPoint works because it is using the current file's BindingContext instead of recompiling the expression). What's even more interesting is the fact that other features don't work with referenceExpressionAtPoint (e.g., completions). Anyway, I'll try to debug this some more. In the meantime, if you have any pointers or suggestions, let me know.

@fwcd fwcd added the enhancement New feature or request label Mar 21, 2022
Copy link
Owner

@fwcd fwcd left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks for digging into this!

@@ -87,4 +86,4 @@ fun goToDefinition(
return destination
}

private fun isInsideJar(uri: String) = uri.contains(".jar!")
private fun isInsideArchive(uri: String) = uri.contains("!")
Copy link
Owner

Choose a reason for hiding this comment

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

I am not sure if this is an edge case we've considered before, but can file names/path segments in URIs contain exclamation marks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I suppose we never considered that before. URIs could potentially contain exclamation marks, but they could also contain stuff like .jar! before an actual jar, so I guess this was sort of a problem before already. I guess in those cases, we would need a more robust mechanism for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the check to be more strict (thus allowing paths with exclamation marks). There are still some edge cases I can think of that won't work properly, but these were already a problem before, so I think we can keep it like this for now.

I think we should probably try to refactor the whole class content provider (and KLS URI) logic at some point, since it's getting a bit too confusing IMO.

@fwcd fwcd merged commit fd9df40 into fwcd:main Apr 5, 2022
@daplf daplf deleted the support-definitions-on-jdk branch April 14, 2022 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants