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

Add breakpoint support while debugging #1163

Merged
merged 21 commits into from Dec 19, 2019

Conversation

@marek1840
Copy link
Collaborator

marek1840 commented Dec 6, 2019

Previously, there were no possibility to add a breakpoint while debugging.
Now, this changes: user can add a breakpoint to scala or java file and expect the debugger to stop there, presenting the stack frames and their variables.

add-breakpoints-support

@marek1840 marek1840 changed the title Add breakpoint support while debuggin Add breakpoint support while debugging Dec 6, 2019
marek1840 added 2 commits Dec 10, 2019
Previously, when a message was created by the proxy
and not a client or server,it didn't have the id assigned. Since
the id is required, we have to set it manually and make sure that
the id assigned wouldn't collide with the ids of non-synthetic messages.
@marek1840 marek1840 force-pushed the marek1840:issue/1010/support-breakpoints branch from ad494d7 to 8deb077 Dec 10, 2019
Copy link
Member

olafurpg left a comment

I haven't reviewed in depth yet, just wanted to ask a quick question

@marek1840 marek1840 force-pushed the marek1840:issue/1010/support-breakpoints branch 7 times, most recently from 118487e to b51678b Dec 10, 2019
@marek1840 marek1840 force-pushed the marek1840:issue/1010/support-breakpoints branch from e23f8fd to aea3fd7 Dec 12, 2019
@marek1840 marek1840 marked this pull request as ready for review Dec 12, 2019
@marek1840 marek1840 requested review from olafurpg and tgodzik Dec 12, 2019
Copy link
Member

olafurpg left a comment

I just gave this a try in the mdoc repo and it's working great! Amazing job @marek1840 👏

I still haven't been able to review the entire PR but want to leave a few quick comments.

I'm struggling to get the "Variables" view to work as expected when running tests. It works great when running main methods

Screenshot 2019-12-15 at 11 39 24

But when I run a test I only get "x: "

Screenshot 2019-12-15 at 11 24 03

Any idea what might be going on?

