Improve the error message for non-associative comparisons. (chapel-la…

…ng#24564)

This PR builds on chapel-lang#24155.
Closes chapel-lang#24559.

In chapel-lang#24155, we made `a < b < c`
a syntax error, because it leads to confusing results: a boolean is
implicitly converted to a numeric value, and compared against another
number. I went about disallowing such syntax by simply removing the
associativity from `<`, `>`, etc. However, this meant that Bison simply
reported "syntax error" when it encountered such an expression, which
leaves much to be desired.

This PR improves on that situation by making adjustments to the parser.
Specifically, it enables the parser to note whether or not parentheses
were used for an expression (including an operator comparison). This is
done using the additional location maps added by @dlongnecke-cray in
chapel-lang#23548. The parser is thus
allowed to accept `a < b < c`, and subsequent checks are executed that
detect `a < b < c` (as opposed to `(a < b) < c`, which is valid). These
checks use the Dyno error system to print a nice error message.

<img width="1153" alt="Screen Shot 2024-03-07 at 3 45 34 PM"
src="https://github.com/chapel-lang/chapel/assets/4361282/02ac537a-1b51-4134-b800-1c6986ed9060">

Reviewed by @dlongnecke-cray -- thanks!

## Testing
- [x] paratest
- [x] dyno tests

frontend/include/chpl/parsing/parser-error-classes-list.h

@@ -67,6 +67,7 @@ ERROR_CLASS(InvalidBlockSize, const uast::AstNode*, const uast::Attribute*)
 ERROR_CLASS(InvalidImplementsIdent, const uast::Implements*, const uast::Identifier*)
 ERROR_CLASS(InvalidParenfulDeprecation, const uast::AttributeGroup*, const uast::AstNode*)
 POSTPARSE_ERROR_CLASS(MultipleManagementStrategies, const uast::New::Management, const uast::New::Management)
+ERROR_CLASS(NonAssociativeComparison, const uast::OpCall*, std::vector<const uast::OpCall*>, std::vector<const uast::AstNode*>)
 POSTPARSE_ERROR_CLASS(PostParseErr, std::string)
 POSTPARSE_WARNING_CLASS(PostParseWarn, std::string)
 ERROR_CLASS(UnsupportedAsIdent, const uast::As*, const uast::AstNode*)

frontend/include/chpl/uast/Builder.h

@@ -103,6 +103,7 @@ class Builder final {
                    declaredHereT& duplicates);
 
   void noteAdditionalLocation(AstLocMap& m, AstNode* ast, Location loc);
+  void tryNoteAdditionalLocation(AstLocMap& m, AstNode* ast, Location loc);
 
  public:
   /** Construct a Builder for parsing a top-level module */
@@ -141,7 +142,8 @@ class Builder final {
       Pairs an AST node (e.g., 'Dot') with a location.
       For a list of all locations see "./all-location-maps.h". */
   #define LOCATION_MAP(ast__, location__) \
-    void note##location__##Location(ast__* ast, Location loc);
+    void note##location__##Location(ast__* ast, Location loc); \
+    void tryNote##location__##Location(ast__* ast, Location loc);
   #include "all-location-maps.h"
   #undef LOCATION_MAP
 

frontend/include/chpl/uast/all-location-maps.h

@@ -28,6 +28,7 @@ LOCATION_MAP(NamedDecl, DeclName)
 LOCATION_MAP(Loop, LoopHeader)
 LOCATION_MAP(AstNode, BlockHeader)
 LOCATION_MAP(NamedDecl, DeclHeader)
+LOCATION_MAP(AstNode, ExprParenth)
 // LOCATION_MAP(AstNode, NamedActual)
 // LOCATION_MAP(AstNode, AttributeNamedActual)
 // LOCATION_MAP(Function, ReturnIntent)

frontend/lib/parsing/chpl.ypp

@@ -456,8 +456,8 @@
 %left TALIGN TBY THASH
 %left TOR
 %left TAND
-%nonassoc TEQUAL TNOTEQUAL
-%nonassoc TLESSEQUAL TGREATEREQUAL TLESS TGREATER
+%left TEQUAL TNOTEQUAL
+%left TLESSEQUAL TGREATEREQUAL TLESS TGREATER
 %left TDOTDOT TDOTDOTOPENHIGH
 // These are not currently supported, though we've discussed adding them
 //%left TDOTDOTOPENLOW TDOTDOTOPENBOTH
@@ -3505,7 +3505,12 @@ dot_expr:
  * ( <tuple_expr_ls> ) -- Two-tuples and up.  A tuple_expr_ls contains at least 2 elements.
  */
 parenthesized_expr:
-  TLP tuple_component TRP           { $$ = $2; }
+  TLP tuple_component TRP
+  {
+    // Use 'tryNote' here in case an expression like ((x)) comes along.
+    BUILDER->tryNoteExprParenthLocation($2, LOC(@$));
+    $$ = $2;
+  }
 | TLP tuple_component TCOMMA TRP
   {
     $$ = Tuple::build(BUILDER, LOC(@$), context->consume($2)).release();

frontend/lib/parsing/parser-error-classes-list.cpp

@@ -172,6 +172,78 @@ void ErrorMultipleExternalRenaming::write(ErrorWriterBase& wr) const {
   wr.code(loc);
 }
 
+void ErrorNonAssociativeComparison::write(ErrorWriterBase& wr) const {
+  auto root = std::get<const uast::OpCall*>(info_);
+  auto& ops = std::get<std::vector<const uast::OpCall*>>(info_);
+  auto& operands = std::get<std::vector<const uast::AstNode*>>(info_);
+
+  bool allSimple = std::all_of(operands.begin(), operands.end(), [](auto operand) {
+    return operand->isLiteral() || operand->isIdentifier();
+  });
+
+  // Bitfield: 001 for < or <=, 010 for > or >=, 100 for == or !=
+  // We only want to suggest the "normalized" form if all operators are
+  // in the same direction, and if no equality constraints are present.
+  //
+  // The reason for the latter is, how would you desugar x != y != z?
+  //     x != y && y != z
+  // or
+  //     x != y && y != z && x != z
+  // It's best not to guess there.
+  int types = 0; 
+  for (auto op : ops) {
+    if (op->op() == "<" || op->op() == "<=") {
+      types |= 0b1;
+    } else if (op->op() == ">" || op->op() == ">=") {
+      types |= 0b10;
+    } else if (op->op() == "==" || op->op() == "!=") {
+      types |= 0b100;
+    }
+  }
+
+  wr.heading(kind_, type_, ops.front(),
+             "comparison operators are not associative.");
+  wr.codeForLocation(ops.front());
+  wr.message("Comparisons in the form 'x op y op z' are not supported.");
+
+  if (allSimple && (types == 0b1 || types == 0b10)) {
+    std::ostringstream oss;
+    // Print out what the user's code would look like elementwise
+    for (size_t idx = 0; idx < ops.size(); idx++) {
+      if (idx > 0) {
+        oss << " && ";
+      }
+
+      operands[idx]->stringify(oss, CHPL_SYNTAX);
+      oss << " " << ops[idx]->op() << " ";
+      operands[idx + 1]->stringify(oss, CHPL_SYNTAX);
+    }
+
+    // TODO: this uses explicit indentation using spaces, which is probably bad.
+    wr.message("If you wanted to perform elementwise comparison, consider using "
+               "the following instead:");
+    wr.message("");
+    wr.message("    ", oss.str());
+    wr.message("");
+  } else {
+    wr.message("If you wanted to perform elementwise comaprison, please use "
+               "'&&' to combine operations.");
+  }
+
+  const uast::OpCall* firstNonRoot = nullptr;
+  for (auto op : ops) {
+    if (op != root) {
+      firstNonRoot = op;
+      break;
+    }
+  }
+
+  wr.message("If you wanted to use the result of a comparison as an operand in "
+             "another comparison, consider using parentheses in a subexpression "
+             "to disambiguate:");
+  wr.code(root, { firstNonRoot });
+}
+
 void ErrorNewWithoutArgs::write(ErrorWriterBase& wr) const {
   auto loc = std::get<const Location>(info_);
   auto expr = std::get<const uast::AstNode*>(info_);

frontend/lib/uast/Builder.cpp

@@ -150,10 +150,23 @@ void Builder::noteAdditionalLocation(AstLocMap& m, AstNode* ast,
   m.emplace(ast, std::move(loc));
 }
 
+void Builder::tryNoteAdditionalLocation(AstLocMap& m, AstNode* ast,
+                                     Location loc) {
+  if (!ast || loc.isEmpty()) return;
+  auto found = m.find(ast);
+  if (found == m.end()) {
+    m.emplace_hint(found, ast, std::move(loc));
+  }
+}
+
 #define LOCATION_MAP(ast__, location__) \
   void Builder::note##location__##Location(ast__* ast, Location loc) { \
     auto& m = CHPL_AST_LOC_MAP(ast__, location__); \
     noteAdditionalLocation(m, ast, std::move(loc)); \
+  } \
+  void Builder::tryNote##location__##Location(ast__* ast, Location loc) { \
+    auto& m = CHPL_AST_LOC_MAP(ast__, location__); \
+    tryNoteAdditionalLocation(m, ast, std::move(loc)); \
   }
 #include "chpl/uast/all-location-maps.h"
 #undef LOCATION_MAP

frontend/lib/uast/post-parse-checks.cpp

@@ -23,6 +23,7 @@
 #include "chpl/framework/global-strings.h"
 #include "chpl/parsing/parsing-queries.h"
 #include "chpl/parsing/parser-error.h"
+#include "chpl/uast/chpl-syntax-printer.h"
 #include "chpl/uast/all-uast.h"
 #include <vector>
 #include <string.h>
@@ -119,6 +120,7 @@ struct Visitor {
   void checkSparseKeyword(const FnCall* node);
   void checkPrimCallInUserCode(const PrimCall* node);
   void checkDmappedKeyword(const OpCall* node);
+  void checkNonAssociativeComparisons(const OpCall* node);
   void checkConstVarNoInit(const Variable* node);
   void checkConfigVar(const Variable* node);
   void checkExportVar(const Variable* node);
@@ -709,6 +711,61 @@ void Visitor::checkDmappedKeyword(const OpCall* node) {
            " instead please use factory functions when available");
 }
 
+static int binOpPrecedence(UniqueString ustr) {
+  bool unary = false;
+  bool postfix = false;
+  return opToPrecedence(ustr, unary, postfix);
+}
+
+static void collectEqualPrecedenceOpsWithoutParens(Context* context,
+                                                   const OpCall* node,
+                                                   int prec,
+                                                   std::vector<const OpCall*>& ops,
+                                                   std::vector<const AstNode*>& operands) {
+  auto check = [context, prec, &ops, &operands](const AstNode* child) {
+    if (auto childOp = child->toOpCall()) {
+      if (childOp->numActuals() == 2 && binOpPrecedence(childOp->op()) == prec) {
+        // The child only counts as a 'problem' if it's not parenthesized.
+        if (parsing::locateExprParenthWithAst(context, childOp).line() == -1) {
+          collectEqualPrecedenceOpsWithoutParens(context, childOp, prec, ops, operands);
+          return;
+        }
+      }
+    }
+
+    operands.push_back(child);
+  };
+
+  check(node->actual(0));
+  ops.push_back(node);
+  check(node->actual(1));
+}
+
+void Visitor::checkNonAssociativeComparisons(const OpCall* node) {
+  if (node->numActuals() != 2) return;
+
+  auto lessThanPrec = binOpPrecedence(USTR("<"));
+  auto eqPrec = binOpPrecedence(USTR("=="));
+  auto opPrec = binOpPrecedence(node->op());
+
+  if (opPrec != lessThanPrec && opPrec != eqPrec) return;
+
+  // If the parent is an operator with the same precedence, avoid re-running
+  // the check since the parent would've already tried.
+  if (!parents_.empty()) {
+    auto parentOp = parents_.back()->toOpCall();
+    if (parentOp && binOpPrecedence(parentOp->op()) == opPrec) return;
+  }
+
+  std::vector<const OpCall*> ops;
+  std::vector<const AstNode*> operands;
+  collectEqualPrecedenceOpsWithoutParens(context_, node, opPrec, ops, operands);
+
+  if (ops.size() > 1) {
+    CHPL_REPORT(context_, NonAssociativeComparison, node, ops, operands);
+  }
+}
+
 
 // TODO: Extend to all 'VarLikeDecl' instead of just variables?
 void Visitor::checkConstVarNoInit(const Variable* node) {
@@ -1450,6 +1507,7 @@ void Visitor::visit(const PrimCall* node) {
 
 void Visitor::visit(const OpCall* node) {
   checkDmappedKeyword(node);
+  checkNonAssociativeComparisons(node);
 }
 
 void Visitor::visit(const Variable* node) {

test/errors/parsing/compareAssoc.1.good

@@ -0,0 +1,7 @@
+compareAssoc.chpl:6: error: comparison operators are not associative
+compareAssoc.chpl:10: error: comparison operators are not associative
+compareAssoc.chpl:14: error: comparison operators are not associative
+compareAssoc.chpl:18: error: comparison operators are not associative
+compareAssoc.chpl:22: error: comparison operators are not associative
+compareAssoc.chpl:26: error: comparison operators are not associative
+compareAssoc.chpl:27: error: comparison operators are not associative

test/errors/parsing/compareAssoc.2.good

@@ -0,0 +1,103 @@
+─── error in compareAssoc.chpl:6 [NonAssociativeComparison] ───
+  Comparison operators are not associative.
+      |
+    6 | var x = a < b < c;
+      |
+  Comparisons in the form 'x op y op z' are not supported.
+  If you wanted to perform elementwise comparison, consider using the following instead:
+
+      a < b && b < c
+
+  If you wanted to use the result of a comparison as an operand in another comparison, consider using parentheses in a subexpression to disambiguate:
+      |
+    6 | var x = a < b < c;
+      |         ⎺⎺⎺⎺⎺
+      |
+
+─── error in compareAssoc.chpl:10 [NonAssociativeComparison] ───
+  Comparison operators are not associative.
+       |
+    10 | var x1 = a < b <= c;
+       |
+  Comparisons in the form 'x op y op z' are not supported.
+  If you wanted to perform elementwise comparison, consider using the following instead:
+
+      a < b && b <= c
+
+  If you wanted to use the result of a comparison as an operand in another comparison, consider using parentheses in a subexpression to disambiguate:
+       |
+    10 | var x1 = a < b <= c;
+       |          ⎺⎺⎺⎺⎺
+       |
+
+─── error in compareAssoc.chpl:14 [NonAssociativeComparison] ───
+  Comparison operators are not associative.
+       |
+    14 | var x2 = a > b <= c;
+       |
+  Comparisons in the form 'x op y op z' are not supported.
+  If you wanted to perform elementwise comaprison, please use '&&' to combine operations.
+  If you wanted to use the result of a comparison as an operand in another comparison, consider using parentheses in a subexpression to disambiguate:
+       |
+    14 | var x2 = a > b <= c;
+       |          ⎺⎺⎺⎺⎺
+       |
+
+─── error in compareAssoc.chpl:18 [NonAssociativeComparison] ───
+  Comparison operators are not associative.
+       |
+    18 | var x3 = a == b == c;
+       |
+  Comparisons in the form 'x op y op z' are not supported.
+  If you wanted to perform elementwise comaprison, please use '&&' to combine operations.
+  If you wanted to use the result of a comparison as an operand in another comparison, consider using parentheses in a subexpression to disambiguate:
+       |
+    18 | var x3 = a == b == c;
+       |          ⎺⎺⎺⎺⎺⎺
+       |
+
+─── error in compareAssoc.chpl:22 [NonAssociativeComparison] ───
+  Comparison operators are not associative.
+       |
+    22 | var x4 = a != b != c;
+       |
+  Comparisons in the form 'x op y op z' are not supported.
+  If you wanted to perform elementwise comaprison, please use '&&' to combine operations.
+  If you wanted to use the result of a comparison as an operand in another comparison, consider using parentheses in a subexpression to disambiguate:
+       |
+    22 | var x4 = a != b != c;
+       |          ⎺⎺⎺⎺⎺⎺
+       |
+
+─── error in compareAssoc.chpl:26 [NonAssociativeComparison] ───
+  Comparison operators are not associative.
+       |
+    26 | var x = a < b < c < d;
+       |
+  Comparisons in the form 'x op y op z' are not supported.
+  If you wanted to perform elementwise comparison, consider using the following instead:
+
+      a < b && b < c && c < d
+
+  If you wanted to use the result of a comparison as an operand in another comparison, consider using parentheses in a subexpression to disambiguate:
+       |
+    26 | var x = a < b < c < d;
+       |         ⎺⎺⎺⎺⎺
+       |
+
+─── error in compareAssoc.chpl:27 [NonAssociativeComparison] ───
+  Comparison operators are not associative.
+       |
+    27 | var x1 = a < b <= c <= d;
+       |
+  Comparisons in the form 'x op y op z' are not supported.
+  If you wanted to perform elementwise comparison, consider using the following instead:
+
+      a < b && b <= c && c <= d
+
+  If you wanted to use the result of a comparison as an operand in another comparison, consider using parentheses in a subexpression to disambiguate:
+       |
+    27 | var x1 = a < b <= c <= d;
+       |          ⎺⎺⎺⎺⎺
+       |
+

test/errors/parsing/compareAssoc.chpl

@@ -0,0 +1,27 @@
+var a = 0;
+var b = 0;
+var c = 0;
+var d = 0;
+
+var x = a < b < c;
+var y = (a < b) < c;
+var z = a < (b < c);
+
+var x1 = a < b <= c;
+var y1 = (a < b) < c;
+var z1 = a < (b < c);
+
+var x2 = a > b <= c;
+var y2 = (a > b) < c;
+var z2 = a < (b > c);
+
+var x3 = a == b == c;
+var y3 = (a == b) == c;
+var z3 = a == (b == c);
+
+var x4 = a != b != c;
+var y4 = (a != b) != c;
+var z4 = a != (b != c);
+
+var x = a < b < c < d;
+var x1 = a < b <= c <= d;