Cleanup to make recent change a bit more clear and efficient

scripts/diagnostics.txt

@@ -697,6 +697,7 @@ error InterfacePortInvalidExpression "invalid expression for interface port conn
 error AssignmentPatternLValueDynamic "lvalue assignment patterns cannot be assigned from dynamic arrays"
 error SelectAfterRangeSelect "cannot chain select expressions after a range select"
 error ClassPrivateMembersBitstream "cannot use class type {} as a bit-stream because it has local or protected members that are not accessible in this context"
+error CannotCompareTwoInstances "cannot compare two instances to each other; at least one side must be a virtual interface"
 warning ignored-slice IgnoredSlice "slice size ignored for left-to-right streaming operator"
 warning unsized-concat UnsizedInConcat "unsized integer in concat; using a width of {}"
 warning width-expand WidthExpand "implicit conversion expands from {} to {} bits"

source/ast/Expression.cpp

@@ -1229,7 +1229,7 @@ Expression* Expression::tryBindInterfaceRef(const ASTContext& context,
     if (!result.found) {
         // We didn't find the name as-is. This might be a case where the user has
         // provided an explicit modport name on top of an array of interfaces,
-        // which we should support by looking up the name again withotu the trailing
+        // which we should support by looking up the name again without the trailing
         // name component and taking the result if it's an iface array.
         if (expr->kind == SyntaxKind::ScopedName) {
             auto& scoped = expr->as<ScopedNameSyntax>();

source/ast/expressions/OperatorExpressions.cpp

@@ -592,38 +592,45 @@ Expression& BinaryExpression::fromSyntax(Compilation& compilation,
         flags = ASTFlags::AllowUnboundedLiteral;
     }
 
+    Expression *lhs, *rhs;
+    auto& syntaxLeft = *syntax.left;
+    auto& syntaxRight = *syntax.right;
+
     auto op = getBinaryOperator(syntax.kind);
     if (op == BinaryOperator::Equality || op == BinaryOperator::Inequality ||
         op == BinaryOperator::CaseEquality || op == BinaryOperator::CaseInequality) {
         flags |= ASTFlags::AllowTypeReferences;
-    }
-
-    Expression *lhs = nullptr, *rhs = nullptr;
-    ExpressionSyntax* syntaxLeft = syntax.left;
-    ExpressionSyntax* syntaxRight = syntax.right;
-    bool notVirtIface = true;
-
-    // Check that there is a comparison with at least one virtual interface.
-    // See IEEE 1800-2017 25.9 clause.
-    if (op == BinaryOperator::Equality || op == BinaryOperator::Inequality) {
-        lhs = tryBindInterfaceRef(context, *syntaxLeft, /* isInterfacePort */ false);
-        if (lhs) {
-            rhs = &selfDetermined(compilation, *syntaxRight, context, flags);
-            notVirtIface = !rhs->type->isVirtualInterface();
-        }
 
-        if (!rhs) {
-            rhs = tryBindInterfaceRef(context, *syntaxRight, /* isInterfacePort */ false);
-            if (rhs) {
-                lhs = &selfDetermined(compilation, *syntaxLeft, context, flags);
-                notVirtIface = !lhs->type->isVirtualInterface();
+        // Special case to handle comparing a virtual interface with an
+        // actual instance. We can't normally bind to an instance from
+        // an expression so we need to explicitly try that separately here.
+        lhs = tryBindInterfaceRef(context, syntaxLeft, /* isInterfacePort */ false);
+        if (!lhs)
+            lhs = &create(compilation, syntaxLeft, context, flags);
+
+        // If we found a virtual interface on the lhs we can also try for an instance
+        // on the rhs. Otherwise we know we're doing normal expression binding.
+        if (lhs->type->isVirtualInterface()) {
+            rhs = tryBindInterfaceRef(context, syntaxRight, /* isInterfacePort */ false);
+            if (!rhs) {
+                rhs = &create(compilation, syntaxRight, context, flags);
             }
+            else if (lhs->kind == ExpressionKind::ArbitrarySymbol &&
+                     rhs->kind == ExpressionKind::ArbitrarySymbol) {
+                // Having an instance on both sides is not allowed. One side must be
+                // an actual virtual interface.
+                context.addDiag(diag::CannotCompareTwoInstances, syntax.operatorToken.location())
+                    << lhs->sourceRange << rhs->sourceRange;
+                return badExpr(compilation, nullptr);
+            }
+        }
+        else {
+            rhs = &create(compilation, syntaxRight, context, flags);
         }
     }
-
-    if (!lhs || !rhs || notVirtIface) {
-        lhs = &create(compilation, *syntaxLeft, context, flags);
-        rhs = &create(compilation, *syntaxRight, context, flags);
+    else {
+        lhs = &create(compilation, syntaxLeft, context, flags);
+        rhs = &create(compilation, syntaxRight, context, flags);
     }
 
     auto& result = fromComponents(*lhs, *rhs, op, syntax.operatorToken.location(),

tests/unittests/ast/MemberTests.cpp

@@ -2679,9 +2679,8 @@ endmodule
     Compilation compilation;
     compilation.addSyntaxTree(tree);
     auto& diags = compilation.getAllDiagnostics();
-    REQUIRE(diags.size() == 2);
-    CHECK(diags[0].code == diag::NotAValue);
-    CHECK(diags[1].code == diag::NotAValue);
+    REQUIRE(diags.size() == 1);
+    CHECK(diags[0].code == diag::CannotCompareTwoInstances);
 }
 
 TEST_CASE("Virtual interfaces of different types comparison") {