diff --git a/verible/common/analysis/linter-test-utils.h b/verible/common/analysis/linter-test-utils.h index 6d04a5602..b34893d3d 100644 --- a/verible/common/analysis/linter-test-utils.h +++ b/verible/common/analysis/linter-test-utils.h @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -114,9 +115,12 @@ void RunLintTestCases(std::initializer_list tests, } struct AutoFixInOut { + static constexpr int NO_FIX_AVAILABLE = -1; + absl::string_view code; absl::string_view expected_output; int fix_alternative = 0; // Some rules provide alternative fixes + int violation_number = 0; }; // Tests that LintTestCase test has expected violations under make_rule @@ -134,10 +138,17 @@ void RunLintAutoFixCase(const AutoFixInOut &test, const LintRuleStatus rule_status = lint_runner.Run(analyzer.Data(), filename); const auto &violations(rule_status.violations); - CHECK_EQ(violations.size(), 1) << "TODO: apply multi-violation fixes"; - CHECK_GT(violations.begin()->autofixes.size(), test.fix_alternative); - const verible::AutoFix &fix = - rule_status.violations.begin()->autofixes[test.fix_alternative]; + CHECK_GT(violations.size(), test.violation_number); + const LintViolation &violation = + *std::next(violations.begin(), test.violation_number); + + if (test.fix_alternative == AutoFixInOut::NO_FIX_AVAILABLE) { + CHECK_EQ(violation.autofixes.size(), 0); + return; + } + + CHECK_GT(violation.autofixes.size(), test.fix_alternative); + const verible::AutoFix &fix = violation.autofixes[test.fix_alternative]; std::string fix_out = fix.Apply(analyzer.Data().Contents()); EXPECT_EQ(test.expected_output, fix_out) << "For input " << test.code; diff --git a/verible/verilog/CST/statement.cc b/verible/verilog/CST/statement.cc index 393c33d18..7cb5a9711 100644 --- a/verible/verilog/CST/statement.cc +++ b/verible/verilog/CST/statement.cc @@ -535,4 +535,34 @@ const verible::SyntaxTreeNode *GetIfHeaderExpression( return verible::GetSubtreeAsNode(*paren_group, NodeEnum::kParenGroup, 1); } +const verible::SyntaxTreeLeaf *GetAssignModifyOperator( + const verible::SyntaxTreeNode &assign_modify) { + return verible::GetSubtreeAsLeaf(assign_modify, + NodeEnum::kAssignModifyStatement, 1); +} + +const verible::SyntaxTreeNode *GetAssignModifyRhs( + const verible::SyntaxTreeNode &assign_modify) { + return verible::GetSubtreeAsNode(assign_modify, + NodeEnum::kAssignModifyStatement, 2); +} + +const verible::SyntaxTreeNode *GetAssignModifyLhs( + const verible::SyntaxTreeNode &assign_modify) { + return verible::GetSubtreeAsNode(assign_modify, + NodeEnum::kAssignModifyStatement, 0); +} + +const verible::SyntaxTreeNode *GetNetVariableAssignmentLhs( + const verible::SyntaxTreeNode &assignment) { + return verible::GetSubtreeAsNode(assignment, NodeEnum::kNetVariableAssignment, + 0); +} + +const verible::SyntaxTreeLeaf *GetNetVariableAssignmentOperator( + const verible::SyntaxTreeNode &assignment) { + return verible::GetSubtreeAsLeaf(assignment, NodeEnum::kNetVariableAssignment, + 1); +} + } // namespace verilog diff --git a/verible/verilog/CST/statement.h b/verible/verilog/CST/statement.h index 2b8ba39a8..9332079fb 100644 --- a/verible/verilog/CST/statement.h +++ b/verible/verilog/CST/statement.h @@ -222,6 +222,31 @@ const verible::SyntaxTreeNode *GetIfClauseHeader( const verible::SyntaxTreeNode *GetIfHeaderExpression( const verible::SyntaxTreeNode &if_header); +// Return the operator from an AssignModifyStatement +// Example: get '&' from 'x &= y' +const verible::SyntaxTreeLeaf *GetAssignModifyOperator( + const verible::SyntaxTreeNode &assign_modify); + +// Return the right hand side (Rhs) from an AssignModifyStatement +// Example: get 'y' from 'x &= y' +const verible::SyntaxTreeNode *GetAssignModifyRhs( + const verible::SyntaxTreeNode &assign_modify); + +// Return the left hand side (Lhs) from an AssignModifyStatement +// Example: get 'x' from 'x &= y' +const verible::SyntaxTreeNode *GetAssignModifyLhs( + const verible::SyntaxTreeNode &assign_modify); + +// Return the left hand side (Lhs) from a NetVariableAssignment +// Example: get 'x' from 'x = y' +const verible::SyntaxTreeNode *GetNetVariableAssignmentLhs( + const verible::SyntaxTreeNode &assignment); + +// Return the operator from a NetVariableAssignment +// Example: get '=' from 'x = y' +const verible::SyntaxTreeLeaf *GetNetVariableAssignmentOperator( + const verible::SyntaxTreeNode &assignment); + } // namespace verilog #endif // VERIBLE_VERILOG_CST_STATEMENT_H_ diff --git a/verible/verilog/CST/statement_test.cc b/verible/verilog/CST/statement_test.cc index e0c7f2d3a..d0951dbd0 100644 --- a/verible/verilog/CST/statement_test.cc +++ b/verible/verilog/CST/statement_test.cc @@ -1585,5 +1585,258 @@ TEST(GetIfClauseExpressionTest, Various) { } } +TEST(GetAssignModifyOperator, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k ", + {kTag, "&="}, + " 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k ", + {kTag, "|="}, + " 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k ", + {kTag, "+="}, + " 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_ff @(posedge clk) begin " + "k <= 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &assign_modify_statements = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekAssignModifyStatement()); + + std::vector operators; + for (const auto &block : assign_modify_statements) { + const auto *operator_ = GetAssignModifyOperator( + verible::SymbolCastToNode(*block.match)); + operators.emplace_back( + TreeSearchMatch{operator_, {/* ignored context */}}); + } + return operators; + }); + } +} + +TEST(GetAssignModifyLhs, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin ", + {kTag, "k"}, + " &= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + {kTag, "k"}, + " |= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + {kTag, "k"}, + " += 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "always_comb begin\n" + "reg k = 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &assign_modify_statements = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekAssignModifyStatement()); + + std::vector left_hand_sides; + for (const auto &block : assign_modify_statements) { + const auto *lhs = + GetAssignModifyLhs(verible::SymbolCastToNode(*block.match)); + left_hand_sides.emplace_back( + TreeSearchMatch{lhs, {/* ignored context */}}); + } + return left_hand_sides; + }); + } +} + +TEST(GetAssignModifyRhs, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin " + "k &= ", + {kTag, "1"}, + ";\nend\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k |= ", + {kTag, "(1 + 2)"}, + ";\nend\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k += ", + {kTag, "1"}, + ";\nend\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k = 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "always_comb begin\n" + "reg k = 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &assign_modify_statements = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekAssignModifyStatement()); + + std::vector right_hand_sides; + for (const auto &block : assign_modify_statements) { + const auto *rhs = + GetAssignModifyRhs(verible::SymbolCastToNode(*block.match)); + right_hand_sides.emplace_back( + TreeSearchMatch{rhs, {/* ignored context */}}); + } + return right_hand_sides; + }); + } +} + +TEST(GetNetVariableAssignmentLhs, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + {kTag, "k"}, + " = 1;\nend\n", + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + "k &= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k |= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_ff begin\n" + "k <= 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &net_var_assignments = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekNetVariableAssignment()); + + std::vector left_hand_sides; + for (const auto &assignment : net_var_assignments) { + const auto *lhs = GetNetVariableAssignmentLhs( + verible::SymbolCastToNode(*assignment.match)); + left_hand_sides.emplace_back( + TreeSearchMatch{lhs, {/* ignored context */}}); + } + return left_hand_sides; + }); + } +} + +TEST(GetNetVariableAssignmentOperator, Various) { + constexpr int kTag = 1; // value doesn't matter + const SyntaxTreeSearchTestCase kTestCases[] = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + "k ", + {kTag, "="}, + " 1;\nend\n", + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n", + "k &= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_comb begin\n" + "k |= 1;\nend\n" + "endmodule\n"}, + {"module m;\n" + "reg k;\n" + "always_ff begin\n" + "k <= 1;\nend\n" + "endmodule\n"}, + }; + for (const auto &test : kTestCases) { + TestVerilogSyntaxRangeMatches( + __FUNCTION__, test, [](const TextStructureView &text_structure) { + const auto &root = text_structure.SyntaxTree(); + const auto &net_var_assignments = SearchSyntaxTree( + *ABSL_DIE_IF_NULL(root), NodekNetVariableAssignment()); + + std::vector left_hand_sides; + for (const auto &assignment : net_var_assignments) { + const auto *lhs = GetNetVariableAssignmentOperator( + verible::SymbolCastToNode(*assignment.match)); + left_hand_sides.emplace_back( + TreeSearchMatch{lhs, {/* ignored context */}}); + } + return left_hand_sides; + }); + } +} + } // namespace } // namespace verilog diff --git a/verible/verilog/analysis/checkers/BUILD b/verible/verilog/analysis/checkers/BUILD index 231f2708a..81c5aebed 100644 --- a/verible/verilog/analysis/checkers/BUILD +++ b/verible/verilog/analysis/checkers/BUILD @@ -1229,13 +1229,17 @@ cc_library( "//verible/common/text:config-utils", "//verible/common/text:symbol", "//verible/common/text:syntax-tree-context", + "//verible/common/text:tree-utils", "//verible/common/util:casts", "//verible/common/util:logging", + "//verible/verilog/CST:expression", + "//verible/verilog/CST:statement", "//verible/verilog/CST:verilog-matchers", "//verible/verilog/CST:verilog-nonterminals", "//verible/verilog/analysis:descriptions", "//verible/verilog/analysis:lint-rule-registry", "@com_google_absl//absl/status", + "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", ], alwayslink = 1, diff --git a/verible/verilog/analysis/checkers/always-ff-non-blocking-rule.cc b/verible/verilog/analysis/checkers/always-ff-non-blocking-rule.cc index 2d7ef57aa..45170c98a 100644 --- a/verible/verilog/analysis/checkers/always-ff-non-blocking-rule.cc +++ b/verible/verilog/analysis/checkers/always-ff-non-blocking-rule.cc @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -15,10 +15,14 @@ #include "verible/verilog/analysis/checkers/always-ff-non-blocking-rule.h" #include +#include #include #include +#include +#include #include "absl/status/status.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "verible/common/analysis/lint-rule-status.h" #include "verible/common/analysis/matcher/bound-symbol-manager.h" @@ -29,8 +33,11 @@ #include "verible/common/text/config-utils.h" #include "verible/common/text/symbol.h" #include "verible/common/text/syntax-tree-context.h" +#include "verible/common/text/tree_utils.h" #include "verible/common/util/casts.h" #include "verible/common/util/logging.h" +#include "verible/verilog/CST/expression.h" +#include "verible/verilog/CST/statement.h" #include "verible/verilog/CST/verilog-matchers.h" #include "verible/verilog/CST/verilog-nonterminals.h" #include "verible/verilog/analysis/descriptions.h" @@ -39,6 +46,7 @@ namespace verilog { namespace analysis { +using verible::AutoFix; using verible::LintRuleStatus; using verible::LintViolation; using verible::SearchSyntaxTree; @@ -102,31 +110,78 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, static const Matcher asgn_incdec_matcher{NodekIncrementDecrementExpression()}; static const Matcher ident_matcher{NodekUnqualifiedId()}; + std::vector autofixes; + // Rule may be waived if complete lhs consists of local variables // -> determine root of lhs const verible::Symbol *check_root = nullptr; + absl::string_view lhs_id; verible::matcher::BoundSymbolManager symbol_man; if (asgn_blocking_matcher.Matches(symbol, &symbol_man)) { - if (const auto *const node = - verible::down_cast(&symbol)) { - check_root = - /* lhs */ verible::down_cast( - node->front().get()); - } + const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol); + check_root = verilog::GetNetVariableAssignmentLhs(*node); + lhs_id = verible::StringSpanOfSymbol(*check_root); + + const verible::SyntaxTreeLeaf *equals = + verilog::GetNetVariableAssignmentOperator(*node); + + autofixes.emplace_back(AutoFix( + "Substitute blocking assignment '=' for nonblocking assignment '<='", + {equals->get(), "<="})); } else { // Not interested in any other blocking assignments unless flagged if (!catch_modifying_assignments_) return; + // These autofixes require substituting the whole expression + absl::string_view original = verible::StringSpanOfSymbol(symbol); if (asgn_modify_matcher.Matches(symbol, &symbol_man)) { - if (const auto *const node = - verible::down_cast(&symbol)) { - check_root = - /* lhs */ verible::down_cast( - node->front().get()); - } + const verible::SyntaxTreeNode *node = &verible::SymbolCastToNode(symbol); + const verible::SyntaxTreeNode &lhs = *GetAssignModifyLhs(*node); + const verible::SyntaxTreeNode &rhs = *GetAssignModifyRhs(*node); + + lhs_id = verible::StringSpanOfSymbol(lhs); + check_root = &lhs; + + bool needs_parenthesis = NeedsParenthesis(rhs); + absl::string_view start_rhs_expr = needs_parenthesis ? " (" : " "; + absl::string_view end_rhs_expr = needs_parenthesis ? ");" : ";"; + + // Extract just the operation. Just '+' from '+=' + const absl::string_view op = + verible::StringSpanOfSymbol(*GetAssignModifyOperator(*node)) + .substr(0, 1); + + const std::string fix = + absl::StrCat(lhs_id, " <= ", lhs_id, " ", op, start_rhs_expr, + verible::StringSpanOfSymbol(rhs), end_rhs_expr); + + autofixes.emplace_back( + AutoFix("Substitute assignment operator for equivalent " + "nonblocking assignment", + {original, fix})); } else if (asgn_incdec_matcher.Matches(symbol, &symbol_man)) { - check_root = &symbol; + const verible::SyntaxTreeNode *operand = + GetIncrementDecrementOperand(symbol); + + check_root = operand; + lhs_id = verible::StringSpanOfSymbol(*operand); + + // Extract just the operation. Just '+' from '++' + const absl::string_view op = + verible::StringSpanOfSymbol(*GetIncrementDecrementOperator(symbol)) + .substr(0, 1); + + // Equivalent nonblocking assignment + // {'x++', '++x'} become 'x <= x + 1' + // {'x--', '--x'} become 'x <= x - 1' + const std::string fix = + absl::StrCat(lhs_id, " <= ", lhs_id, " ", op, " 1;"); + + autofixes.emplace_back( + AutoFix("Substitute increment/decrement operator for " + "equivalent nonblocking assignment", + {original, fix})); } else { // Not a blocking assignment return; @@ -160,7 +215,16 @@ void AlwaysFFNonBlockingRule::HandleSymbol(const verible::Symbol &symbol, } // Enqueue detected violation unless waived - if (!waived) violations_.insert(LintViolation(symbol, kMessage, context)); + if (waived) return; + + // Dont autofix if the faulting expression is inside an expression + // Example: "p <= k++" + if (IsAutoFixSafe(symbol, lhs_id) && + !context.IsInside(NodeEnum::kExpression)) { + violations_.insert(LintViolation(symbol, kMessage, context, autofixes)); + } else { + violations_.insert(LintViolation(symbol, kMessage, context)); + } } // HandleSymbol() bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol, @@ -185,6 +249,7 @@ bool AlwaysFFNonBlockingRule::InsideBlock(const verible::Symbol &symbol, if (always_ff_matcher.Matches(symbol, &symbol_man)) { VLOG(4) << "always_ff @DEPTH=" << depth << std::endl; inside_ = depth; + CollectLocalReferences(symbol); } return false; } @@ -227,5 +292,96 @@ bool AlwaysFFNonBlockingRule::LocalDeclaration(const verible::Symbol &symbol) { return false; } // LocalDeclaration() +bool AlwaysFFNonBlockingRule::NeedsParenthesis( + const verible::Symbol &rhs) const { + // Avoid inserting parenthesis for simple expressions + // For example: x &= 1 -> x <= x & 1, and not x <= x & (1) + // This could be more precise, but checking every specific + // case where parenthesis are needed is hard. Adding them + // doesn't hurt and the user can remove them if needed. + const bool complex_rhs_expr = + verible::GetLeftmostLeaf(rhs) != verible::GetRightmostLeaf(rhs); + if (!complex_rhs_expr) return false; + + // Check if expression is wrapped in parenthesis + const verible::Symbol *inner = verilog::UnwrapExpression(rhs); + if (inner->Kind() == verible::SymbolKind::kLeaf) return false; + + return !verible::SymbolCastToNode(*inner).MatchesTag(NodeEnum::kParenGroup); +} + +void AlwaysFFNonBlockingRule::CollectLocalReferences( + const verible::Symbol &root) { + const Matcher reference_matcher{NodekReference()}; + + const std::vector references_ = + verible::SearchSyntaxTree(root, reference_matcher); + + // Precompute the StringSpan of every identifier referenced to. + // Avoid recomputing it several times + references.resize(references_.size()); + std::transform(references_.cbegin(), references_.cend(), references.begin(), + [](const verible::TreeSearchMatch &match) { + return ReferenceWithId{ + match, verible::StringSpanOfSymbol(*match.match)}; + }); +} + +bool AlwaysFFNonBlockingRule::IsAutoFixSafe( + const verible::Symbol &faulting_assignment, + absl::string_view lhs_id) const { + std::vector::const_iterator itr = references.end(); + + // Let's assume that 'x' is the variable affected by the faulting + // assignment. In order to ensure that the autofix is safe we have + // to ensure that there is no later reference to 'x' + // + // Can't autofix Can autofix + // begin begin + // x = x + 1; x = x + 1; + // y = x; y <= y + 1; + // end end + // + // In practical terms: we'll scan the 'references' vector for kReferences + // to 'x' that appear after the faulting assignment in the always_ff block + + const Matcher reference_matcher{NodekReference()}; + + // Extract kReferences inside the faulting expression + const std::vector references_ = + verible::SearchSyntaxTree(faulting_assignment, reference_matcher); + + // ref points to the latest reference to 'x' in our faulting expression + // 'x++', 'x = x + 1', 'x &= x + 1' + // ^ ^ ^ + // We need this to know from where to start searching for later references + // to 'x', and decide whether the AutoFix is safe or not + const verible::Symbol *ref = + std::find_if(std::rbegin(references_), std::rend(references_), + [&](const verible::TreeSearchMatch &m) { + return verible::StringSpanOfSymbol(*m.match) == lhs_id; + }) + ->match; + + // Shouldn't happen, sanity check to avoid crashes + if (!ref) return false; + + // Find the last reference to 'x' in the faulting assignment + itr = std::find_if( + std::begin(references), std::end(references), + [&](const ReferenceWithId &r) { return r.match.match == ref; }); + + // Search from the last reference to 'x' onwards. If there is any, + // we can't apply the autofix safely + itr = std::find_if( + std::next(itr), std::end(references), + [&](const ReferenceWithId &ref) { return ref.id == lhs_id; }); + + // Let's say 'x' is affected by a flagged operation { 'x = 1', 'x++', ... } + // We can safely autofix if after the flagged operation there are no + // more references to 'x' + return references.end() == itr; +} + } // namespace analysis } // namespace verilog diff --git a/verible/verilog/analysis/checkers/always-ff-non-blocking-rule.h b/verible/verilog/analysis/checkers/always-ff-non-blocking-rule.h index 2693c3725..cea83e0be 100644 --- a/verible/verilog/analysis/checkers/always-ff-non-blocking-rule.h +++ b/verible/verilog/analysis/checkers/always-ff-non-blocking-rule.h @@ -1,4 +1,4 @@ -// Copyright 2017-2020 The Verible Authors. +// Copyright 2017-2023 The Verible Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -24,6 +24,7 @@ #include "absl/strings/string_view.h" #include "verible/common/analysis/lint-rule-status.h" #include "verible/common/analysis/syntax-tree-lint-rule.h" +#include "verible/common/analysis/syntax_tree_search.h" #include "verible/common/text/symbol.h" #include "verible/common/text/syntax-tree-context.h" #include "verible/verilog/analysis/descriptions.h" @@ -48,6 +49,16 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule { bool InsideBlock(const verible::Symbol &symbol, int depth); // Processes local declarations bool LocalDeclaration(const verible::Symbol &symbol); + // Check the need of parenthesis in certain autofixes + // Might have false positives. Example: + // Fixing 'x *= y + 1' requires adding parenthesis + // 'x <= x & (y + 1)' + bool NeedsParenthesis(const verible::Symbol &rhs) const; + // Collect local references inside an always_ff block + void CollectLocalReferences(const verible::Symbol &root); + // Check if is safe to autofix + bool IsAutoFixSafe(const verible::Symbol &faulting_assignment, + absl::string_view lhs_id) const; private: // Collected violations. @@ -72,6 +83,16 @@ class AlwaysFFNonBlockingRule : public verible::SyntaxTreeLintRule { // In-order stack of local variable names std::vector locals_; + + // NodekReference + string representation + // of the ID they're referring to + struct ReferenceWithId { + verible::TreeSearchMatch match; + absl::string_view id; + }; + // Collection of references to variables + // assumed to be in program order + std::vector references; }; } // namespace analysis diff --git a/verible/verilog/analysis/checkers/always-ff-non-blocking-rule_test.cc b/verible/verilog/analysis/checkers/always-ff-non-blocking-rule_test.cc index ac6f17f89..e76eda042 100644 --- a/verible/verilog/analysis/checkers/always-ff-non-blocking-rule_test.cc +++ b/verible/verilog/analysis/checkers/always-ff-non-blocking-rule_test.cc @@ -27,6 +27,7 @@ namespace analysis { namespace { using verible::LintTestCase; +using verible::RunApplyFixCases; using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; @@ -117,6 +118,15 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) { {kToken, "a"}, "= b;\n" "end\nend\nendmodule"}, + // Waive, 'a' is a local declaration + {"module m;\nalways_ff @(posedge c) begin static type(b) a; a = b; " + "end\nendmodule"}, + {"module m;\nalways_ff @(posedge c) begin static type(b) a; a++; " + "end\nendmodule"}, + {"module m;\nalways_ff @(posedge c) begin static type(b) a; ++a; " + "end\nendmodule"}, + {"module m;\nalways_ff @(posedge c) begin static type(b) a; a &= b; " + "end\nendmodule"}, }; RunConfiguredLintTestCases( @@ -124,6 +134,96 @@ TEST(AlwaysFFNonBlockingRule, LocalWaiving) { "catch_modifying_assignments:on;waive_for_locals:on"); } +TEST(AlwaysFFNonBlockingRule, AutoFixDefault) { + const std::initializer_list kTestCases = { + // Check we're not waiving local variables + {"module m;\nalways_ff begin reg k; k = 1; end\nendmodule", + "module m;\nalways_ff begin reg k; k <= 1; end\nendmodule"}, + // Check we're ignoring modifying assignments + {"module m;\nalways_ff begin k &= 1; k = 1; end\nendmodule", + "module m;\nalways_ff begin k &= 1; k <= 1; end\nendmodule"}, + }; + + RunApplyFixCases(kTestCases, ""); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixCatchModifyingAssignments) { + const std::initializer_list kTestCases = { + {"module m;\nalways_ff begin k &= 1; end\nendmodule", + "module m;\nalways_ff begin k <= k & 1; end\nendmodule"}, + {"module m;\nalways_ff begin k &= a; end\nendmodule", + "module m;\nalways_ff begin k <= k & a; end\nendmodule"}, + {"module m;\nalways_ff begin k |= (2 + 1); end\nendmodule", + "module m;\nalways_ff begin k <= k | (2 + 1); end\nendmodule"}, + {"module m;\nalways_ff begin k |= 2 + (1); end\nendmodule", + "module m;\nalways_ff begin k <= k | (2 + (1)); end\nendmodule"}, + {"module m;\nalways_ff begin k |= (2) + (1); end\nendmodule", + "module m;\nalways_ff begin k <= k | ((2) + (1)); end\nendmodule"}, + {"module m;\nalways_ff begin k *= 2 + 1; end\nendmodule", + "module m;\nalways_ff begin k <= k * (2 + 1); end\nendmodule"}, + {"module m;\nalways_ff begin a++; end\nendmodule", + "module m;\nalways_ff begin a <= a + 1; end\nendmodule"}, + {"module m;\nalways_ff begin ++a; end\nendmodule", + "module m;\nalways_ff begin a <= a + 1; end\nendmodule"}, + }; + + RunApplyFixCases( + kTestCases, "catch_modifying_assignments:on"); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixDontBreakCode) { + const std::initializer_list kTestCases = { + // We can't fix 'k &= 1', because it would affect + // 'p <= k' + {"module m;\nalways_ff begin\n" + "k &= 1;\n" + "p <= k;\n" + "a++;" + "end\nendmodule", + "module m;\nalways_ff begin\n" + "k &= 1;\n" + "p <= k;\n" + "a <= a + 1;" + "end\nendmodule", + 0, 1}, + {"module m;\nalways_ff begin\n" + "k &= 1;\n" // First violation, can't be autofixed + "p <= k;\n" + "a++;" + "end\nendmodule", + "module m;\nalways_ff begin\n" + "k &= 1;\n" + "p <= k;\n" + "a <= a + 1;" + "end\nendmodule", + verible::AutoFixInOut::NO_FIX_AVAILABLE, 0}, + // Correctly fix despite there is a reference to 'k' in the rhs + {"module m;\nalways_ff begin k = k + 1; end\nendmodule", + "module m;\nalways_ff begin k <= k + 1; end\nendmodule"}, + // Dont autofix inside expressions + {"module m;\nalways_ff begin k <= p++; p++; end\nendmodule", + "module m;\nalways_ff begin k <= p++; p <= p + 1; end\nendmodule", 0, 1}, + {"module m;\nalways_ff begin k <= p++; p++; end\nendmodule", + "module m;\nalways_ff begin k <= p++; p <= p + 1; end\nendmodule", + verible::AutoFixInOut::NO_FIX_AVAILABLE, 0}, + }; + + RunApplyFixCases( + kTestCases, "catch_modifying_assignments:on"); +} + +TEST(AlwaysFFNonBlockingRule, AutoFixWaiveLocals) { + const std::initializer_list kTestCases = { + // Check that we're correctly waiving the 'p = 0' as it is a local + // variable + {"module m;\nalways_ff begin reg p; p = 0; k = 1; end\nendmodule", + "module m;\nalways_ff begin reg p; p = 0; k <= 1; end\nendmodule"}, + }; + + RunApplyFixCases( + kTestCases, "waive_for_locals:on"); +} + } // namespace } // namespace analysis } // namespace verilog