From a49439ea425910c7c9e518332490b1bf97ce9576 Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Fri, 4 Oct 2024 22:27:59 +0200 Subject: [PATCH 1/5] linter_test_utils: implement multi-violation AutoFix tests Allow AutoFix test cases to specify the violation for which the AutoFix test is intended. Keep backwards compatibility by defaulting the new field to 0, given that existing tests assume there is only violation per test case. --- verible/common/analysis/linter-test-utils.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/verible/common/analysis/linter-test-utils.h b/verible/common/analysis/linter-test-utils.h index 6d04a5602..320e545e5 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 @@ -117,6 +118,7 @@ struct AutoFixInOut { 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 +136,11 @@ 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); + 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; From ee31efb6eecb6b608fae520d863da70e1ae37f38 Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Tue, 19 Nov 2024 19:43:19 +0100 Subject: [PATCH 2/5] linter_test_utils: allow testing that a LintViolation has no autofixes Autofixes might be undesirable in some scenarios, for example: they change the behavior of the original code. --- verible/common/analysis/linter-test-utils.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/verible/common/analysis/linter-test-utils.h b/verible/common/analysis/linter-test-utils.h index 320e545e5..b34893d3d 100644 --- a/verible/common/analysis/linter-test-utils.h +++ b/verible/common/analysis/linter-test-utils.h @@ -115,6 +115,8 @@ 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 @@ -139,6 +141,12 @@ void RunLintAutoFixCase(const AutoFixInOut &test, 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()); From d3e58a0ecce3902f88e731880f6deb4dab269a02 Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Tue, 19 Nov 2024 20:12:18 +0100 Subject: [PATCH 3/5] verilog: CST: add getters for AssignModifyStatements --- verible/verilog/CST/statement.cc | 17 +++ verible/verilog/CST/statement.h | 15 +++ verible/verilog/CST/statement_test.cc | 160 ++++++++++++++++++++++++++ 3 files changed, 192 insertions(+) diff --git a/verible/verilog/CST/statement.cc b/verible/verilog/CST/statement.cc index 393c33d18..4b57cd38a 100644 --- a/verible/verilog/CST/statement.cc +++ b/verible/verilog/CST/statement.cc @@ -535,4 +535,21 @@ 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); +} } // namespace verilog diff --git a/verible/verilog/CST/statement.h b/verible/verilog/CST/statement.h index 2b8ba39a8..c8b5432a1 100644 --- a/verible/verilog/CST/statement.h +++ b/verible/verilog/CST/statement.h @@ -222,6 +222,21 @@ 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); + } // 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..7bc7f1e2c 100644 --- a/verible/verilog/CST/statement_test.cc +++ b/verible/verilog/CST/statement_test.cc @@ -1585,5 +1585,165 @@ 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; + }); + } +} + } // namespace } // namespace verilog From 1aa1e75f364c05e37867ef6019769c21f5edb497 Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Tue, 19 Nov 2024 20:14:02 +0100 Subject: [PATCH 4/5] verilog: CST: add getters for NetVariableAssignment --- verible/verilog/CST/statement.cc | 13 ++++ verible/verilog/CST/statement.h | 10 +++ verible/verilog/CST/statement_test.cc | 93 +++++++++++++++++++++++++++ 3 files changed, 116 insertions(+) diff --git a/verible/verilog/CST/statement.cc b/verible/verilog/CST/statement.cc index 4b57cd38a..7cb5a9711 100644 --- a/verible/verilog/CST/statement.cc +++ b/verible/verilog/CST/statement.cc @@ -552,4 +552,17 @@ const verible::SyntaxTreeNode *GetAssignModifyLhs( 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 c8b5432a1..9332079fb 100644 --- a/verible/verilog/CST/statement.h +++ b/verible/verilog/CST/statement.h @@ -237,6 +237,16 @@ const verible::SyntaxTreeNode *GetAssignModifyRhs( 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 7bc7f1e2c..d0951dbd0 100644 --- a/verible/verilog/CST/statement_test.cc +++ b/verible/verilog/CST/statement_test.cc @@ -1745,5 +1745,98 @@ TEST(GetAssignModifyRhs, Various) { } } +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 From db7c12c7a506680012d7a1d13d4ad76330c52f65 Mon Sep 17 00:00:00 2001 From: Ignacio Encinas Date: Fri, 4 Oct 2024 22:27:59 +0200 Subject: [PATCH 5/5] verilog: analysis: checkers: add autofixes for always_ff_non_blocking_rule Offer autofixes without modifying program behaviour by taking care of corner cases such as: always_ff @(posedge clk) begin x++; y <= x; end which can't be converted to always_ff @(posedge clk) begin x <= x + 1; y <= x; end Similarly, parenthesis are inserted where they might be needed: a *= b + 1; becomes a <= b * (k + 1); --- verible/verilog/analysis/checkers/BUILD | 4 + .../checkers/always-ff-non-blocking-rule.cc | 186 ++++++++++++++++-- .../checkers/always-ff-non-blocking-rule.h | 23 ++- .../always-ff-non-blocking-rule_test.cc | 100 ++++++++++ 4 files changed, 297 insertions(+), 16 deletions(-) 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