KT-5488 Invalid ambiguity between plus and plusAssign

#KT-5488  Fixed

compiler/frontend/src/org/jetbrains/kotlin/types/expressions/BasicExpressionTypingVisitor.java

@@ -837,33 +837,43 @@ public class BasicExpressionTypingVisitor extends ExpressionTypingVisitor {
         return !context.dataFlowInfo.getNullability(dataFlowValue).canBeNull();
     }
 
-    public static void checkLValue(@NotNull BindingTrace trace, @NotNull JetExpression expression) {
+    /**
-        checkLValue(trace, expression, false);
+     * @return {@code true} iff expression can be assigned to
+     */
+    public static boolean checkLValue(@NotNull BindingTrace trace, @NotNull JetExpression expression) {
+        return checkLValue(trace, expression, false);
     }
 
-    private static void checkLValue(@NotNull BindingTrace trace, @NotNull JetExpression expressionWithParenthesis, boolean canBeThis) {
+    private static boolean checkLValue(@NotNull BindingTrace trace, @NotNull JetExpression expressionWithParenthesis, boolean canBeThis) {
         JetExpression expression = JetPsiUtil.deparenthesize(expressionWithParenthesis);
         if (expression instanceof JetArrayAccessExpression) {
             JetExpression arrayExpression = ((JetArrayAccessExpression) expression).getArrayExpression();
-            if (arrayExpression != null) {
+            if (arrayExpression == null) return false;
-                checkLValue(trace, arrayExpression, true);
+
-            }
+            return checkLValue(trace, arrayExpression, true);
-            return;
         }
-        if (canBeThis && expression instanceof JetThisExpression) return;
+        if (canBeThis && expression instanceof JetThisExpression) return true;
         VariableDescriptor variable = BindingContextUtils.extractVariableDescriptorIfAny(trace.getBindingContext(), expression, true);
 
+        boolean result = true;
         JetExpression reportOn = expression != null ? expression : expressionWithParenthesis;
         if (variable instanceof PropertyDescriptor) {
             PropertyDescriptor propertyDescriptor = (PropertyDescriptor) variable;
             if (propertyDescriptor.isSetterProjectedOut()) {
                 trace.report(SETTER_PROJECTED_OUT.on(reportOn, propertyDescriptor));
+                result = false;
             }
         }
 
         if (variable == null) {
             trace.report(VARIABLE_EXPECTED.on(reportOn));
+            result = false;
         }
+        else if (!variable.isVar()) {
+            result = false;
+        }
+
+        return result;
     }
 
     @Override

compiler/frontend/src/org/jetbrains/kotlin/types/expressions/ExpressionTypingVisitorForStatements.java

@@ -36,6 +36,7 @@ import org.jetbrains.kotlin.resolve.TemporaryBindingTrace;
 import org.jetbrains.kotlin.resolve.calls.context.TemporaryTraceAndCache;
 import org.jetbrains.kotlin.resolve.calls.model.ResolvedCall;
 import org.jetbrains.kotlin.resolve.calls.results.OverloadResolutionResults;
+import org.jetbrains.kotlin.resolve.calls.results.OverloadResolutionResultsImpl;
 import org.jetbrains.kotlin.resolve.calls.results.OverloadResolutionResultsUtil;
 import org.jetbrains.kotlin.resolve.calls.smartcasts.DataFlowInfo;
 import org.jetbrains.kotlin.resolve.calls.smartcasts.DataFlowValue;
@@ -276,15 +277,24 @@ public class ExpressionTypingVisitorForStatements extends ExpressionTypingVisito
         JetType assignmentOperationType = OverloadResolutionResultsUtil.getResultingType(assignmentOperationDescriptors,
                                                                                          context.contextDependency);
 
-        // Check for '+'
+        OverloadResolutionResults<FunctionDescriptor> binaryOperationDescriptors;
-        Name counterpartName = OperatorConventions.BINARY_OPERATION_NAMES.get(OperatorConventions.ASSIGNMENT_OPERATION_COUNTERPARTS.get(operationType));
+        JetType binaryOperationType;
         TemporaryTraceAndCache temporaryForBinaryOperation = TemporaryTraceAndCache.create(
                 context, "trace to check binary operation like '+' for", expression);
-        OverloadResolutionResults<FunctionDescriptor> binaryOperationDescriptors = components.callResolver.resolveBinaryCall(
+        boolean lhsAssignable = BasicExpressionTypingVisitor.checkLValue(TemporaryBindingTrace.create(context.trace, "Trace for checking assignability"), left);
-                context.replaceTraceAndCache(temporaryForBinaryOperation).replaceScope(scope),
+        if (assignmentOperationType == null || lhsAssignable) {
-                receiver, expression, counterpartName
+            // Check for '+'
-        );
+            Name counterpartName = OperatorConventions.BINARY_OPERATION_NAMES.get(OperatorConventions.ASSIGNMENT_OPERATION_COUNTERPARTS.get(operationType));
-        JetType binaryOperationType = OverloadResolutionResultsUtil.getResultingType(binaryOperationDescriptors, context.contextDependency);
+            binaryOperationDescriptors = components.callResolver.resolveBinaryCall(
+                    context.replaceTraceAndCache(temporaryForBinaryOperation).replaceScope(scope),
+                    receiver, expression, counterpartName
+            );
+            binaryOperationType = OverloadResolutionResultsUtil.getResultingType(binaryOperationDescriptors, context.contextDependency);
+        }
+        else {
+            binaryOperationDescriptors = OverloadResolutionResultsImpl.nameNotFound();
+            binaryOperationType = null;
+        }
 
         JetType type = assignmentOperationType != null ? assignmentOperationType : binaryOperationType;
         if (assignmentOperationDescriptors.isSuccess() && binaryOperationDescriptors.isSuccess()) {

compiler/testData/diagnostics/tests/operatorsOverloading/AssignOperatorAmbiguity.kt

@@ -8,7 +8,7 @@ class MyInt(val i: Int) {
 fun Any.plusAssign(<!UNUSED_PARAMETER!>a<!>: Any) {}
 
 fun test(m: MyInt) {
-    m <!ASSIGN_OPERATOR_AMBIGUITY!>+=<!> m
+    m += m
 
     var i = 1
     i <!ASSIGN_OPERATOR_AMBIGUITY!>+=<!> 34

compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnArray.kt

@@ -0,0 +1,20 @@
+// !DIAGNOSTICS: -UNUSED_PARAMETER
+
+class C {
+    fun get(i: Int): C = this
+}
+
+fun C.plus(a: Any): C = this
+fun C.plusAssign(a: Any) {}
+
+class C1 {
+    fun get(i: Int): C = C()
+    fun set(i: Int, v: C) {}
+}
+
+fun test() {
+    val c = C()
+    c[0] += ""
+    var c1 = C1()
+    c1[0] <!ASSIGN_OPERATOR_AMBIGUITY!>+=<!> ""
+}

compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnArray.txt

@@ -0,0 +1,22 @@
+package
+
+internal fun test(): kotlin.Unit
+internal fun C.plus(/*0*/ a: kotlin.Any): C
+internal fun C.plusAssign(/*0*/ a: kotlin.Any): kotlin.Unit
+
+internal final class C {
+    public constructor C()
+    public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean
+    internal final fun get(/*0*/ i: kotlin.Int): C
+    public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int
+    public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String
+}
+
+internal final class C1 {
+    public constructor C1()
+    public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean
+    internal final fun get(/*0*/ i: kotlin.Int): C
+    public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int
+    internal final fun set(/*0*/ i: kotlin.Int, /*1*/ v: C): kotlin.Unit
+    public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String
+}

compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnLocal.kt

@@ -0,0 +1,13 @@
+// !DIAGNOSTICS: -UNUSED_PARAMETER
+
+class C
+
+fun C.plus(a: Any): C = this
+fun C.plusAssign(a: Any) {}
+
+fun test() {
+    val c = C()
+    c += ""
+    var c1 = C()
+    c1 <!ASSIGN_OPERATOR_AMBIGUITY!>+=<!> ""
+}

compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnLocal.txt

@@ -0,0 +1,12 @@
+package
+
+internal fun test(): kotlin.Unit
+internal fun C.plus(/*0*/ a: kotlin.Any): C
+internal fun C.plusAssign(/*0*/ a: kotlin.Any): kotlin.Unit
+
+internal final class C {
+    public constructor C()
+    public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean
+    public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int
+    public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String
+}

compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnProperty.kt

@@ -0,0 +1,19 @@
+// !DIAGNOSTICS: -UNUSED_PARAMETER
+
+class C {
+    val c: C = C()
+}
+
+fun C.plus(a: Any): C = this
+fun C.plusAssign(a: Any) {}
+
+class C1 {
+    var c: C = C()
+}
+
+fun test() {
+    val c = C()
+    c.c += ""
+    var c1 = C1()
+    c1.c <!ASSIGN_OPERATOR_AMBIGUITY!>+=<!> ""
+}

compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnProperty.txt

@@ -0,0 +1,21 @@
+package
+
+internal fun test(): kotlin.Unit
+internal fun C.plus(/*0*/ a: kotlin.Any): C
+internal fun C.plusAssign(/*0*/ a: kotlin.Any): kotlin.Unit
+
+internal final class C {
+    public constructor C()
+    internal final val c: C
+    public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean
+    public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int
+    public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String
+}
+
+internal final class C1 {
+    public constructor C1()
+    internal final var c: C
+    public open override /*1*/ /*fake_override*/ fun equals(/*0*/ other: kotlin.Any?): kotlin.Boolean
+    public open override /*1*/ /*fake_override*/ fun hashCode(): kotlin.Int
+    public open override /*1*/ /*fake_override*/ fun toString(): kotlin.String
+}

compiler/tests/org/jetbrains/kotlin/checkers/JetDiagnosticsTestGenerated.java

@@ -8110,6 +8110,24 @@ public class JetDiagnosticsTestGenerated extends AbstractJetDiagnosticsTest {
                 String fileName = JetTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/operatorsOverloading/kt3450.kt");
                 doTest(fileName);
             }
+
+            @TestMetadata("plusAssignOnArray.kt")
+            public void testPlusAssignOnArray() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnArray.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("plusAssignOnLocal.kt")
+            public void testPlusAssignOnLocal() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnLocal.kt");
+                doTest(fileName);
+            }
+
+            @TestMetadata("plusAssignOnProperty.kt")
+            public void testPlusAssignOnProperty() throws Exception {
+                String fileName = JetTestUtils.navigationMetadata("compiler/testData/diagnostics/tests/operatorsOverloading/plusAssignOnProperty.kt");
+                doTest(fileName);
+            }
         }
 
         @TestMetadata("compiler/testData/diagnostics/tests/overload")

spec-docs/operator-conventions.md

@@ -135,9 +135,16 @@ Parentheses are translated to calls to invoke with appropriate number of argumen
 For the assignment operations, e.g. `a += b`, the compiler performs the following steps:
 
 * If the function from the right column is available
-  * If the corresponding binary function (i.e. `plus()` for `plusAssign()`) is available too, report error (ambiguity).
+  * If the left-hand side can be assigned to and the corresponding binary function (i.e. `plus()` for `plusAssign()`) is available, report error (ambiguity).
   * Make sure its return type is `Unit`, and report an error otherwise.
   * Generate code for `a.plusAssign(b)`
 * Otherwise, try to generate code for `a = a + b` (this includes a type check: the type of `a + b` must be a subtype of `a`).
 
-*Note*: assignments are *NOT* expressions in Kotlin.
+*Note*: assignments are *NOT* expressions in Kotlin.
+
+**Discussion of the ambiguity rule**:
+We raise an error when both `plus()` and `plusAssign()` are available only if the lhs is assignable. Otherwise, the availability of `plus()`
+is irrelevant, because we know that `a = a + b` can not compile. An important concern here is what happens when the lhs *becomes assignable*
+after the fact (e.g. the user changes *val* to *var* or provides a `set()` function for indexing convention): in this case, the previously
+correct call site may become incorrect, but not the other way around, which is safe, because former calls to `plusAssign()` can not be silently
+turned into calls to `plus()`.