diff --git a/include/slang/ast/Expression.h b/include/slang/ast/Expression.h index 9a00ff6bd..1a5b874d6 100644 --- a/include/slang/ast/Expression.h +++ b/include/slang/ast/Expression.h @@ -121,6 +121,14 @@ SLANG_ENUM(RangeSelectionKind, RANGE) #undef RANGE // clang-format on +bool isBitwiseOperator(BinaryOperator op); +bool isComparisonOperator(BinaryOperator op); +bool isShiftOperator(BinaryOperator op); +bool isArithmeticOperator(BinaryOperator op); +bool isRelationalOperator(BinaryOperator op); +std::string_view getOperatorText(BinaryOperator op); +int getOperatorPrecedence(BinaryOperator op); + /// The base class for all expressions in SystemVerilog. class SLANG_EXPORT Expression { public: @@ -354,6 +362,10 @@ class SLANG_EXPORT Expression { /// Returns true if any subexpression of this expression is a hierarchical reference. bool hasHierarchicalReference() const; + /// Returns true if this expression is known to be within a pair of parentheses, + /// and otherwise false. + bool isParenthesized() const; + /// If this expression is an implicit conversion, recursively unwraps to the /// target operand. Otherwise returns `*this`. const Expression& unwrapImplicitConversions() const; diff --git a/include/slang/ast/expressions/OperatorExpressions.h b/include/slang/ast/expressions/OperatorExpressions.h index 20c416b35..4b9efb05b 100644 --- a/include/slang/ast/expressions/OperatorExpressions.h +++ b/include/slang/ast/expressions/OperatorExpressions.h @@ -21,9 +21,13 @@ class SLANG_EXPORT UnaryExpression : public Expression { /// The operator. UnaryOperator op; + /// The source range of the operator token. + SourceRange opRange; + UnaryExpression(UnaryOperator op, const Type& type, Expression& operand, - SourceRange sourceRange) : - Expression(ExpressionKind::UnaryOp, type, sourceRange), op(op), operand_(&operand) {} + SourceRange sourceRange, SourceRange opRange) : + Expression(ExpressionKind::UnaryOp, type, sourceRange), op(op), opRange(opRange), + operand_(&operand) {} /// @returns the operand const Expression& operand() const { return *operand_; } diff --git a/include/slang/parsing/Parser.h b/include/slang/parsing/Parser.h index eb7835f2a..4c0bc6c10 100644 --- a/include/slang/parsing/Parser.h +++ b/include/slang/parsing/Parser.h @@ -463,10 +463,6 @@ 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. diff --git a/include/slang/syntax/SyntaxFacts.h b/include/slang/syntax/SyntaxFacts.h index 6e2bcc95d..fe92d5da7 100644 --- a/include/slang/syntax/SyntaxFacts.h +++ b/include/slang/syntax/SyntaxFacts.h @@ -326,21 +326,6 @@ 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); diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt index 640281043..e2a199081 100644 --- a/scripts/diagnostics.txt +++ b/scripts/diagnostics.txt @@ -311,18 +311,6 @@ 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" @@ -788,6 +776,18 @@ warning arith-op-mismatch ArithOpMismatch "arithmetic between operands of differ warning bitwise-op-mismatch BitwiseOpMismatch "bitwise operation between operands of different types ({} and {})" warning comparison-mismatch ComparisonMismatch "comparison between operands of different types ({} and {})" warning sign-compare SignCompare "comparison of differently signed types ({} and {})" +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 Statements error ReturnNotInSubroutine "return statement is only valid inside task and function blocks" diff --git a/source/ast/Expression.cpp b/source/ast/Expression.cpp index 8fc0ca64e..f99ee803d 100644 --- a/source/ast/Expression.cpp +++ b/source/ast/Expression.cpp @@ -746,6 +746,10 @@ bool Expression::hasHierarchicalReference() const { return visitor.any; } +bool Expression::isParenthesized() const { + return syntax && syntax->kind == SyntaxKind::ParenthesizedExpression; +} + const Expression& Expression::unwrapImplicitConversions() const { auto expr = this; while (expr->kind == ExpressionKind::Conversion) { diff --git a/source/ast/expressions/OperatorExpressions.cpp b/source/ast/expressions/OperatorExpressions.cpp index 8315d0a73..2b1b69d02 100644 --- a/source/ast/expressions/OperatorExpressions.cpp +++ b/source/ast/expressions/OperatorExpressions.cpp @@ -26,6 +26,7 @@ #include "slang/numeric/MathUtils.h" #include "slang/parsing/LexerFacts.h" #include "slang/syntax/AllSyntax.h" +#include "slang/text/SourceManager.h" namespace { @@ -325,7 +326,8 @@ Expression& UnaryExpression::fromSyntax(Compilation& compilation, Expression& operand = create(compilation, *syntax.operand, context, extraFlags); const Type* type = operand.type; - auto result = compilation.emplace(op, *type, operand, syntax.sourceRange()); + auto result = compilation.emplace(op, *type, operand, syntax.sourceRange(), + syntax.operatorToken.range()); if (operand.bad()) return badExpr(compilation, result); @@ -405,7 +407,8 @@ Expression& UnaryExpression::fromSyntax(Compilation& compilation, const Type* type = operand.type; Expression* result = compilation.emplace(getUnaryOperator(syntax.kind), *type, - operand, syntax.sourceRange()); + operand, syntax.sourceRange(), + syntax.operatorToken.range()); if (operand.bad() || !operand.requireLValue(context, syntax.operatorToken.location())) return badExpr(compilation, result); @@ -428,13 +431,13 @@ Expression& UnaryExpression::fromSyntax(Compilation& compilation, } bool UnaryExpression::propagateType(const ASTContext& context, const Type& newType, - SourceRange opRange) { + SourceRange propRange) { switch (op) { case UnaryOperator::Plus: case UnaryOperator::Minus: case UnaryOperator::BitwiseNot: type = &newType; - contextDetermined(context, operand_, this, newType, opRange); + contextDetermined(context, operand_, this, newType, propRange); return true; case UnaryOperator::BitwiseAnd: case UnaryOperator::BitwiseOr: @@ -679,6 +682,131 @@ Expression& BinaryExpression::fromSyntax(Compilation& compilation, return result; } +static void analyzePrecedence(const ASTContext& context, const Expression& lhs, + const Expression& rhs, BinaryOperator op, SourceRange opRange) { + auto getBin = [](const Expression& expr) { + return expr.isParenthesized() ? nullptr : expr.as_if(); + }; + + // Warn when mixing bitwise ops and comparisons -- comparisons have higher precedence. + // ex: `flags & 'h20 != 0` is equivalent to `flags & 1` + if (isBitwiseOperator(op)) { + auto lhsBin = getBin(lhs); + auto rhsBin = getBin(rhs); + + const bool isLeftComp = lhsBin && isComparisonOperator(lhsBin->op); + const bool isRightComp = rhsBin && isComparisonOperator(rhsBin->op); + if (isLeftComp != isRightComp) { + // Bitwise operations are sometimes used as eager logical ops. + // Don't diagnose this. + const bool isLeftBitwise = lhsBin && isBitwiseOperator(lhsBin->op); + const bool isRightBitwise = rhsBin && isBitwiseOperator(rhsBin->op); + if (!isLeftBitwise && !isRightBitwise) { + auto opStr = getOperatorText(op); + auto compOpStr = isLeftComp ? getOperatorText(lhsBin->op) + : getOperatorText(rhsBin->op); + auto compRange = isLeftComp ? lhs.sourceRange : rhs.sourceRange; + + auto& diag = context.addDiag(diag::BitwiseRelPrecedence, opRange); + 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 = [&] { + if (auto sm = context.getCompilation().getSourceManager()) + return sm->isMacroLoc(opRange.start()); + return false; + }; + + auto warnAWithinB = [&](DiagCode code, const BinaryExpression& binOp) { + auto binOpText = getOperatorText(binOp.op); + auto& diag = context.addDiag(code, binOp.opRange); + diag << binOpText << getOperatorText(op); + diag << binOp.sourceRange << opRange; + diag.addNote(diag::NotePrecedenceSilence, binOp.opRange) << binOpText << binOp.sourceRange; + }; + + // Warn about `a & b | c` constructs (except in macros). + if ((op == BinaryOperator::BinaryOr || op == BinaryOperator::BinaryXor || + op == BinaryOperator::BinaryXnor) && + !isInMacro()) { + + auto check = [&](const Expression& expr) { + if (auto binOp = getBin(expr)) { + if (isBitwiseOperator(binOp->op) && + getOperatorPrecedence(binOp->op) > getOperatorPrecedence(op)) { + warnAWithinB(diag::BitwiseOpParentheses, *binOp); + } + } + }; + + check(lhs); + check(rhs); + } + + // Warn about `a && b || c` constructs (except in macros). + if (op == BinaryOperator::LogicalOr && !isInMacro()) { + auto check = [&](const Expression& expr) { + if (auto binOp = getBin(expr)) { + if (binOp->op == BinaryOperator::LogicalAnd) + warnAWithinB(diag::LogicalOpParentheses, *binOp); + } + }; + + check(lhs); + check(rhs); + } + + // Warn about `x << 1 + 1` constructs. + if (isShiftOperator(op)) { + auto check = [&](const Expression& expr) { + if (auto binOp = getBin(expr)) { + if (binOp->op == BinaryOperator::Add || binOp->op == BinaryOperator::Subtract) { + auto binOpText = getOperatorText(binOp->op); + auto& diag = context.addDiag(diag::ArithInShift, binOp->opRange); + diag << getOperatorText(op) << binOpText << binOpText; + diag << binOp->sourceRange << opRange; + diag.addNote(diag::NotePrecedenceSilence, binOp->opRange) + << binOpText << binOp->sourceRange; + } + } + }; + + check(lhs); + check(rhs); + } + + // Warn about `!x < y` and `!x & y` where they probably meant `!(x < y)` and `!(x & y)` + if ((isComparisonOperator(op) || op == BinaryOperator::BinaryAnd) && + lhs.kind == ExpressionKind::UnaryOp && !lhs.isParenthesized() && + lhs.as().op == UnaryOperator::LogicalNot) { + + auto kindStr = op == BinaryOperator::BinaryAnd ? "bitwise operator"sv : "comparison"sv; + auto& unary = lhs.as(); + auto& diag = context.addDiag(diag::LogicalNotParentheses, unary.opRange); + diag << kindStr << opRange; + + SourceRange range(unary.operand().sourceRange.start(), 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(op)) { + if (auto binOp = getBin(lhs); binOp && isRelationalOperator(binOp->op)) + context.addDiag(diag::ConsecutiveComparison, opRange); + } +} + Expression& BinaryExpression::fromComponents(Expression& lhs, Expression& rhs, BinaryOperator op, SourceRange opRange, SourceRange sourceRange, const ASTContext& context) { @@ -901,6 +1029,8 @@ Expression& BinaryExpression::fromComponents(Expression& lhs, Expression& rhs, B return badExpr(compilation, result); } + analyzePrecedence(context, lhs, rhs, op, opRange); + auto& clt = lt->getCanonicalType(); auto& crt = rt->getCanonicalType(); if (!clt.isMatching(crt)) { @@ -1371,6 +1501,25 @@ Expression& ConditionalExpression::fromSyntax(Compilation& comp, return badExpr(comp, result); } + // 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 (conditions.size() == 1 && !conditions[0].pattern) { + if (auto binOp = conditions[0].expr->as_if(); + binOp && !binOp->isParenthesized()) { + if (isArithmeticOperator(binOp->op) || isShiftOperator(binOp->op)) { + auto opStr = getOperatorText(binOp->op); + auto& diag = context.addDiag(diag::ConditionalPrecedence, + syntax.question.location()); + diag << opStr << opStr << binOp->sourceRange; + + SourceRange range(binOp->right().sourceRange.start(), right.sourceRange.end()); + diag.addNote(diag::NoteConditionalPrecedenceFix, range); + diag.addNote(diag::NotePrecedenceSilence, binOp->sourceRange) << opStr; + } + } + } + context.setAttributes(*result, syntax.attributes); return *result; } @@ -2641,4 +2790,182 @@ ConstantValue Expression::evalBinaryOperator(BinaryOperator op, const ConstantVa SLANG_UNREACHABLE; } +bool isBitwiseOperator(BinaryOperator op) { + switch (op) { + case BinaryOperator::BinaryOr: + case BinaryOperator::BinaryAnd: + case BinaryOperator::BinaryXor: + case BinaryOperator::BinaryXnor: + return true; + default: + return false; + } +} + +bool isComparisonOperator(BinaryOperator op) { + switch (op) { + case BinaryOperator::Equality: + case BinaryOperator::Inequality: + case BinaryOperator::CaseEquality: + case BinaryOperator::CaseInequality: + case BinaryOperator::WildcardEquality: + case BinaryOperator::WildcardInequality: + case BinaryOperator::GreaterThanEqual: + case BinaryOperator::GreaterThan: + case BinaryOperator::LessThanEqual: + case BinaryOperator::LessThan: + return true; + default: + return false; + } +} + +bool isShiftOperator(BinaryOperator op) { + switch (op) { + case BinaryOperator::LogicalShiftLeft: + case BinaryOperator::LogicalShiftRight: + case BinaryOperator::ArithmeticShiftLeft: + case BinaryOperator::ArithmeticShiftRight: + return true; + default: + return false; + } +} + +bool isArithmeticOperator(BinaryOperator op) { + switch (op) { + case BinaryOperator::Add: + case BinaryOperator::Subtract: + case BinaryOperator::Multiply: + case BinaryOperator::Divide: + case BinaryOperator::Mod: + case BinaryOperator::Power: + return true; + default: + return false; + } +} + +bool isRelationalOperator(BinaryOperator op) { + switch (op) { + case BinaryOperator::GreaterThanEqual: + case BinaryOperator::GreaterThan: + case BinaryOperator::LessThanEqual: + case BinaryOperator::LessThan: + return true; + default: + return false; + } +} + +std::string_view getOperatorText(BinaryOperator op) { + switch (op) { + case BinaryOperator::Add: + return "+"; + case BinaryOperator::Subtract: + return "-"; + case BinaryOperator::Multiply: + return "*"; + case BinaryOperator::Divide: + return "/"; + case BinaryOperator::Mod: + return "%"; + case BinaryOperator::BinaryAnd: + return "&"; + case BinaryOperator::BinaryOr: + return "|"; + case BinaryOperator::BinaryXor: + return "^"; + case BinaryOperator::BinaryXnor: + return "~^"; + case BinaryOperator::Equality: + return "=="; + case BinaryOperator::Inequality: + return "!="; + case BinaryOperator::CaseEquality: + return "==="; + case BinaryOperator::CaseInequality: + return "!=="; + case BinaryOperator::GreaterThanEqual: + return ">="; + case BinaryOperator::GreaterThan: + return ">"; + case BinaryOperator::LessThanEqual: + return "<="; + case BinaryOperator::LessThan: + return "<"; + case BinaryOperator::WildcardEquality: + return "==?"; + case BinaryOperator::WildcardInequality: + return "!=?"; + case BinaryOperator::LogicalAnd: + return "&&"; + case BinaryOperator::LogicalOr: + return "||"; + case BinaryOperator::LogicalImplication: + return "->"; + case BinaryOperator::LogicalEquivalence: + return "<->"; + case BinaryOperator::LogicalShiftLeft: + return "<<"; + case BinaryOperator::LogicalShiftRight: + return ">>"; + case BinaryOperator::ArithmeticShiftLeft: + return "<<<"; + case BinaryOperator::ArithmeticShiftRight: + return ">>>"; + case BinaryOperator::Power: + return "**"; + } + SLANG_UNREACHABLE; +} + +int getOperatorPrecedence(BinaryOperator op) { + // Note: the precedence levels here match what is returned by + // SyntaxFacts::getPrecedence. + switch (op) { + case BinaryOperator::LogicalImplication: + case BinaryOperator::LogicalEquivalence: + return 2; + case BinaryOperator::LogicalOr: + return 3; + case BinaryOperator::LogicalAnd: + return 4; + case BinaryOperator::BinaryOr: + return 5; + case BinaryOperator::BinaryXor: + case BinaryOperator::BinaryXnor: + return 6; + case BinaryOperator::BinaryAnd: + return 7; + case BinaryOperator::Equality: + case BinaryOperator::Inequality: + case BinaryOperator::CaseEquality: + case BinaryOperator::CaseInequality: + case BinaryOperator::WildcardEquality: + case BinaryOperator::WildcardInequality: + return 8; + case BinaryOperator::GreaterThanEqual: + case BinaryOperator::GreaterThan: + case BinaryOperator::LessThanEqual: + case BinaryOperator::LessThan: + return 9; + case BinaryOperator::LogicalShiftLeft: + case BinaryOperator::LogicalShiftRight: + case BinaryOperator::ArithmeticShiftLeft: + case BinaryOperator::ArithmeticShiftRight: + return 10; + case BinaryOperator::Add: + case BinaryOperator::Subtract: + return 11; + case BinaryOperator::Multiply: + case BinaryOperator::Divide: + case BinaryOperator::Mod: + return 12; + case BinaryOperator::Power: + return 13; + } + SLANG_UNREACHABLE; +} + } // namespace slang::ast diff --git a/source/parsing/Parser_expressions.cpp b/source/parsing/Parser_expressions.cpp index 7eb29fb74..123b7910b 100644 --- a/source/parsing/Parser_expressions.cpp +++ b/source/parsing/Parser_expressions.cpp @@ -9,7 +9,6 @@ #include "slang/parsing/Lexer.h" #include "slang/parsing/Parser.h" #include "slang/parsing/Preprocessor.h" -#include "slang/text/SourceManager.h" namespace slang::parsing { @@ -103,125 +102,6 @@ ExpressionSyntax& Parser::parseSubExpression(bitmask 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(); - auto rhsBin = rhs.as_if(); - - 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()) { - 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()) { - 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()) { - 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(); - 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 options, int precedence) { @@ -285,7 +165,6 @@ 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)) @@ -313,24 +192,6 @@ 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()) { - 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; - } - } - } } } diff --git a/source/syntax/SyntaxFacts.cpp b/source/syntax/SyntaxFacts.cpp index 84f34e942..1c36e637d 100644 --- a/source/syntax/SyntaxFacts.cpp +++ b/source/syntax/SyntaxFacts.cpp @@ -1497,74 +1497,6 @@ 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