Debugger: Fix breakpoint applicability (KT-10984)

Ensure that breakpoints of each type can be placed only on lines where it makes sense to place a breakpoint.

Here is a quick summary of the rules:
1. Method breakpoints are available for functions, property accessors, constructors;
2. Line breakpoints are available on any line with an expression, excluding some cases like 'const' property initializers or annotations;
3. Line breakpoints should be available on a '}' in functions and lambdas;
4. Line breakpoints are not suggested for one-liners;
5. Lambda breakpoints should be shown for single-line lambdas.

idea/jvm-debugger/jvm-debugger-core/src/org/jetbrains/kotlin/idea/debugger/breakpoints/KotlinFieldBreakpointType.kt

@@ -41,6 +41,7 @@ import org.jetbrains.kotlin.idea.debugger.breakpoints.dialog.AddFieldBreakpointD
 import org.jetbrains.kotlin.idea.util.application.runWriteAction
 import org.jetbrains.kotlin.psi.KtDeclarationContainer
 import org.jetbrains.kotlin.psi.KtFile
+import org.jetbrains.kotlin.psi.KtParameter
 import org.jetbrains.kotlin.psi.KtProperty
 import javax.swing.JComponent
 
@@ -52,7 +53,13 @@ class KotlinFieldBreakpointType : JavaBreakpointType<KotlinPropertyBreakpointPro
     }
 
     override fun canPutAt(file: VirtualFile, line: Int, project: Project): Boolean {
-        return canPutAt(file, line, project, this::class.java)
+        return isBreakpointApplicable(file, line, project) { element ->
+            when (element) {
+                is KtProperty -> ApplicabilityResult.definitely(!element.isLocal)
+                is KtParameter -> ApplicabilityResult.definitely(element.hasValOrVar())
+                else -> ApplicabilityResult.UNKNOWN
+            }
+        }
     }
 
     override fun getPriority() = 120

idea/jvm-debugger/jvm-debugger-core/src/org/jetbrains/kotlin/idea/debugger/breakpoints/KotlinLineBreakpointType.java

@@ -29,6 +29,7 @@ import com.intellij.openapi.util.Comparing;
 import com.intellij.openapi.util.TextRange;
 import com.intellij.openapi.vfs.VirtualFile;
 import com.intellij.psi.PsiElement;
+import com.intellij.psi.impl.source.tree.LeafPsiElement;
 import com.intellij.psi.PsiFile;
 import com.intellij.psi.PsiManager;
 import com.intellij.psi.util.PsiTreeUtil;
@@ -40,16 +41,19 @@ import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.jetbrains.java.debugger.breakpoints.properties.JavaBreakpointProperties;
 import org.jetbrains.java.debugger.breakpoints.properties.JavaLineBreakpointProperties;
+import org.jetbrains.kotlin.idea.KotlinBundle;
 import org.jetbrains.kotlin.idea.debugger.KotlinPositionManager;
-import org.jetbrains.kotlin.psi.KtClassInitializer;
+import org.jetbrains.kotlin.lexer.KtTokens;
 import org.jetbrains.kotlin.psi.KtElement;
-import org.jetbrains.kotlin.psi.KtFunction;
+import org.jetbrains.kotlin.psi.*;
 
 import java.util.List;
 
