Debugger: Fix step over in functions with default parameters (KT-14828)

Adds a synthetic line number just before the original function call. The new line number is recognized by the debugger which replaces the 'step over' action with 'step into' and stops.
2020-02-05 19:05:42 +09:00
parent d0f34624bd
commit 4244f05307
21 changed files with 357 additions and 19 deletions
@@ -47,13 +47,20 @@ class KotlinSyntheticTypeComponentProvider : SyntheticTypeComponentProvider {
    }

    override fun isNotSynthetic(typeComponent: TypeComponent?): Boolean {
-        if (typeComponent is Method && typeComponent.name().endsWith(SUSPEND_IMPL_NAME_SUFFIX)) {
-            if (typeComponent.location()?.isInKotlinSources() == true) {
-                val containingClass = typeComponent.declaringType()
-                if (typeComponent.argumentTypeNames().firstOrNull() == containingClass.name()) {
-                    // Suspend wrapper for open method
-                    return true
+        if (typeComponent is Method) {
+            val name = typeComponent.name()
+
+            if (name.endsWith(SUSPEND_IMPL_NAME_SUFFIX)) {
+                if (typeComponent.location()?.isInKotlinSources() == true) {
+                    val containingClass = typeComponent.declaringType()
+                    if (typeComponent.argumentTypeNames().firstOrNull() == containingClass.name()) {
+                        // Suspend wrapper for open method
+                        return true
+                    }
                }
+            } else if (name.endsWith(JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX)) {
+                val originalName = name.dropLast(JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX.length)
+                return typeComponent.declaringType().methodsByName(originalName).isNotEmpty()
            }
        }

@@ -34,4 +34,8 @@ object CoroutineBreakpointFacility : AbstractCoroutineBreakpointFacility() {

        return true
    }
+}
+
+fun SuspendContextImpl.getLocationCompat(): Location? {
+    return this.location
 }
@@ -11,6 +11,7 @@ import com.intellij.debugger.ui.breakpoints.RunToCursorBreakpoint
 import com.sun.jdi.Location
 import com.sun.jdi.Method
 import com.sun.jdi.event.LocatableEvent
+import org.jetbrains.kotlin.idea.debugger.safeLocation

 object CoroutineBreakpointFacility : AbstractCoroutineBreakpointFacility() {
    override fun installCoroutineResumedBreakpoint(context: SuspendContextImpl, location: Location, method: Method): Boolean {
@@ -38,4 +39,8 @@ object CoroutineBreakpointFacility : AbstractCoroutineBreakpointFacility() {

        return true
    }
+}
+
+fun SuspendContextImpl.getLocationCompat(): Location? {
+    return this.frameProxy?.safeLocation()
 }
@@ -6,6 +6,7 @@
 package org.jetbrains.kotlin.idea.debugger.stepping

 import com.intellij.debugger.engine.DebugProcessImpl
+import com.intellij.debugger.engine.MethodFilter
 import com.intellij.debugger.engine.SuspendContextImpl
 import org.jetbrains.kotlin.idea.debugger.stepping.filter.KotlinStepOverFilter
 import org.jetbrains.kotlin.idea.debugger.stepping.filter.LocationToken
@@ -18,6 +19,12 @@ sealed class KotlinStepAction {
        }
    }

+    class StepInto(private val filter: MethodFilter?) : KotlinStepAction() {
+        override fun apply(debugProcess: DebugProcessImpl, suspendContext: SuspendContextImpl, ignoreBreakpoints: Boolean) {
+            debugProcess.createStepIntoCommand(suspendContext, ignoreBreakpoints, filter).contextAction(suspendContext)
+        }
+    }
+
    object StepOut : KotlinStepAction() {
        override fun apply(debugProcess: DebugProcessImpl, suspendContext: SuspendContextImpl, ignoreBreakpoints: Boolean) {
            debugProcess.createStepOutCommand(suspendContext).contextAction(suspendContext)
@@ -22,12 +22,15 @@ import com.intellij.debugger.engine.SuspendContextImpl
 import com.intellij.debugger.engine.evaluation.EvaluateException
 import com.intellij.debugger.jdi.ThreadReferenceProxyImpl
 import com.intellij.openapi.diagnostic.Logger
+import com.sun.jdi.Location
 import com.sun.jdi.VMDisconnectedException
 import com.sun.jdi.request.StepRequest
 import org.jetbrains.kotlin.idea.debugger.isOnSuspensionPoint
 import org.jetbrains.kotlin.idea.debugger.safeLineNumber
 import org.jetbrains.kotlin.idea.debugger.safeLocation
 import org.jetbrains.kotlin.idea.debugger.safeMethod
+import org.jetbrains.kotlin.load.java.JvmAbi
+import org.jetbrains.org.objectweb.asm.Type

 // Originally copied from RequestHint
 class KotlinStepOverRequestHint(
@@ -39,6 +42,18 @@ class KotlinStepOverRequestHint(
        private val LOG = Logger.getInstance(KotlinStepOverRequestHint::class.java)
    }

+    private class LocationData(val method: String, val signature: Type, val declaringType: String) {
+        companion object {
+            fun create(location: Location?): LocationData? {
+                val method = location?.safeMethod() ?: return null
+                val signature = Type.getMethodType(method.signature())
+                return LocationData(method.name(), signature, location.declaringType().name())
+            }
+        }
+    }
+
+    private val startLocation = LocationData.create(suspendContext.getLocationCompat())
+
    override fun getNextStepDepth(context: SuspendContextImpl): Int {
        try {
            val frameProxy = context.frameProxy ?: return STOP
@@ -53,7 +68,13 @@ class KotlinStepOverRequestHint(
                val isAcceptable = location != null && filter.locationMatches(context, location)
                return if (isAcceptable) STOP else StepRequest.STEP_OVER
            } else if (isSteppedOut) {
-                val lineNumber = frameProxy.safeLocation()?.safeLineNumber(JAVA_STRATUM) ?: -1
+                val location = frameProxy.safeLocation()
+                val method = location?.safeMethod()
+                if (method != null && method.isSyntheticMethodForDefaultParameters() && isSteppedFromDefaultParamsOriginal(location)) {
+                    return StepRequest.STEP_OVER
+                }
+
+                val lineNumber = location?.safeLineNumber(JAVA_STRATUM) ?: -1
                return if (lineNumber >= 0) STOP else StepRequest.STEP_OVER
            }

@@ -66,6 +87,45 @@ class KotlinStepOverRequestHint(
        return STOP
    }

+    private fun isSteppedFromDefaultParamsOriginal(location: Location): Boolean {
+        val startLocation = this.startLocation ?: return false
+        val endLocation = LocationData.create(location) ?: return false
+
+        if (startLocation.declaringType != endLocation.declaringType) {
+            return false
+        }
+
+        if (startLocation.method + JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX != endLocation.method) {
+            return false
+        }
+
+        val startArgs = startLocation.signature.argumentTypes
+        val endArgs = endLocation.signature.argumentTypes
+
+        if (startArgs.size >= endArgs.size) {
+            // Default params function should always have at least one additional flag parameter
+            return false
+        }
+
+        for ((index, type) in startArgs.withIndex()) {
+            if (endArgs[index] != type) {
+                return false
+            }
+        }
+
+        for (index in startArgs.size until (endArgs.size - 1)) {
+            if (endArgs[index].sort != Type.INT) {
+                return false
+            }
+        }
+
+        if (endArgs[endArgs.size - 1].descriptor != "Ljava/lang/Object;") {
+            return false
+        }
+
+        return true
+    }
+
    private fun installCoroutineResumedBreakpoint(context: SuspendContextImpl): Boolean {
        val frameProxy = context.frameProxy ?: return false
        val location = frameProxy.safeLocation() ?: return false
@@ -15,20 +15,19 @@ import com.intellij.debugger.impl.JvmSteppingCommandProvider
 import com.intellij.debugger.jdi.StackFrameProxyImpl
 import com.intellij.debugger.settings.DebuggerSettings
 import com.intellij.psi.PsiElement
-import com.sun.jdi.AbsentInformationException
-import com.sun.jdi.LocalVariable
-import com.sun.jdi.Location
-import com.sun.jdi.StackFrame
+import com.intellij.util.Range
+import com.sun.jdi.*
 import org.jetbrains.annotations.TestOnly
-import org.jetbrains.kotlin.asJava.unwrapped
 import org.jetbrains.kotlin.idea.caches.resolve.resolveToCall
 import org.jetbrains.kotlin.idea.core.util.CodeInsightUtils
 import org.jetbrains.kotlin.idea.core.util.getLineNumber
 import org.jetbrains.kotlin.idea.debugger.*
+import org.jetbrains.kotlin.idea.debugger.stepping.filter.KotlinStepOverParamDefaultImplsMethodFilter
 import org.jetbrains.kotlin.idea.debugger.stepping.filter.LocationToken
 import org.jetbrains.kotlin.idea.debugger.stepping.filter.StepOverCallerInfo
 import org.jetbrains.kotlin.idea.refactoring.fqName.getKotlinFqName
 import org.jetbrains.kotlin.idea.util.application.runReadAction
+import org.jetbrains.kotlin.load.java.JvmAbi
 import org.jetbrains.kotlin.name.FqName
 import org.jetbrains.kotlin.psi.*
 import org.jetbrains.kotlin.psi.psiUtil.endOffset
@@ -152,6 +151,12 @@ fun getStepOverAction(
    val method = location.safeMethod() ?: return KotlinStepAction.JvmStepOver
    val token = LocationToken.from(stackFrame).takeIf { it.lineNumber > 0 } ?: return KotlinStepAction.JvmStepOver

+    if (token.inlineVariables.isEmpty() && method.isSyntheticMethodForDefaultParameters()) {
+        val psiLineNumber = location.lineNumber() - 1
+        val lineNumbers = Range(psiLineNumber, psiLineNumber)
+        return KotlinStepAction.StepInto(KotlinStepOverParamDefaultImplsMethodFilter.create(location, lineNumbers))
+    }
+
    val inlinedFunctionArgumentRanges = sourcePosition.collectInlineFunctionArgumentRanges()
    val positionManager = suspendContext.debugProcess.positionManager

@@ -176,6 +181,10 @@ fun getStepOverAction(
    return KotlinStepAction.KotlinStepOver(tokensToSkip, StepOverCallerInfo.from(location))
 }

+fun Method.isSyntheticMethodForDefaultParameters(): Boolean {
+    return isSynthetic && name().endsWith(JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX)
+}
+
 private fun isInlineFunctionFromLibrary(positionManager: PositionManager, location: Location, token: LocationToken): Boolean {
    if (token.inlineVariables.isEmpty()) {
        return false
@@ -26,6 +26,7 @@ import org.jetbrains.kotlin.idea.caches.resolve.resolveToDescriptorIfAny
 import org.jetbrains.kotlin.idea.codeInsight.DescriptorToSourceUtilsIde
 import org.jetbrains.kotlin.idea.debugger.stepping.smartStepInto.KotlinMethodSmartStepTarget
 import org.jetbrains.kotlin.idea.util.application.runReadAction
+import org.jetbrains.kotlin.load.java.JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX
 import org.jetbrains.kotlin.psi.KtClass
 import org.jetbrains.kotlin.psi.KtDeclaration
 import org.jetbrains.kotlin.psi.KtProperty
@@ -49,7 +50,11 @@ class KotlinOrdinaryMethodFilter(target: KotlinMethodSmartStepTarget) : NamedMet

    override fun locationMatches(process: DebugProcessImpl, location: Location): Boolean {
        val method = location.method()
-        if (targetMethodName != method.name()) return false
+        val actualMethodName = method.name()
+
+        if (!nameMatches(actualMethodName)) {
+            return false
+        }

        val positionManager = process.positionManager

@@ -60,7 +65,7 @@ class KotlinOrdinaryMethodFilter(target: KotlinMethodSmartStepTarget) : NamedMet
                it !is KtProperty || !it.isLocal
            }

-            if (declaration is KtClass && method.name() == "<init>") {
+            if (declaration is KtClass && actualMethodName == "<init>") {
                declaration.resolveToDescriptorIfAny()?.unsubstitutedPrimaryConstructor to declaration
            } else {
                declaration?.resolveToDescriptorIfAny() to declaration
@@ -94,4 +99,12 @@ class KotlinOrdinaryMethodFilter(target: KotlinMethodSmartStepTarget) : NamedMet
            psiManager.areElementsEquivalent(declaration, currentBaseDeclaration)
        }
    }
+
+    private fun nameMatches(actualName: String): Boolean {
+        return when (actualName) {
+            targetMethodName -> true
+            "$targetMethodName$DEFAULT_PARAMS_IMPL_SUFFIX" -> true
+            else -> false
+        }
+    }
 }
@@ -0,0 +1,70 @@
+/*
+ * Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors.
+ * Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
+ */
+
+package org.jetbrains.kotlin.idea.debugger.stepping.filter
+
+import com.intellij.debugger.engine.DebugProcessImpl
+import com.intellij.debugger.engine.MethodFilter
+import com.intellij.util.Range
+import com.sun.jdi.Location
+import org.jetbrains.kotlin.idea.debugger.safeMethod
+import org.jetbrains.kotlin.load.java.JvmAbi
+import org.jetbrains.org.objectweb.asm.Type
+
+class KotlinStepOverParamDefaultImplsMethodFilter(
+    private val name: String,
+    private val defaultSignature: String,
+    private val containingTypeName: String,
+    private val expressionLines: Range<Int>
+) : MethodFilter {
+    companion object {
+        fun create(location: Location, expressionLines: Range<Int>): KotlinStepOverParamDefaultImplsMethodFilter? {
+            if (location.lineNumber() < 0) {
+                return null
+            }
+
+            val method = location.safeMethod() ?: return null
+            val name = method.name()
+            assert(name.endsWith(JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX));
+            val originalName = name.dropLast(JvmAbi.DEFAULT_PARAMS_IMPL_SUFFIX.length)
+            val signature = method.signature()
+            val containingTypeName = location.declaringType().name()
+            return KotlinStepOverParamDefaultImplsMethodFilter(originalName, signature, containingTypeName, expressionLines)
+        }
+    }
+
+    override fun locationMatches(process: DebugProcessImpl?, location: Location?): Boolean {
+        val method = location?.safeMethod() ?: return true
+        val containingTypeName = location.declaringType().name()
+
+        return method.name() == name
+                && containingTypeName == this.containingTypeName
+                && signatureMatches(defaultSignature, method.signature())
+    }
+
+    private fun signatureMatches(default: String, actual: String): Boolean {
+        val defaultType = Type.getMethodType(default)
+        val actualType = Type.getMethodType(actual)
+
+        val defaultArgTypes = defaultType.argumentTypes
+        val actualArgTypes = actualType.argumentTypes
+
+        if (defaultArgTypes.size <= actualArgTypes.size) {
+            // Default method should have more parameters than the implementation
+            // because of flags
+            return false
+        }
+
+        for ((index, type) in actualArgTypes.withIndex()) {
+            if (defaultArgTypes[index] != type) {
+                return false
+            }
+        }
+
+        return true
+    }
+
+    override fun getCallingExpressionLines(): Range<Int>? = expressionLines
+}
@@ -1047,6 +1047,26 @@ public class KotlinSteppingTestGenerated extends AbstractKotlinSteppingTest {
            KotlinTestUtils.runTest(this::doCustomTest, this, testDataFilePath);
        }

+        @TestMetadata("afterDefaultParameterValues.kt")
+        public void testAfterDefaultParameterValues() throws Exception {
+            runTest("idea/jvm-debugger/jvm-debugger-test/testData/stepping/custom/afterDefaultParameterValues.kt");
+        }
+
+        @TestMetadata("afterDefaultParameterValues2.kt")
+        public void testAfterDefaultParameterValues2() throws Exception {
+            runTest("idea/jvm-debugger/jvm-debugger-test/testData/stepping/custom/afterDefaultParameterValues2.kt");
+        }
+
+        @TestMetadata("afterDefaultParameterValues2Intf.kt")
+        public void testAfterDefaultParameterValues2Intf() throws Exception {
+            runTest("idea/jvm-debugger/jvm-debugger-test/testData/stepping/custom/afterDefaultParameterValues2Intf.kt");
+        }
+
+        @TestMetadata("afterDefaultParameterValuesIntf.kt")
+        public void testAfterDefaultParameterValuesIntf() throws Exception {
+            runTest("idea/jvm-debugger/jvm-debugger-test/testData/stepping/custom/afterDefaultParameterValuesIntf.kt");
+        }
+
        public void testAllFilesPresentInCustom() throws Exception {
            KotlinTestUtils.assertAllTestsPresentByMetadataWithExcluded(this.getClass(), new File("idea/jvm-debugger/jvm-debugger-test/testData/stepping/custom"), Pattern.compile("^([^.]+)\\.kt$"), null, true);
        }
@@ -0,0 +1,21 @@
+package test
+
+fun main(args: Array<String>) {
+    //Breakpoint!
+    call()
+    foo()
+}
+
+fun call(a: Int = def()) {
+    foo()
+}
+
+fun def(): Int {
+    return libFun() // 'Step over' causes debugger skipping call()
+}
+
+fun foo() {}
+fun libFun() = 12
+
+// STEP_INTO: 1
+// STEP_OVER: 2
@@ -0,0 +1,10 @@
+LineBreakpoint created at afterDefaultParameterValues.kt:5
+Run Java
+Connected to the target VM
+afterDefaultParameterValues.kt:5
+afterDefaultParameterValues.kt:9
+afterDefaultParameterValues.kt:10
+afterDefaultParameterValues.kt:11
+Disconnected from the target VM
+
+Process finished with exit code 0
@@ -0,0 +1,21 @@
+package test
+
+fun main(args: Array<String>) {
+    //Breakpoint!
+    call()
+    foo()
+}
+
+fun call(a: Int = def()) {
+    foo()
+}
+
+fun def(): Int {
+    return libFun() // 'Step over' causes debugger skipping call()
+}
+
+fun foo() {}
+fun libFun() = 12
+
+// STEP_INTO: 2
+// STEP_OVER: 5
@@ -0,0 +1,14 @@
+LineBreakpoint created at afterDefaultParameterValues2.kt:5
+Run Java
+Connected to the target VM
+afterDefaultParameterValues2.kt:5
+afterDefaultParameterValues2.kt:9
+afterDefaultParameterValues2.kt:14
+afterDefaultParameterValues2.kt:9
+afterDefaultParameterValues2.kt:10
+afterDefaultParameterValues2.kt:11
+afterDefaultParameterValues2.kt:6
+afterDefaultParameterValues2.kt:7
+Disconnected from the target VM
+
+Process finished with exit code 0
@@ -0,0 +1,26 @@
+package test
+
+fun main() {
+    val foo = FooImpl()
+    //Breakpoint!
+    foo.call()
+    foo.foo()
+}
+
+interface Foo {
+    fun call(a: Int = def()) {
+        foo()
+    }
+
+    fun def(): Int {
+        return libFun() // 'Step over' causes debugger skipping call()
+    }
+
+    fun foo() {}
+    fun libFun() = 12
+}
+
+class FooImpl : Foo
+
+// STEP_INTO: 2
+// STEP_OVER: 7
@@ -0,0 +1,16 @@
+LineBreakpoint created at afterDefaultParameterValues2Intf.kt:6
+Run Java
+Connected to the target VM
+afterDefaultParameterValues2Intf.kt:6
+afterDefaultParameterValues2Intf.kt:11
+afterDefaultParameterValues2Intf.kt:16
+afterDefaultParameterValues2Intf.kt:23
+afterDefaultParameterValues2Intf.kt:11
+afterDefaultParameterValues2Intf.kt:12
+afterDefaultParameterValues2Intf.kt:13
+afterDefaultParameterValues2Intf.kt:23
+afterDefaultParameterValues2Intf.kt:11
+afterDefaultParameterValues2Intf.kt:7
+Disconnected from the target VM
+
+Process finished with exit code 0
@@ -0,0 +1,26 @@
+package test
+
+fun main() {
+    val foo = FooImpl()
+    //Breakpoint!
+    foo.call()
+    foo.foo()
+}
+
+interface Foo {
+    fun call(a: Int = def()) {
+        foo()
+    }
+
+    fun def(): Int {
+        return libFun() // 'Step over' causes debugger skipping call()
+    }
+
+    fun foo() {}
+    fun libFun() = 12
+}
+
+class FooImpl : Foo
+
+// STEP_INTO: 1
+// STEP_OVER: 2
@@ -0,0 +1,10 @@
+LineBreakpoint created at afterDefaultParameterValuesIntf.kt:6
+Run Java
+Connected to the target VM
+afterDefaultParameterValuesIntf.kt:6
+afterDefaultParameterValuesIntf.kt:11
+afterDefaultParameterValuesIntf.kt:12
+afterDefaultParameterValuesIntf.kt:13
+Disconnected from the target VM
+
+Process finished with exit code 0
@@ -7,11 +7,11 @@ Connected to the target VM
 smartStepIntoFunWithDefaultArgs.kt:17
 smartStepIntoFunWithDefaultArgs.kt:4
 smartStepIntoFunWithDefaultArgs.kt:22
-smartStepIntoFunWithDefaultArgs.kt:4
+smartStepIntoFunWithDefaultArgs.kt:3
 smartStepIntoFunWithDefaultArgs.kt:27
 smartStepIntoFunWithDefaultArgs.kt:8
 smartStepIntoFunWithDefaultArgs.kt:32
-smartStepIntoFunWithDefaultArgs.kt:8
+smartStepIntoFunWithDefaultArgs.kt:7
 Disconnected from the target VM

 Process finished with exit code 0
@@ -16,7 +16,7 @@ smartStepIntoInterfaceImpl.kt:91
 smartStepIntoInterfaceImpl.kt:96
 smartStepIntoInterfaceImpl.kt:9
 smartStepIntoInterfaceImpl.kt:101
-smartStepIntoInterfaceImpl.kt:14
+smartStepIntoInterfaceImpl.kt:13
 smartStepIntoInterfaceImpl.kt:106
 smartStepIntoInterfaceImpl.kt:14
 smartStepIntoInterfaceImpl.kt:111
@@ -39,4 +39,4 @@ suspend fun run() {
    }
 }

-// STEP_OVER: 4
+// STEP_OVER: 3
@@ -3,7 +3,6 @@ Run Java
 Connected to the target VM
 soSuspendableCallInEndOfFun.kt:29
 soSuspendableCallInEndOfFun.kt:17
-soSuspendableCallInEndOfFun.kt:16
 soSuspendableCallInEndOfFun.kt:18
 soSuspendableCallInEndOfFun.kt:37
 Disconnected from the target VM