Implement 'find in JAR files' LSP extension #3093

Z1kkurat · 2021-09-01T10:45:21Z

This is an attempt to implement this functionality: scalameta/metals-feature-requests#196
The implementation is very naive and just scans through all workspace jars, reading files by mask and searching for a full content match. As was discussed on Discord with @olafurpg, this can be tolerated as the initial implementation.

To test this I implemented client-side functionality in metals-vscode using a separate custom tree view just to showcase that the functionality itself works. I think I will attempt to make a PR with it once I clean it up (the code is very bad and dirty right now).

dos65

Demonstration looks awesome! Thanks for this contribution!

The approach in general looks good.
The only thing is that I'm not sure if it should be named Find in non-source files.
Maybe Search in dependency sources? Anyway let's wait what others will say.

I left some suggestions to the code:

metals/src/main/scala/scala/meta/internal/metals/FindNonSourceTextRequest.scala

metals/src/main/scala/scala/meta/internal/metals/FindTextInFiles.scala

dos65 · 2021-09-01T11:29:56Z

metals/src/main/scala/scala/meta/internal/metals/FindTextInFiles.scala

+    if (mask == null || content == null) {
+      List.empty[Location]


Probably null checking might done earlier. Somewhere at the moment of extracting parameters

Probably an annotation would work there?

You mean javax.annotation.Nonnull? I didn't see its usages in metals, will it work?

metals/src/main/scala/scala/meta/internal/metals/FindTextInFiles.scala

dos65 · 2021-09-01T12:11:05Z

tests/unit/src/test/scala/tests/FindTextInFilesSuite.scala

+      locations <- server.findTextInFiles(".conf", "jvm-shutdown-hooks")
+      _ = assertEquals(locations, expectedLocations)


In metals tests, usually received and expected results are rendered into String representation that is more readable.
Could you do assert in a similar way to how it's done in WorkspaceSymbolLspSuite?

metals/tests/unit/src/test/scala/tests/WorkspaceSymbolLspSuite.scala

Lines 160 to 198 in 55161a5

optionSourcePath = server

.toPath("scala/Option.scala")

.toRelative(workspace)

.toString

.replace("\\", "/")

_ = assertNoDiff(

optionReferences,

s"""|a/src/main/scala/a/A.scala:4:24: info: reference

| val x: Option[Int] = None

| ^^^^

|$optionSourcePath:29:50: info: reference

| def apply[A](x: A): Option[A] = if (x == null) None else Some(x)

| ^^^^

|$optionSourcePath:34:30: info: reference

| def empty[A] : Option[A] = None

| ^^^^

|$optionSourcePath:230:18: info: reference

| if (isEmpty) None else Some(f(this.get))

| ^^^^

|$optionSourcePath:271:18: info: reference

| if (isEmpty) None else f(this.get)

| ^^^^

|$optionSourcePath:274:18: info: reference

| if (isEmpty) None else ev(this.get)

| ^^^^

|$optionSourcePath:289:43: info: reference

| if (isEmpty || p(this.get)) this else None

| ^^^^

|$optionSourcePath:304:44: info: reference

| if (isEmpty || !p(this.get)) this else None

| ^^^^

|$optionSourcePath:432:42: info: reference

| if (!isEmpty) pf.lift(this.get) else None

| ^^^^

|$optionSourcePath:527:13: info: reference

|case object None extends Option[Nothing] {

| ^^^^

|""".stripMargin

)

Not yet done, moved file so this change is not really outdated, will fix this

tgodzik

Looks great!

I wonder though if it wouldn't be a better UX if we had just one QuickPick displayed that would contain all text files (we could cache them), we can than easily filter those in the quick pick window by writing .conf. That would mean we would need to search for a specific string later on, so maybe that wouldn't be better 🤔

tgodzik · 2021-09-01T15:16:52Z

metals/src/main/scala/scala/meta/internal/metals/FindTextInFiles.scala

+    } else {
+      val allLocations: ArrayBuffer[Location] = new ArrayBuffer[Location]()
+
+      buildTargets.allWorkspaceJars.foreach { classpathEntry =>


I think we could cache text file paths when indexing, which would make it all much faster and wouldn't take up much more memory.

So essentially an index of jar path -> list of files inside a jar? Would that not be huge (since there are lots of classfiles etc)?

tgodzik · 2021-09-01T15:20:24Z

metals/src/main/scala/scala/meta/internal/metals/FindNonSourceTextRequest.scala

+package scala.meta.internal.metals
+
+case class FindNonSourceTextRequest(
+    mask: java.lang.String,


Do we need a mask really? Jars will contain .conf files and .properties most likely, so not much possible values there.

I think there are more plain-text extensions that might be presented in jars and we will never cover them all.
Also the mask might be used not only for filtering by extension but also just by full name like: sample.json.

I did it by advice from @dos65 and I guess he has a point - some people put xml/json/txt files in jars, so who knows what a user may need to search for

tgodzik · 2021-09-01T15:29:48Z

To test this I implemented client-side functionality in metals-vscode using a separate custom tree view just to showcase that the functionality itself works. I think I will attempt to make a PR with it once I clean it up (the code is very bad and dirty right now).

What if we unpack all the matched files to .metals/dependencies and run search in VS Code on those files? That would mean we wouldn't need a new view.

Z1kkurat · 2021-09-01T16:18:44Z

What if we unpack all the matched files to .metals/dependencies and run search in VS Code on those files? That would mean we wouldn't need a new view.

Hmm, I never thought of that, a good point! But here we need to estimate the performance impact, I guess? Copying (potentially) many files to dependencies directory (and no guarantees that we will find required content there) versus reading files in-place. I have nothing against both solutions, but I guess they are different in their nature of the interaction with the editor - copying is a side-effect which returns nothing and requires executing the editor command later and in-place search is a plain function which returns a list of locations so editor client can update some internal view representation.
Do you think copying would be a better approach?

I wonder though if it wouldn't be a better UX if we had just one QuickPick displayed that would contain all text files (we could cache them), we can than easily filter those in the quick pick window by writing .conf. That would mean we would need to search for a specific string later on, so maybe that wouldn't be better 🤔

Initially, I wanted to provide a 'peek' view, similar to the one used in 'find references', but VSCode API said no 😄 - it required peeking from some specific file location. TreeView looks okay I think - it gives both the file name and a preview of found entries. But I also don't mind coming to a consensus for the best solution. And other editors will be able to implement it the way they see fit for themselves anyway 😄

Z1kkurat · 2021-09-03T13:42:05Z

A task list for myself:

fix equality assertion in suite
(possibly?) index for jar contents per Tomasz's suggestion - need to research this
discuss the possibility of copying files with opening editor native search later instead of scanning and reading files

dos65 · 2021-09-03T15:18:49Z

It's too late, but I realized that we need to check if vscode is able to search in virtual file systems.
I took a look at vscode sources and probably it can do that.

This feature is anyway requires support from the editor side so maybe we can do that using std vscode api.

Z1kkurat · 2021-09-03T15:21:57Z

It's too late, but I realized that we need to check if vscode is able to search in virtual file systems.
I took a look at vscode sources and probably it can do that.

This feature is anyway requires support from the editor side so maybe we can do that using std vscode api.

In my showcase implementation I used only standard vscode API for opening files and showing them, nothing fancy, so I guess it should work?

Z1kkurat · 2021-09-03T15:28:09Z

By the way, here it is - very unpolished and not yet ready for PR to the main repo, so I made a PR to the main branch of my fork so changes can be easily seen:
Branch: https://github.com/Z1kkurat/metals-vscode/tree/find-files-in-dependency-jars
PR: Z1kkurat/metals-vscode#1

dos65 · 2021-09-03T15:41:59Z

@Z1kkurat I'm about a different thing. There is registerFileSystemProvider function in workspace api. I'm wondering if we can make a custom one for dependencies and vscode will be able to search trough it.

Z1kkurat · 2021-09-06T09:33:17Z

@dos65 can you please elaborate? I'm not very familiar with vscode API, so I guess we will need to index such dependency files to present them as a filesystem tree, since my current implementation scans files on demand?

dos65 · 2021-09-14T10:26:24Z

I checked vscode api. There are plans to expose interfaces to make custom search providers but it still in proposed status

I think the solution in this pr is quite good and cover this missing feature for some users who are switching from IDEA to Metals.
I don't think that there is a need in indexing for this search as it should be fast enough plus I don't think that it will be used often.
The only issue might happen with large files but I think we can add a limit on filesize.

@tgodzik @ckipp01 wdyt? Could we agree on introducing this new endpoint and push this feature further?

ckipp01

Great job on this @Z1kkurat, as @dos65 said I think this is a big feature to help those transitioning from IntelliJ. I know I would use this feature.

@tgodzik @ckipp01 wdyt? Could we agree on introducing this new endpoint and push this feature further?

👍🏼 from me

metals/src/main/scala/scala/meta/internal/metals/FindTextInDependencyJarsRequest.scala

olafurpg · 2021-09-20T06:31:14Z

I checked vscode api. There are plans to expose interfaces to make custom search providers but it still in proposed status

Can we confirm that the current extension will be usable with the new proposed text search API? It would be a shame if we have to change the JSON-RPC endpoints to make it work with the new text search API once it’s available.

tgodzik

Thanks for the great work! I finally managed to dig into the PR and think about it. I left a bunch of comments, which might be a bit overwhelming, so if you need help let me know I would be happy to work on this a bit! We don't have to implement everything in this PR for sure,

tgodzik · 2021-09-24T13:58:19Z

metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala

@@ -1891,6 +1894,46 @@ class MetalsLanguageServer(
        .orNull
    }.asJava

+  @JsonRequest("metals/findTextInDependencyJars")


We should document it similar to docs/integrations/tree-view-protocol.md

+1 on this. Before merging this in lets get a basic section on this in the new-editor.md if possible. Not sure it needs to be a full page on its own since it's quite small, but having it documented somewhere that it exists, what it expects, and what it returns in important for other clients to know how to implement this.

@tgodzik do you mind if I do it as a separate PR? This one is quite old already and has more than 40 comments, so I would prefer to finally merge it when others approve.

Sure, not an issue. Let me know if you don't manage to do it

Sorry was mid review and didn't see this before. We can merge and do a follow-up if you'd prefer, but I do think it's pretty important to get this documented. If not others have to dig into the code to know how to implement it.

metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala

metals/src/main/scala/scala/meta/internal/metals/FindTextInDependencyJarsRequest.scala

metals/src/main/scala/scala/meta/internal/metals/FindTextInDependencyJars.scala

metals/src/main/scala/scala/meta/internal/metals/FindTextInDependencyJarsRequest.scala

Z1kkurat · 2021-09-27T13:19:06Z

Wow, that's a lot of feedback, thank you very much @tgodzik ! 😄
I am planning to come back to it somewhere during this week, but I'm not sure I will have enough time to address all of the comments, so I would definitely appreciate your help on that after I address as many of them as I can.

Z1kkurat · 2021-10-06T20:56:42Z

@tgodzik I think that I addressed most of the comments. Please let me know if there's anything else on your mind regarding this PR or if you notice that I have missed something!

tgodzik

Looks amazing! With a properly specified API we can improve it later on when the text search API becomes available. Do you have a PR for VS Code ready?

We can merge this for sure, will just leave it for a bit for others to comment if they want.

Z1kkurat · 2021-10-08T09:55:16Z

@tgodzik Thank you!

Do you have a PR for VS Code ready?

Honestly, it is more of a PoC implementation. I made it to my fork and it has these peculiarities:

it uses a separate view (not inside metals' one, as can be seen from GIF in the first post of this thread) because the views which are created by treeviews.startTreeView are configured in a specific way to interact with a language client in a specific protocol with server notifications, so I couldn't figure out that part quickly and created a separate view which is manually controlled
as a consequence, it uses the same metals icon 😄

I've never encountered any vscode APIs previously, so I might've just misused them or haven't got the idea of what's going on properly. Here's the link if you are interested: Z1kkurat/metals-vscode#1.
But, again, I might have done everything wrong in there 😆

tgodzik · 2021-10-08T10:47:16Z

It should be ok to create a WiP PR for VS Code and iterate over that. Would you be interested in working on it? We can have a separate treeview I think, but maybe it would make sense to use the search icon?

Z1kkurat · 2021-10-08T13:09:46Z

Yes, I definitely would be. PR is created: scalameta/metals-vscode#719

dos65 · 2021-10-08T13:58:45Z

metals/src/main/scala/scala/meta/internal/metals/findfiles/FindTextInDependencyJars.scala

+          buildTargets.allWorkspaceJars.foreach { classpathEntry =>
+            try {
+              val jarLocations: List[Location] =
+                if (classpathEntry.isFile && classpathEntry.isJar) {


I've tried this locally on metals repo with following parameters: *.java + startsWith and got an empty response.
You need to include JdkSources to all jars and allow .zip files.

Done! Turned out the same jar-related API worked on JDK zip just fine. Although I'm not sure about tests - I guess almost anything we can search for in the JDK sources would be repeated tens or hundreds of times there 😄 Besides, the test will depend on the specific JDK version since sources will change accordingly

I think searching for a constructor of in a old public class should work stable and have only a single occurrence.
Maybe public String(StringBuffer buffer) { ?

Z1kkurat · 2021-10-08T14:01:59Z

Added a small fix to create PathMatcher for include/exclude only once instead of doing it for every checked file

This is needed for the functionality added in scalameta/metals#3093.

ckipp01 · 2021-10-08T14:42:04Z

🚀 working in nvim-metals.

dos65

@Z1kkurat Awesome work. Thx!

Z1kkurat · 2021-10-08T14:46:09Z

@ckipp01 you implemented that way too fast 😄 Amazing! And it works not too bad, considering that you were searching for a quite common word. Can you please try it with my latest commit with JDK sources scanning? I wonder if you would have to wait for a minute to get results from that...

Z1kkurat · 2021-10-08T15:25:36Z

tests/unit/src/test/scala/tests/FindTextInDependencyJarsSuite.scala

+      workspace
+        .resolve(Directories.dependencies)
+        .resolve("src.zip")
+        .resolve("java.base")


Ooh, I think this won't work on JDK8... @dos65 WDYT about that?

Damn!
For jdk8 you need skip resolve("java.base")

ckipp01 · 2021-10-08T20:12:27Z

I wonder if you would have to wait for a minute to get results from that.

Honestly the time difference wasn't that noticeable.

ckipp01

LGTM! Great job on the @Z1kkurat!

ckipp01 · 2021-10-08T20:14:45Z

I'll go ahead and merge seeing that we have 3 ✅ , although please do follow up and add some docs @Z1kkurat.

dos65 requested changes Sep 1, 2021

View reviewed changes

tgodzik reviewed Sep 1, 2021

View reviewed changes

ckipp01 reviewed Sep 15, 2021

View reviewed changes

metals/src/main/scala/scala/meta/internal/metals/FindTextInDependencyJarsRequest.scala Outdated Show resolved Hide resolved

tgodzik requested changes Sep 24, 2021

View reviewed changes

Z1kkurat added 5 commits October 6, 2021 11:42

Implement find in files LSP extension

56bf358

Fix formatting

103add7

Organize imports

a021707

Addressed some of comments

24d87c6

Fix fmt

6c4a7f8

Z1kkurat force-pushed the feature/find-in-files branch from e9b29a2 to 6c4a7f8 Compare October 6, 2021 09:47

Z1kkurat added 2 commits October 6, 2021 20:29

Address review comments

849d8f2

Use glob syntax

74af023

tgodzik approved these changes Oct 8, 2021

View reviewed changes

Z1kkurat mentioned this pull request Oct 8, 2021

Implement VSCode side of 'find text in dependency JARs' scalameta/metals-vscode#719

Merged

Don't create unnecessary PathMatcher

43a44fb

dos65 reviewed Oct 8, 2021

View reviewed changes

Include JDK zip sources

427937f

ckipp01 added a commit to ckipp01/nvim-metals that referenced this pull request Oct 8, 2021

Basic findTextInDependencyJars implemented.

a3dd7f7

This is needed for the functionality added in scalameta/metals#3093.

ckipp01 added a commit to ckipp01/nvim-metals that referenced this pull request Oct 8, 2021

Basic findTextInDependencyJars implemented.

df1dbb9

This is needed for the functionality added in scalameta/metals#3093.

ckipp01 mentioned this pull request Oct 8, 2021

Basic findTextInDependencyJars implemented. scalameta/nvim-metals#247

Merged

dos65 approved these changes Oct 8, 2021

View reviewed changes

Add searching in JDK lib

1c3187d

Z1kkurat commented Oct 8, 2021

View reviewed changes

Z1kkurat added 2 commits October 8, 2021 17:46

Fix JDK check in test

3239879

Add test case for JDK 17

fe9bb0f

ckipp01 approved these changes Oct 8, 2021

View reviewed changes

ckipp01 merged commit 5286568 into scalameta:main Oct 8, 2021

ckipp01 mentioned this pull request Oct 12, 2021

Add support for findTextInDependencyJars. scalameta/coc-metals#433

Open

ayoub-benali mentioned this pull request Nov 14, 2021

Implement metals/findTextInDependencyJars scalameta/metals-sublime#47

Closed

dos65 mentioned this pull request Mar 11, 2022

FindTextInJars - implement search rules to support all fields from TextSearchQuery scalameta/metals-feature-requests#261

Open

		locations <- server.findTextInFiles(".conf", "jvm-shutdown-hooks")
		_ = assertEquals(locations, expectedLocations)

	optionSourcePath = server
	.toPath("scala/Option.scala")
	.toRelative(workspace)
	.toString
	.replace("\\", "/")
	_ = assertNoDiff(
	optionReferences,
	s"""\|a/src/main/scala/a/A.scala:4:24: info: reference
	\| val x: Option[Int] = None
	\| ^^^^
	\|$optionSourcePath:29:50: info: reference
	\| def apply[A](x: A): Option[A] = if (x == null) None else Some(x)
	\| ^^^^
	\|$optionSourcePath:34:30: info: reference
	\| def empty[A] : Option[A] = None
	\| ^^^^
	\|$optionSourcePath:230:18: info: reference
	\| if (isEmpty) None else Some(f(this.get))
	\| ^^^^
	\|$optionSourcePath:271:18: info: reference
	\| if (isEmpty) None else f(this.get)
	\| ^^^^
	\|$optionSourcePath:274:18: info: reference
	\| if (isEmpty) None else ev(this.get)
	\| ^^^^
	\|$optionSourcePath:289:43: info: reference
	\| if (isEmpty \|\| p(this.get)) this else None
	\| ^^^^
	\|$optionSourcePath:304:44: info: reference
	\| if (isEmpty \|\| !p(this.get)) this else None
	\| ^^^^
	\|$optionSourcePath:432:42: info: reference
	\| if (!isEmpty) pf.lift(this.get) else None
	\| ^^^^
	\|$optionSourcePath:527:13: info: reference
	\|case object None extends Option[Nothing] {
	\| ^^^^
	\|""".stripMargin
	)

Implement 'find in JAR files' LSP extension #3093

Implement 'find in JAR files' LSP extension #3093

Conversation

Z1kkurat commented Sep 1, 2021

dos65 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgodzik commented Sep 1, 2021

Z1kkurat commented Sep 1, 2021

Z1kkurat commented Sep 3, 2021 • edited Loading

dos65 commented Sep 3, 2021

Z1kkurat commented Sep 3, 2021

Z1kkurat commented Sep 3, 2021

dos65 commented Sep 3, 2021

Z1kkurat commented Sep 6, 2021

dos65 commented Sep 14, 2021

ckipp01 left a comment

Choose a reason for hiding this comment

olafurpg commented Sep 20, 2021

tgodzik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Z1kkurat commented Sep 27, 2021

Z1kkurat commented Oct 6, 2021

tgodzik left a comment

Choose a reason for hiding this comment

Z1kkurat commented Oct 8, 2021

tgodzik commented Oct 8, 2021

Z1kkurat commented Oct 8, 2021

dos65 Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Z1kkurat commented Oct 8, 2021

ckipp01 commented Oct 8, 2021

dos65 left a comment

Choose a reason for hiding this comment

Z1kkurat commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckipp01 commented Oct 8, 2021

ckipp01 left a comment

Choose a reason for hiding this comment

ckipp01 commented Oct 8, 2021

Z1kkurat commented Sep 3, 2021 •

edited

Loading

dos65 Oct 8, 2021 •

edited

Loading