Add Wparentheses group of warnings

include/slang/parsing/Parser.h

@@ -463,6 +463,10 @@ class SLANG_EXPORT Parser : ParserBase, syntax::SyntaxFacts {
     void checkEmptyBody(const syntax::SyntaxNode& syntax, Token prevToken,
                         std::string_view syntaxName);
 
+    void analyzePrecedence(const syntax::ExpressionSyntax& left,
+                           const syntax::ExpressionSyntax& right, syntax::SyntaxKind opKind,
+                           Token opToken);
+
     // ---- Member variables ----
 
     // The factory used to create new syntax nodes.

include/slang/syntax/SyntaxFacts.h

@@ -326,6 +326,21 @@ class SLANG_EXPORT SyntaxFacts {
     /// @return true if the given syntax kind is an assignment operator.
     static bool isAssignmentOperator(SyntaxKind kind);
 
+    /// @return true if the given syntax kind is a bitwise operator.
+    static bool isBitwiseOperator(SyntaxKind kind);
+
+    /// @return true if the given syntax kind is a comparison operator.
+    static bool isComparisonOperator(SyntaxKind kind);
+
+    /// @return true if the given syntax kind is a shift operator.
+    static bool isShiftOperator(SyntaxKind kind);
+
+    /// @return true if the given syntax kind is an arithmetic operator.
+    static bool isArithmeticOperator(SyntaxKind kind);
+
+    /// @return true if the given syntax kind is a relational operator.
+    static bool isRelationalOperator(SyntaxKind kind);
+
     /// @return a string representing the name of the given data type, if it has a simple name.
     static std::string_view getSimpleTypeName(const DataTypeSyntax& syntax);
 

scripts/diagnostics.txt

@@ -311,6 +311,18 @@ warning split-distweight-op SplitDistWeightOp "split dist weight operator is not
 note NoteToMatchThis "to match this '{}'"
 note NoteLastBlockStarted "last complete block started here"
 note NoteLastBlockEnded "and ended here"
+warning bitwise-rel-precedence BitwiseRelPrecedence "'{}' has lower precedence than '{}'; '{}' will be evaluated first"
+note NotePrecedenceBitwiseFirst "place parentheses around the '{}' expression to evaluate it first"
+note NotePrecedenceSilence "place parentheses around the '{}' expression to silence this warning"
+warning bitwise-op-parentheses BitwiseOpParentheses "'{}' within '{}'"
+warning logical-op-parentheses LogicalOpParentheses "'{}' within '{}'"
+warning arith-in-shift ArithInShift "'{}' has lower precedence than '{}'; '{}' will be evaluated first"
+warning logical-not-parentheses LogicalNotParentheses "logical not is only applied to the left hand side of this {}"
+note NoteLogicalNotFix "add parentheses after the '!' to evaluate the {} first"
+note NoteLogicalNotSilence "add parentheses around left hand side expression to silence this warning"
+warning conditional-precedence ConditionalPrecedence "operator '?:' has lower precedence than '{}'; '{}' will be evaluated first"
+note NoteConditionalPrecedenceFix "place parentheses around the '?:' expression to evaluate it first"
+warning consecutive-comparison ConsecutiveComparison "comparisons like 'x < y < z' don't have their mathematical meaning"
 
 subsystem Declarations
 error LocalParamNoInitializer "local parameter is missing an initializer"
@@ -1151,7 +1163,8 @@ group extra = { empty-member empty-stmt dup-import pointless-void-cast case-gen-
                 unused-result format-real ignored-slice task-ignored width-trunc dup-attr event-const
                 ineffective-sign port-width-trunc constant-conversion float-bool-conv int-bool-conv
                 useless-cast unsigned-arith-shift static-init-order static-init-value float-int-conv
-                float-narrow }
+                float-narrow bitwise-rel-precedence arith-in-shift logical-not-parentheses
+                consecutive-comparison }
 
 group pedantic = { empty-pattern implicit-net-port nonstandard-escape-code nonstandard-generate format-multibit-strength
                    nonstandard-sys-func nonstandard-foreach nonstandard-dist isunbounded-param-arg udp-coverage }
@@ -1164,3 +1177,6 @@ group unused = { unused-def unused-net unused-implicit-net unused-variable undri
                  unused-but-set-net unused-but-set-variable unused-port undriven-port unused-argument
                  unused-parameter unused-type-parameter unused-typedef unused-genvar unused-assertion-decl
                  unused-but-set-port unused-config-cell unused-config-instance unused-import unused-wildcard-import }
+
+group parentheses = { bitwise-rel-precedence arith-in-shift logical-not-parentheses bitwise-op-parentheses
+                      logical-op-parentheses conditional-precedence consecutive-comparison }

scripts/warning_docs.txt

@@ -1393,3 +1393,88 @@ module m;
     initial b = a;
 endmodule
 ```
+
+-Wbitwise-rel-precedence
+Identifies a bitwise operator used in a relational expression without parentheses,
+which can indicate a mistake in the intended order of operations.
+```
+module m;
+    int unsigned flags;
+    initial begin
+        if (flags & 'h1 == 'h1) begin end
+    end
+endmodule
+```
+
+-Warith-in-shift
+Identifies an arithmetic operator used in a shift expression, which can
+indicate a mistake in the intended order of operations.
+```
+module m;
+    logic a;
+    initial begin
+        if (a << 1 + 1) begin end
+    end
+endmodule
+```
+
+-Wlogical-not-parentheses
+Identifies a logical not operator used without parentheses around the operand,
+followed by a comparison operator. This can indicate a mistake in the intended
+order of operations.
+```
+module m;
+    logic a, b;
+    initial begin
+        if (!a < b) begin end
+    end
+endmodule
+```
+
+-Wbitwise-op-parentheses
+Identifies a mix of bitwise operators with different levels of precedence without
+any parentheses, which can indicate a mistake in the intended order of operations.
+```
+module m;
+    logic a, b, c;
+    initial begin
+        if (a & b | c) begin end
+    end
+endmodule
+```
+
+-Wlogical-op-parentheses
+Identifies a mix of logical operators with different levels of precedence without
+any parentheses, which can indicate a mistake in the intended order of operations.
+```
+module m;
+    logic a, b, c;
+    initial begin
+        if (a || b && c) begin end
+    end
+endmodule
+```
+
+-Wconditional-precedence
+Identifies a conditional expression mixed with a binary expression in way that
+suggests a different intended order of operations.
+```
+module m;
+    logic a, b;
+    initial begin
+        if ((a + b ? 1 : 2) == 2) begin end
+    end
+endmodule
+```
+
+-Wconsecutive-comparison
+Identifies a sequence of comparison operators that suggest an incorrect notion
+of how the comparisons will be evaluated.
+```
+module m;
+    logic a, b, c;
+    initial begin
+        if (a < b < c) begin end
+    end
+endmodule
+```

source/parsing/Parser_expressions.cpp

@@ -9,6 +9,7 @@
 #include "slang/parsing/Lexer.h"
 #include "slang/parsing/Parser.h"
 #include "slang/parsing/Preprocessor.h"
+#include "slang/text/SourceManager.h"
 
 namespace slang::parsing {
 
@@ -102,6 +103,125 @@ ExpressionSyntax& Parser::parseSubExpression(bitmask<ExpressionOptions> options,
     return parseBinaryExpression(leftOperand, options, precedence);
 }
 
+void Parser::analyzePrecedence(const ExpressionSyntax& lhs, const ExpressionSyntax& rhs,
+                               SyntaxKind opKind, Token opToken) {
+    // Warn when mixing bitwise ops and comparisons -- comparisons have higher precedence.
+    // ex: `flags & 'h20 != 0` is equivalent to `flags & 1`
+    if (isBitwiseOperator(opKind)) {
+        auto lhsBin = lhs.as_if<BinaryExpressionSyntax>();
+        auto rhsBin = rhs.as_if<BinaryExpressionSyntax>();
+
+        const bool isLeftComp = lhsBin && isComparisonOperator(lhsBin->kind);
+        const bool isRightComp = rhsBin && isComparisonOperator(rhsBin->kind);
+        if (isLeftComp != isRightComp) {
+            // Bitwise operations are sometimes used as eager logical ops.
+            // Don't diagnose this.
+            const bool isLeftBitwise = lhsBin && isBitwiseOperator(lhsBin->kind);
+            const bool isRightBitwise = rhsBin && isBitwiseOperator(rhsBin->kind);
+            if (!isLeftBitwise && !isRightBitwise) {
+                auto opStr = opToken.valueText();
+                auto compOpStr = isLeftComp ? lhsBin->operatorToken.valueText()
+                                            : rhsBin->operatorToken.valueText();
+                auto compRange = isLeftComp ? lhs.sourceRange() : rhs.sourceRange();
+
+                auto& diag = addDiag(diag::BitwiseRelPrecedence, opToken.range());
+                diag << opStr << compOpStr << compOpStr << compRange;
+
+                auto rewrittenRange =
+                    isLeftComp
+                        ? SourceRange(lhsBin->right->sourceRange().start(), rhs.sourceRange().end())
+                        : SourceRange(lhs.sourceRange().start(), rhsBin->left->sourceRange().end());
+                diag.addNote(diag::NotePrecedenceBitwiseFirst, rewrittenRange) << opStr;
+                diag.addNote(diag::NotePrecedenceSilence, compRange) << compOpStr;
+            }
+        }
+    }
+
+    auto isInMacro = [&] { return getPP().getSourceManager().isMacroLoc(opToken.location()); };
+
+    auto warnAWithinB = [&](DiagCode code, const BinaryExpressionSyntax& binOp) {
+        auto binOpTok = binOp.operatorToken;
+        auto& diag = addDiag(code, binOpTok.range());
+        diag << binOpTok.valueText() << opToken.valueText();
+        diag << binOp.sourceRange() << opToken.range();
+        diag.addNote(diag::NotePrecedenceSilence, binOpTok.range())
+            << binOpTok.valueText() << binOp.sourceRange();
+    };
+
+    // Warn about `a & b | c` constructs (except in macros).
+    if ((opKind == SyntaxKind::BinaryOrExpression || opKind == SyntaxKind::BinaryXorExpression ||
+         opKind == SyntaxKind::BinaryXnorExpression) &&
+        !isInMacro()) {
+
+        auto check = [&](const ExpressionSyntax& expr) {
+            if (auto binOp = expr.as_if<BinaryExpressionSyntax>()) {
+                if (isBitwiseOperator(binOp->kind) &&
+                    getPrecedence(binOp->kind) > getPrecedence(opKind)) {
+                    warnAWithinB(diag::BitwiseOpParentheses, *binOp);
+                }
+            }
+        };
+
+        check(lhs);
+        check(rhs);
+    }
+
+    // Warn about `a && b || c` constructs (except in macros).
+    if (opKind == SyntaxKind::LogicalOrExpression && !isInMacro()) {
+        auto check = [&](const ExpressionSyntax& expr) {
+            if (auto binOp = expr.as_if<BinaryExpressionSyntax>()) {
+                if (binOp->kind == SyntaxKind::LogicalAndExpression)
+                    warnAWithinB(diag::LogicalOpParentheses, *binOp);
+            }
+        };
+
+        check(lhs);
+        check(rhs);
+    }
+
+    // Warn about `x << 1 + 1` constructs.
+    if (isShiftOperator(opKind)) {
+        auto check = [&](const ExpressionSyntax& expr) {
+            if (auto binOp = expr.as_if<BinaryExpressionSyntax>()) {
+                if (binOp->kind == SyntaxKind::AddExpression ||
+                    binOp->kind == SyntaxKind::SubtractExpression) {
+
+                    auto binOpTok = binOp->operatorToken;
+                    auto& diag = addDiag(diag::ArithInShift, binOpTok.range());
+                    diag << opToken.valueText() << binOpTok.valueText() << binOpTok.valueText();
+                    diag << binOp->sourceRange() << opToken.range();
+                    diag.addNote(diag::NotePrecedenceSilence, binOpTok.range())
+                        << binOpTok.valueText() << binOp->sourceRange();
+                }
+            }
+        };
+
+        check(lhs);
+        check(rhs);
+    }
+
+    // Warn about `!x < y` and `!x & y` where they probably meant `!(x < y)` and `!(x & y)`
+    if ((isComparisonOperator(opKind) || opKind == SyntaxKind::BinaryAndExpression) &&
+        lhs.kind == SyntaxKind::UnaryLogicalNotExpression) {
+
+        auto kindStr = opKind == SyntaxKind::BinaryAndExpression ? "bitwise operator"sv
+                                                                 : "comparison"sv;
+        auto& unary = lhs.as<PrefixUnaryExpressionSyntax>();
+        auto notOpTok = unary.operatorToken;
+        auto& diag = addDiag(diag::LogicalNotParentheses, notOpTok.location());
+        diag << kindStr << opToken.range();
+
+        SourceRange range(unary.operand->getFirstToken().location(), rhs.sourceRange().end());
+        diag.addNote(diag::NoteLogicalNotFix, range) << kindStr;
+        diag.addNote(diag::NoteLogicalNotSilence, lhs.sourceRange());
+    }
+
+    // Warn about comparisons like `x < y < z` which doesn't compare the way you'd
+    // mathematically expect.
+    if (isRelationalOperator(opKind) && isRelationalOperator(lhs.kind))
+        addDiag(diag::ConsecutiveComparison, opToken.range());
+}
+
 ExpressionSyntax& Parser::parseBinaryExpression(ExpressionSyntax* left,
                                                 bitmask<ExpressionOptions> options,
                                                 int precedence) {
@@ -165,6 +285,7 @@ ExpressionSyntax& Parser::parseBinaryExpression(ExpressionSyntax* left,
             auto opToken = consume();
             auto attributes = parseAttributes();
             auto& rightOperand = parseSubExpression(options, newPrecedence);
+            analyzePrecedence(*left, rightOperand, opKind, opToken);
             left = &factory.binaryExpression(opKind, *left, opToken, attributes, rightOperand);
 
             if (!attributes.empty() && isAssignmentOperator(opKind))
@@ -192,6 +313,24 @@ ExpressionSyntax& Parser::parseBinaryExpression(ExpressionSyntax* left,
             auto colon = expect(TokenKind::Colon);
             auto& rhs = parseSubExpression(options, logicalOrPrecedence - 1);
             left = &factory.conditionalExpression(predicate, question, attributes, lhs, colon, rhs);
+
+            // Warn about cases where a conditional expression and a binary operator
+            // are mixed in a way that suggests the user mixed up the precedence order.
+            // ex: `a + b ? 1 : 2`
+            if (predicate.conditions.size() == 1 && !predicate.conditions[0]->matchesClause) {
+                if (auto binOp = predicate.conditions[0]->expr->as_if<BinaryExpressionSyntax>()) {
+                    if (isArithmeticOperator(binOp->kind) || isShiftOperator(binOp->kind)) {
+                        auto opStr = binOp->operatorToken.valueText();
+                        auto& diag = addDiag(diag::ConditionalPrecedence, question.location());
+                        diag << opStr << opStr << binOp->sourceRange();
+
+                        SourceRange range(binOp->right->getFirstToken().location(),
+                                          rhs.sourceRange().end());
+                        diag.addNote(diag::NoteConditionalPrecedenceFix, range);
+                        diag.addNote(diag::NotePrecedenceSilence, binOp->sourceRange()) << opStr;
+                    }
+                }
+            }
         }
     }
 

source/syntax/SyntaxFacts.cpp

@@ -1497,6 +1497,74 @@ bool SyntaxFacts::isAssignmentOperator(SyntaxKind kind) {
     }
 }
 