@scala.annotation.tailrec
def qualifier(symbol: String, acc: List[String]): String = {
val owner = symbol.owner
if (owner == Symbols.RootPackage || owner == Symbols.EmptyPackage) {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

What about the case when owner == Symbols.None?

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 16, 2019

Author Collaborator

With your next suggestion (ownerChain) it should be covered

def toTypeSignature(definition: SymbolOccurrence): TypeSignature = {
import scala.meta.internal.semanticdb.Scala._
@scala.annotation.tailrec
def qualifier(symbol: String, acc: List[String]): String = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

Did you try using ownerChain? It's easy to end in infinite recursion when walking up owner chains

      import scala.meta.internal.semanticdb.Scala._
      "a/b/c#".ownerChain.map(_.desc.value)
res0: List[String] = List("_root_", "a", "b", "c")

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 16, 2019

Author Collaborator

Thanks, I didn't know the method!

@@ -411,6 +411,13 @@ object MetalsEnrichments

def toAbsolutePath: AbsolutePath =
AbsolutePath(Paths.get(URI.create(value.stripPrefix("metals:")))).dealias

def findAll(pattern: String): Iterator[Int] = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

Did you try using findAllMatchIn?

Screenshot 2019-12-15 at 11 33 17

@@ -31,14 +31,28 @@ final class WorkspaceSymbolProvider(
var inDependencies: ClasspathSearch =
ClasspathSearch.fromClasspath(Nil, bucketSize)

def search(query: String): Seq[l.SymbolInformation] = {
search(query, () => ())
def search(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

Are these changes used?

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 16, 2019

Author Collaborator

Not after switching to the DefinitionProvider, no. Thanks for catching - reverted.

}
}

def stackFrame(threadId: lang.Long): Future[StackFrame] = {

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

nit: let's use import java.lang{Long => JLong} instead

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 16, 2019

Author Collaborator

Good catch. In fact, we should use the Scala Long here.

import org.eclipse.lsp4j.jsonrpc.messages.ResponseMessage
import scala.collection.mutable

final class DummyServer(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 15, 2019

Member

For consistency with TestingServer and TestingClient?

Suggested change
final class DummyServer(
final class TestingDebugServer(

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 16, 2019

Author Collaborator

good catch. In fact, we should use the Scala Long here

@olafurpg

This comment has been minimized.

Copy link
Member

olafurpg commented Dec 15, 2019

Would it be possible to add explicit "debug" code lenses next to "run" and "test"? It would be nice to be able to ignore the breakpoints while when clicking on "run"/"test"

Co-Authored-By: Ólafur Páll Geirsson <olafurpg@gmail.com>
@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Dec 16, 2019

I'm struggling to get the "Variables" view to work as expected when running tests. It works great when running main methods

It was fixed in scalacenter/bloop#1115

@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Dec 16, 2019

Would it be possible to add explicit "debug" code lenses next to "run" and "test"? It would be nice to be able to ignore the breakpoints while when clicking on "run"/"test"

Certainly, I will look into it - possibly as a separate PR, since this one is already way too big.

Copy link
Member

olafurpg left a comment

I just tried again with the latest Bloop and can confirm that the variables display correctly now when running tests!

I tried adding a breakpoint in .metals/readonly/*.scala files and it Just Worked™️

I'm super impressed with the test infrastructure and the comprehensive test cases that also document current limitations. I only have a few minor suggestions to remove TODO comments, otherwise I believe this PR is ready to go 🚀

adapters: MetalsDebugAdapters
)(implicit ec: ExecutionContext) {

// TODO the partitioning does not work for cases when the same file has other types defined

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 16, 2019

Member

I'm not sure I understand this TODO, could we open a followup ticket?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 18, 2019

Member

@marek1840 can we link to the issue here?

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 18, 2019

Author Collaborator

done!

)
)

assertProjectBreakpoints("not-matching-filename")(

This comment has been minimized.

Copy link
@olafurpg

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 16, 2019

Member

So much blood, sweat and tears to deliver this functionality 💪

|""".stripMargin
)

// TODO investigate what happens

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 16, 2019

Member

Let's open followup ticket

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 18, 2019

Member

Did we open a followup ticket?

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 18, 2019

Author Collaborator

Nope, I was iterating over a List() which is empty, hence the breakpoint was not effective

)
)

def assertStackFrame(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 16, 2019

Member

Can we add a test case with symbolic identifiers? I noticed that :: was displayed as $colon$colon in the variable view

Screenshot 2019-12-16 at 11 03 07

No need to change the current behavior, just document it with a test case.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 18, 2019

Member

Did we add a test case with symbolic identifier?

instrument = steps =>
steps
.at("a/src/main/scala/Main.scala", line = 5)(StepOut)
.at("a/src/main/scala/Main.scala", line = 9)(Continue)

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 16, 2019

Member

Could we avoid hardcoding the line numbers here somehow? For example, using comment markers

             |>>  println() // debug: step 1
             
// ...
           |    foo() // debug: step 2
// ...
                .at("a/src/main/scala/Main.scala", line = "step 1")(StepOut)
                .at("a/src/main/scala/Main.scala", line = "step 2")(Continue)

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 17, 2019

Author Collaborator

That would be problematic for cases when a single line is hit multiple times. Also, developer would have to either declare the methods in order of breakpoints or somehow indicate the order within the commit markers which I think would be less readable than having all of the steps in one place. Also, we would need to encode the step type in the comment and it all becomes not worth the hassle in my opinion.
The test cases are small enough that they shouldn't be changed that often, so let's hope we won't have too many reasons to do so.

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 17, 2019

Member

Fair enough, as a general principle I try to avoid hardcoded line numbers in test cases because the line numbers change when you change the code.

Copy link
Collaborator

tgodzik left a comment

Looks really promising 🎉 , just a couple of comments.

s"""|/metals.json
|{ "a": {}, "b": {} }
|
|${fileLayouts.map(_.layout).mkString("\n")}

This comment has been minimized.

Copy link
@tgodzik

tgodzik Dec 16, 2019

Collaborator

Can we reuse FileLayout here?

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 18, 2019

Member

Can you elaborate?

@marek1840 marek1840 force-pushed the marek1840:issue/1010/support-breakpoints branch from b463c21 to 201d98b Dec 17, 2019
marek1840 added 3 commits Dec 17, 2019
@marek1840 marek1840 force-pushed the marek1840:issue/1010/support-breakpoints branch from 7147ca5 to d3196b5 Dec 17, 2019
@tgodzik tgodzik requested review from tgodzik and olafurpg Dec 17, 2019
@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Dec 17, 2019

Looks like mill tests are still flaky.

Copy link
Member

olafurpg left a comment

Just a few clarifying questions. Otherwise this PR is ready to go feature-wise! I would ideally like to either remove all TODO comments or associate every TODO comment with a link to a github issue.

adapters: MetalsDebugAdapters
)(implicit ec: ExecutionContext) {

// TODO the partitioning does not work for cases when the same file has other types defined

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 18, 2019

Member

@marek1840 can we link to the issue here?

|""".stripMargin
)

// TODO investigate what happens

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 18, 2019

Member

Did we open a followup ticket?

s"""|/metals.json
|{ "a": {}, "b": {} }
|
|${fileLayouts.map(_.layout).mkString("\n")}

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 18, 2019

Member

Can you elaborate?

)
)

def assertStackFrame(

This comment has been minimized.

Copy link
@olafurpg

olafurpg Dec 18, 2019

Member

Did we add a test case with symbolic identifier?

@marek1840

This comment has been minimized.

Copy link
Collaborator Author

marek1840 commented Dec 18, 2019

I have associated all of the remaining TODOs with their github issues so that the places which need to be changed are easy to find.

Copy link
Member

olafurpg left a comment

Thank you @marek1840! LGTM 🎉

val path = adapters.adaptPath(request.getSource.getPath)
val input = path.toAbsolutePath.toInput
val occurrences = Mtags.allToplevels(input).occurrences
val groups = request.getBreakpoints.groupBy { breakpoint =>

This comment has been minimized.

Copy link
@jvican

jvican Dec 18, 2019

Collaborator

Are we sending fqcns only for symbols from third-party deps? Or are we sending fqcns for symbols for Java and Scala sources defined in the project? Are we following the recommendation in the PR introducing support for breakpoints in Bloop?

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 18, 2019

Author Collaborator

We are avoiding extra branching by not checking whether the source is defined within the workspace or is a part of a dependency. We are confident this approach is working thanks to the extensive suite of tests we provide alongside. If you know a case when this will fail, please don't hesitate to point to it.

Or did you have another recommendation in mind?

This comment has been minimized.

Copy link
@jvican

jvican Dec 18, 2019

Collaborator

My recommendation is not to send dap-fqcn for files defined inside user-defined projects. It should only be used for third-party dependencies.

Bloop and Zinc are the source of truth here. Regardless of how confident we're from Metals doing the right thing, we should be relying on the source of truth if that's possible to avoid issues/discrepancies in the future.

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 19, 2019

Author Collaborator

Fair enough. For now, though, we will rely on the compiler to emit LineNumberTable according to the JVM spec.
If the compiler, at any point, proves to be unreliable by messing the line numbers, then I believe neither bloop nor zinc would help us.

This comment has been minimized.

Copy link
@jvican

jvican Dec 19, 2019

Collaborator

I feel pretty strongly this is not the way to go. You should not be reimplementing this part of the logic. Metals should be reusing the breakpoint logic for user-defined sources implemented in Bloop.

This comment has been minimized.

Copy link
@marek1840

marek1840 Dec 19, 2019

Author Collaborator

The logic you say I am reimplementing is required for the library sources anyway, since bloop solution works only for workspace sources.
Since it is required either way, right now, I don't see a reason for falling back to bloop and not using it also for user-defined sources. It does not mean there is no such reason but only that I don't see it currently - can you help me in this regard?

This comment has been minimized.

Copy link
@jvican

jvican Dec 19, 2019

Collaborator

The logic you say I am reimplementing is required for the library sources anyway, since bloop solution works only for workspace sources.

Yes, the whole reason why we have dap-fqcn is so that IDEs that manage library sources can pass in the resolved fully qualified class names.

I don't see a reason for falling back to bloop and not using it also for user-defined sources. It does not mean there is no such reason but only that I don't see it currently - can you help me in this regard?

Yes, the point I'm trying to make is that the logic for library sources is necessary since Bloop doesn't support it. But the logic for workspace sources isn't. There is a reference implementation that it's as good as it gets: it's using the same resolution mechanism the JVM uses for debugging (what's happening under the hood is that the JVM gets our fully qualified class name we pass it and undoes some of the work Bloop has done and finds the class file the loaded class was loaded from in the classloader).

At a high level, the process looks like this:

Bloop: $WORKSPACE_SOURCE_FILE -> $WORKSPACE_CLASS_FILE -> $FQCN ->
JVM: $FQCN -> $WORKSPACE_CLASS_FILE

The JVM uses the LineNumberTable of the $WORKSPACE_CLASS_FILE to correctly set the breakpoint. This is why it's so important that WORKSPACE_CLASS_FILE matches the one the JVM picks and why the Bloop implementation is the source of the truth -- because it mimics exactly the logic of the JVM.

What Metals is doing here might be correct based on the semantic-level information you have. But it's creating a layer of indirection that should be avoided as it could produce issues or inconsistent results in the future. I think it's a good practice to rely on the source of truth if it's possible and in this case it is.

Copy link
Collaborator

tgodzik left a comment

LGTM! Looks cool, looking forward to checking this out 😄

@marek1840 marek1840 merged commit 239b639 into scalameta:master Dec 19, 2019
12 checks passed
12 checks passed
windows-latest unit tests
Details
macOS-latest unit tests
Details
ubuntu-latest unit tests
Details
ubuntu-latest unit tests
Details
Sbt integration
Details
Maven integration
Details
Gradle integration
Details
Mill integration
Details
Pants integration
Details
LSP integration tests
Details
Scala cross tests
Details
Scalafmt/Scalafix/Docs
Details
@marek1840 marek1840 deleted the marek1840:issue/1010/support-breakpoints branch Dec 19, 2019
@esHack

This comment has been minimized.

Copy link

esHack commented Dec 19, 2019

It would be great if we some documentation on how to run debugger using metals

@tgodzik

This comment has been minimized.

Copy link
Collaborator

tgodzik commented Dec 19, 2019

It would be great if we some documentation on how to run debugger using metals

There will be additional code lenses for debugging with breakpoints, we also need to merge the VS Code extension PR for this to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.