+import static org.jetbrains.kotlin.idea.debugger.breakpoints.BreakpointTypeUtilsKt.isBreakpointApplicable;
+
 public class KotlinLineBreakpointType extends JavaLineBreakpointType {
     public KotlinLineBreakpointType() {
-        super("kotlin-line", "Kotlin Line Breakpoints");
+        super("kotlin-line", KotlinBundle.message("debugger.line.breakpoints.tab.title"));
     }
 
     @NotNull
@@ -126,13 +130,50 @@ public class KotlinLineBreakpointType extends JavaLineBreakpointType {
 
     @Override
     public boolean canPutAt(@NotNull VirtualFile file, int line, @NotNull Project project) {
-        return BreakpointTypeUtilsKt.canPutAt(file, line, project, getClass());
+        return isBreakpointApplicable(file, line, project, element -> {
+            if (element instanceof KtDestructuringDeclaration) {
+                return ApplicabilityResult.MAYBE_YES;
+            }
+
+            if (isClosingBraceInMethod(element)) {
+                return ApplicabilityResult.MAYBE_YES;
+            }
+
+            if (element instanceof KtElement) {
+                LineBreakpointExpressionVisitor visitor = LineBreakpointExpressionVisitor.of(file, line);
+                if (visitor != null) {
+                    ApplicabilityResult result = ((KtElement) element).accept(visitor, null);
+                    if (result == null) {
+                        return ApplicabilityResult.UNKNOWN;
+                    }
+                    return result;
+                }
+            }
+
+            return ApplicabilityResult.UNKNOWN;
+        });
+    }
+
+    private static boolean isClosingBraceInMethod(PsiElement element) {
+        if (element instanceof LeafPsiElement && element.getNode().getElementType() == KtTokens.RBRACE) {
+            PsiElement blockExpression = element.getParent();
+            if (blockExpression instanceof KtFunctionLiteral) {
+                return true;
+            }
+            if (blockExpression instanceof KtBlockExpression) {
+                PsiElement owner = blockExpression.getParent();
+                if (owner instanceof KtFunction || owner instanceof KtClassInitializer) {
+                    return true;
+                }
+            }
+        }
+        return false;
     }
 
     @NotNull
     @Override
     public List<JavaBreakpointVariant> computeVariants(@NotNull Project project, @NotNull XSourcePosition position) {
-        return BreakpointTypeUtilsKt.computeVariants(project, position, this);
+        return BreakpointTypeUtilsKt.computeLineBreakpointVariants(project, position, this);
     }
 
     @Nullable
@@ -195,4 +236,32 @@ public class KotlinLineBreakpointType extends JavaLineBreakpointType {
 
         return super.getSourcePosition(breakpoint);
     }
+
+    public class LineKotlinBreakpointVariant extends LineJavaBreakpointVariant {
+        public LineKotlinBreakpointVariant(@NotNull XSourcePosition position, @Nullable PsiElement element, Integer lambdaOrdinal) {
+            super(position, element, lambdaOrdinal);
+        }
+
+        @NotNull
+        @Override
+        public String getText() {
+            return "Line Breakpoint";
+        }
+    }
+
+    public class KotlinBreakpointVariant extends JavaBreakpointVariant {
+        private final int lambdaCount;
+
+        public KotlinBreakpointVariant(@NotNull XSourcePosition position, int lambdaCount) {
+            super(position);
+            this.lambdaCount = lambdaCount;
+        }
+
+        @NotNull
+        @Override
+        public String getText() {
+            String lambdas = lambdaCount > 1 ? "Lambdas" : "Lambda";
+            return "Line and " + lambdas + " Breakpoints";
+        }
+    }
 }

idea/jvm-debugger/jvm-debugger-core/src/org/jetbrains/kotlin/idea/debugger/breakpoints/LineBreakpointExpressionVisitor.kt

@@ -0,0 +1,217 @@
+/*
+ * Copyright 2010-2019 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.breakpoints
+
+import com.intellij.openapi.editor.Document
+import com.intellij.openapi.fileEditor.FileDocumentManager
+import com.intellij.openapi.vfs.VirtualFile
+import com.intellij.psi.PsiElement
+import org.jetbrains.kotlin.lexer.KtTokens
+import org.jetbrains.kotlin.psi.*
+import org.jetbrains.kotlin.psi.psiUtil.endOffset
+import org.jetbrains.kotlin.psi.psiUtil.startOffset
+
+class LineBreakpointExpressionVisitor private constructor(
+    private val document: Document,
+    private val mainLine: Int
+) : KtVisitor<ApplicabilityResult, Unit?>() {
+    companion object {
+        @JvmStatic
+        fun of(file: VirtualFile, line: Int): LineBreakpointExpressionVisitor? {
+            val document = FileDocumentManager.getInstance().getDocument(file) ?: return null
+            return LineBreakpointExpressionVisitor(document, line)
+        }
+    }
+
+    override fun visitKtElement(element: KtElement, data: Unit?): ApplicabilityResult {
+        return visitChildren(element.children.asList(), data)
+    }
+
+    private fun visitChildren(children: List<PsiElement>, data: Unit?): ApplicabilityResult {
+        if (children.isEmpty() || !children.first().isSameLine()) {
+            return ApplicabilityResult.UNKNOWN
+        }
+
+        var isApplicable = false
+
+        for (child in children) {
+            val ktElement = child as? KtElement ?: continue
+            val result = ktElement.accept(this, data)
+            if (result.shouldStop) {
+                return result
+            }
+
+            isApplicable = isApplicable or result.isApplicable
+        }
+
+        return ApplicabilityResult.maybe(isApplicable)
+    }
+
+    override fun visitExpression(expression: KtExpression, data: Unit?): ApplicabilityResult {
+        if (expression.isSameLine()) {
+            val superResult = super.visitExpression(expression, data)
+            if (superResult.shouldStop) {
+                return superResult
+            }
+
+            return ApplicabilityResult.MAYBE_YES
+        }
+
+        return ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitClass(klass: KtClass, data: Unit?): ApplicabilityResult {
+        visitChildren(klass.primaryConstructorParameters, data).handle()?.let { return it }
+        return klass.body?.accept(this, data) ?: ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitObjectDeclaration(declaration: KtObjectDeclaration, data: Unit?): ApplicabilityResult {
+        return declaration.body?.accept(this, data) ?: ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitSecondaryConstructor(constructor: KtSecondaryConstructor, data: Unit?): ApplicabilityResult {
+        return constructor.bodyExpression?.accept(this, data)
+            ?.acceptIfMultiLineParent(constructor)
+            ?: ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitParameter(parameter: KtParameter, data: Unit?): ApplicabilityResult {
+        val defaultValue = parameter.defaultValue
+        if (defaultValue.isSameLine()) {
+            return ApplicabilityResult.DEFINITELY_YES
+        }
+
+        return ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitCatchSection(catchClause: KtCatchClause, data: Unit?): ApplicabilityResult {
+        return ApplicabilityResult.MAYBE_YES
+    }
+
+    override fun visitAnnotationEntry(annotationEntry: KtAnnotationEntry, data: Unit?): ApplicabilityResult {
+        return ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitProperty(property: KtProperty, data: Unit?): ApplicabilityResult {
+        if (property.hasModifier(KtTokens.CONST_KEYWORD)) {
+            return ApplicabilityResult.UNKNOWN
+        }
+
+        return super.visitProperty(property, data)
+    }
+
+    override fun visitNamedFunction(function: KtNamedFunction, data: Unit?): ApplicabilityResult {
+        if (function.isSameLine() && KtPsiUtil.isLocal(function)) {
+            return ApplicabilityResult.MAYBE_YES
+        }
+
+        visitChildren(function.valueParameters, data).handle()?.let { return it }
+
+        return function.bodyExpression?.accept(this, data)
+            ?.acceptIfMultiLineParent(function)
+            ?: ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitPropertyAccessor(accessor: KtPropertyAccessor, data: Unit?): ApplicabilityResult {
+        return accessor.bodyExpression?.accept(this, data)
+            ?.acceptIfMultiLineParent(accessor)
+            ?: ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitPropertyDelegate(delegate: KtPropertyDelegate, data: Unit?): ApplicabilityResult {
+        return ApplicabilityResult.maybe(delegate.isSameLine())
+    }
+
+    override fun visitLambdaExpression(expression: KtLambdaExpression, data: Unit?): ApplicabilityResult {
+        if (expression.isSameLine()) {
+            return ApplicabilityResult.DEFINITELY_YES
+        }
+
+        return ApplicabilityResult.UNKNOWN
+    }
+
+    override fun visitWhenEntry(jetWhenEntry: KtWhenEntry, data: Unit?): ApplicabilityResult {
+        jetWhenEntry.expression?.accept(this, data)?.handle()?.let { return it }
+        return ApplicabilityResult.maybe(jetWhenEntry.conditions.isNotEmpty() || jetWhenEntry.isElse)
+    }
+
+    override fun visitDestructuringDeclaration(multiDeclaration: KtDestructuringDeclaration, data: Unit?): ApplicabilityResult {
+        return ApplicabilityResult.maybe(multiDeclaration.isSameLine())
+    }
+
+    override fun visitBlockExpression(expression: KtBlockExpression, data: Unit?): ApplicabilityResult {
+        return visitChildren(expression.statements, data)
+    }
+
+    override fun visitParenthesizedExpression(expression: KtParenthesizedExpression, data: Unit?): ApplicabilityResult {
+        val parenthesized = expression.expression ?: return ApplicabilityResult.UNKNOWN
+        val lines = parenthesized.getLines()
+
+        if (lines.start > mainLine) {
+            return ApplicabilityResult.UNKNOWN
+        }
+
+        if (lines.isMultiLine) {
+            return parenthesized.accept(this, data)
+        }
+
+        return ApplicabilityResult.UNKNOWN
+    }
+
+    private fun PsiElement?.isSameLine(): Boolean {
+        val lines = getLines()
+        return lines.start == mainLine
+    }
+
+    private fun PsiElement?.getLines(): Lines {
+        if (this == null) {
+            return Lines.EMPTY
+        }
+
+        val startOffset = this.startOffset
+        val endOffset = this.endOffset
+
+        if (startOffset < 0 || endOffset < 0) {
+            return Lines.EMPTY
+        }
+
+        val startLine = document.getLineNumber(startOffset)
+        val endLine = document.getLineNumber(endOffset)
+        return Lines(startLine, endLine)
+    }
+
+    private fun ApplicabilityResult.acceptIfMultiLineParent(parent: KtExpression): ApplicabilityResult? {
+        if (this.shouldStop) {
+            return this
+        }
+
+        if (parent.getLines().isSingleLine) {
+            return null
+        }
+
+        return this
+    }
+}
+
+private fun ApplicabilityResult.handle(): ApplicabilityResult? {
+    if (this.shouldStop || this.isApplicable) {
+        return this
+    }
+
+    return null
+}
+
+private class Lines(val start: Int, val end: Int) {
+    companion object {
+        val EMPTY = Lines(-1, -1)
+    }
+
+    val isSingleLine: Boolean
+        get() = start == end && start >= 0
+
+    val isMultiLine: Boolean
+        get() = start != end && start >= 0 && end >= 0
+}

idea/jvm-debugger/jvm-debugger-core/src/org/jetbrains/kotlin/idea/debugger/breakpoints/breakpointTypeUtils.kt

@@ -39,51 +39,6 @@ import org.jetbrains.kotlin.psi.psiUtil.*
 import org.jetbrains.kotlin.utils.addToStdlib.firstIsInstance
 import java.util.*
 
-fun canPutAt(file: VirtualFile, line: Int, project: Project, breakpointTypeClass: Class<*>): Boolean {
-    val psiFile = PsiManager.getInstance(project).findFile(file)
-
-    if (psiFile == null || psiFile.virtualFile?.fileType != KotlinFileType.INSTANCE) {
-        return false
-    }
-
-    val document = FileDocumentManager.getInstance().getDocument(file) ?: return false
-
-    var result: Class<*>? = null
-    XDebuggerUtil.getInstance().iterateLine(project, document, line, fun (el: PsiElement): Boolean {
-        // avoid comments
-        if (el is PsiWhiteSpace || PsiTreeUtil.getParentOfType(el, PsiComment::class.java, false) != null) {
-            return true
-        }
-
-        var element = el
-        var parent = element.parent
-        while (parent != null) {
-            val offset = parent.textOffset
-            if (offset >= 0 && document.getLineNumber(offset) != line) break
-
-            element = parent
-            parent = element.parent
-        }
-
-        if (element is KtProperty || element is KtParameter) {
-            result = if ((element is KtParameter && element.hasValOrVar()) || (element is KtProperty && !element.isLocal)) {
-                KotlinFieldBreakpointType::class.java
-            }
-            else {
-                KotlinLineBreakpointType::class.java
-            }
-            return false
-        }
-        else {
-            result = KotlinLineBreakpointType::class.java
-        }
-
-        return true
-    })
-
-    return result == breakpointTypeClass
-}
-
 class ApplicabilityResult(val isApplicable: Boolean, val shouldStop: Boolean) {
     companion object {
         @JvmStatic
@@ -111,32 +66,34 @@ fun isBreakpointApplicable(file: VirtualFile, line: Int, project: Project, check
     }
 
     val document = FileDocumentManager.getInstance().getDocument(file) ?: return false
-    var isApplicable = false
 
-    val checked = HashSet<PsiElement>()
+    return runReadAction {
+        var isApplicable = false
+        val checked = HashSet<PsiElement>()
 
-    XDebuggerUtil.getInstance().iterateLine(project, document, line, fun(element: PsiElement): Boolean {
-        if (element is PsiWhiteSpace || element.getParentOfType<PsiComment>(false) != null) {
-            return true
-        }
+        XDebuggerUtil.getInstance().iterateLine(project, document, line, fun(element: PsiElement): Boolean {
+            if (element is PsiWhiteSpace || element.getParentOfType<PsiComment>(false) != null) {
+                return true
+            }
 
-        val parent = getTopmostParentOnLineOrSelf(element, document, line)
-        if (!checked.add(parent)) {
-            return true
-        }
+            val parent = getTopmostParentOnLineOrSelf(element, document, line)
+            if (!checked.add(parent)) {
+                return true
+            }
 
-        val result = checker(parent)
+            val result = checker(parent)
 
-        if (result.shouldStop && !result.isApplicable) {
-            isApplicable = false
-            return false
-        }
+            if (result.shouldStop && !result.isApplicable) {
+                isApplicable = false
+                return false
+            }
 
-        isApplicable = isApplicable or result.isApplicable
-        return !result.shouldStop
-    })
+            isApplicable = isApplicable or result.isApplicable
+            return !result.shouldStop
+        })
 
-    return isApplicable
+        return@runReadAction isApplicable
+    }
 }
 
 private fun getTopmostParentOnLineOrSelf(element: PsiElement, document: Document, line: Int): PsiElement {
@@ -153,7 +110,7 @@ private fun getTopmostParentOnLineOrSelf(element: PsiElement, document: Document
     return current
 }
 
-fun computeVariants(
+fun computeLineBreakpointVariants(
     project: Project,
     position: XSourcePosition,
     kotlinBreakpointType: KotlinLineBreakpointType
@@ -167,15 +124,19 @@ fun computeVariants(
     val result = LinkedList<JavaLineBreakpointType.JavaBreakpointVariant>()
 
     val elementAt = pos.elementAt.parentsWithSelf.firstIsInstance<KtElement>()
-    val mainMethod = KotlinLineBreakpointType.getContainingMethod(elementAt)
+    val mainMethod = PsiTreeUtil.getParentOfType(elementAt, KtFunction::class.java, false)
+
+    var mainMethodAdded = false
+
     if (mainMethod != null) {
-        result.add(
-            kotlinBreakpointType.LineJavaBreakpointVariant(
-                position,
-                CodeInsightUtils.getTopmostElementAtOffset(elementAt, pos.offset),
-                -1
-            )
-        )
+        val bodyExpression = mainMethod.bodyExpression
+        val isLambdaResult = bodyExpression is KtLambdaExpression && bodyExpression.functionLiteral in lambdas
+
+        if (!isLambdaResult) {
+            val variantElement = CodeInsightUtils.getTopmostElementAtOffset(elementAt, pos.offset)
+            result.add(kotlinBreakpointType.LineKotlinBreakpointVariant(position, variantElement, -1))
+            mainMethodAdded = true
+        }
     }
 
     lambdas.forEachIndexed { ordinal, lambda ->
@@ -186,7 +147,9 @@ fun computeVariants(
         }
     }
 
-    result.add(kotlinBreakpointType.JavaBreakpointVariant(position))
+    if (mainMethodAdded && result.size > 1) {
+        result.add(kotlinBreakpointType.KotlinBreakpointVariant(position, lambdas.size))
+    }
 
     return result
 }