+bool SyntaxFacts::isBitwiseOperator(SyntaxKind kind) {
+    switch (kind) {
+        case SyntaxKind::BinaryOrExpression:
+        case SyntaxKind::BinaryAndExpression:
+        case SyntaxKind::BinaryXorExpression:
+        case SyntaxKind::BinaryXnorExpression:
+            return true;
+        default:
+            return false;
+    }
+}
+
+bool SyntaxFacts::isComparisonOperator(SyntaxKind kind) {
+    switch (kind) {
+        case SyntaxKind::EqualityExpression:
+        case SyntaxKind::InequalityExpression:
+        case SyntaxKind::CaseEqualityExpression:
+        case SyntaxKind::CaseInequalityExpression:
+        case SyntaxKind::WildcardEqualityExpression:
+        case SyntaxKind::WildcardInequalityExpression:
+        case SyntaxKind::LessThanExpression:
+        case SyntaxKind::LessThanEqualExpression:
+        case SyntaxKind::GreaterThanExpression:
+        case SyntaxKind::GreaterThanEqualExpression:
+            return true;
+        default:
+            return false;
+    }
+}
+
+bool SyntaxFacts::isShiftOperator(SyntaxKind kind) {
+    switch (kind) {
+        case SyntaxKind::LogicalShiftLeftExpression:
+        case SyntaxKind::LogicalShiftRightExpression:
+        case SyntaxKind::ArithmeticShiftLeftExpression:
+        case SyntaxKind::ArithmeticShiftRightExpression:
+            return true;
+        default:
+            return false;
+    }
+}
+
+bool SyntaxFacts::isArithmeticOperator(SyntaxKind kind) {
+    switch (kind) {
+        case SyntaxKind::AddExpression:
+        case SyntaxKind::SubtractExpression:
+        case SyntaxKind::MultiplyExpression:
+        case SyntaxKind::DivideExpression:
+        case SyntaxKind::ModExpression:
+        case SyntaxKind::PowerExpression:
+            return true;
+        default:
+            return false;
+    }
+}
+
+bool SyntaxFacts::isRelationalOperator(SyntaxKind kind) {
+    switch (kind) {
+        case SyntaxKind::LessThanExpression:
+        case SyntaxKind::LessThanEqualExpression:
+        case SyntaxKind::GreaterThanExpression:
+        case SyntaxKind::GreaterThanEqualExpression:
+            return true;
+        default:
+            return false;
+    }
+}
+
 // clang-format on
 
 } // namespace slang::syntax