From 33749fe3698fbad860c584a9035db0143a90056c Mon Sep 17 00:00:00 2001 From: Lukasz Dalek Date: Fri, 2 Apr 2021 12:49:32 +0200 Subject: [PATCH] linter: Add forbid-implicit-declarations rule Signed-off-by: Lukasz Dalek --- verilog/analysis/checkers/BUILD | 42 ++++ .../forbid_implicit_declarations_rule.cc | 119 +++++++++ .../forbid_implicit_declarations_rule.h | 70 ++++++ .../forbid_implicit_declarations_rule_test.cc | 234 ++++++++++++++++++ verilog/analysis/default_rules.h | 1 + verilog/tools/lint/BUILD | 1 + .../testdata/forbid_implicit_declarations.sv | 3 + .../testdata/suggest_parentheses_example.sv | 2 + 8 files changed, 472 insertions(+) create mode 100644 verilog/analysis/checkers/forbid_implicit_declarations_rule.cc create mode 100644 verilog/analysis/checkers/forbid_implicit_declarations_rule.h create mode 100644 verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc create mode 100644 verilog/tools/lint/testdata/forbid_implicit_declarations.sv diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index d10874622..7c5b42a4e 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -30,6 +30,7 @@ cc_library( ":explicit_task_lifetime_rule", ":forbid_consecutive_null_statements_rule", ":forbid_defparam_rule", + ":forbid_implicit_declarations_rule", ":forbidden_anonymous_enums_rule", ":forbidden_anonymous_structs_unions_rule", ":forbidden_macro_rule", @@ -357,6 +358,47 @@ cc_test( ], ) +cc_library( + name = "forbid_implicit_declarations_rule", + srcs = ["forbid_implicit_declarations_rule.cc"], + hdrs = ["forbid_implicit_declarations_rule.h"], + deps = [ + "//common/analysis:citation", + "//common/analysis:lint_rule_status", + "//common/analysis:text_structure_lint_rule", + "//common/analysis/matcher", + "//common/analysis/matcher:bound_symbol_manager", + "//common/analysis/matcher:core_matchers", + "//common/analysis/matcher:matcher_builders", + "//common/text:symbol", + "//common/text:tree_context_visitor", + "//common/util:auto_pop_stack", + "//verilog/CST:identifier", + "//verilog/CST:verilog_matchers", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint_rule_registry", + "//verilog/analysis:symbol_table", + "//verilog/analysis:verilog_project", + "@com_google_absl//absl/strings", + ], + alwayslink = 1, +) + +cc_test( + name = "forbid_implicit_declarations_rule_test", + srcs = ["forbid_implicit_declarations_rule_test.cc"], + deps = [ + ":forbid_implicit_declarations_rule", + "//common/analysis:linter_test_utils", + "//common/analysis:text_structure_linter_test_utils", + "//common/text:symbol", + "//verilog/CST:verilog_nonterminals", + "//verilog/CST:verilog_treebuilder_utils", + "//verilog/analysis:verilog_analyzer", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "mismatched_labels_rule", srcs = ["mismatched_labels_rule.cc"], diff --git a/verilog/analysis/checkers/forbid_implicit_declarations_rule.cc b/verilog/analysis/checkers/forbid_implicit_declarations_rule.cc new file mode 100644 index 000000000..e1ae41600 --- /dev/null +++ b/verilog/analysis/checkers/forbid_implicit_declarations_rule.cc @@ -0,0 +1,119 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/forbid_implicit_declarations_rule.h" + +#include +#include + +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" +#include "common/analysis/citation.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/matcher/bound_symbol_manager.h" +#include "common/text/symbol.h" +#include "common/text/syntax_tree_context.h" +#include "common/text/tree_context_visitor.h" +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/lint_rule_registry.h" +#include "verilog/CST/identifier.h" +#include "verilog/analysis/symbol_table.h" + +namespace verilog { +namespace analysis { + +using verible::GetStyleGuideCitation; +using verible::LintRuleStatus; +using verible::LintViolation; +using verible::SyntaxTreeContext; + +// Register ForbidImplicitDeclarationsRule +VERILOG_REGISTER_LINT_RULE(ForbidImplicitDeclarationsRule); + +// forbid-implicit-net-declarations? +absl::string_view ForbidImplicitDeclarationsRule::Name() { + return "forbid-implicit-declarations"; +} +const char ForbidImplicitDeclarationsRule::kTopic[] = "implicit-declarations"; +const char ForbidImplicitDeclarationsRule::kMessage[] = + "Nets must be declared explicitly."; + +std::string ForbidImplicitDeclarationsRule::GetDescription(DescriptionType description_type) { + return absl::StrCat("Checks that there are no occurrences of " + "implicitly declared nets."); +} + +void ForbidImplicitDeclarationsRule::Lint(const verible::TextStructureView& text_structure, + absl::string_view filename) { + SymbolTable symbol_table(nullptr); + + ParsedVerilogSourceFile* src = + new ParsedVerilogSourceFile("internal", &text_structure); + // Already parsed, calling to ensure that VerilogSourceFile internals are in + // correct state + const auto status =src->Parse(); + CHECK_EQ(status.ok(), true); + + auto diagnostics = BuildSymbolTable(*src, &symbol_table); + for (const auto& itr : diagnostics) { + CHECK_EQ(itr.ok(), true); + } + // Skipping resolving stage as implicit declarations are pre-resolved + // during symbol table building stage + + auto& violations = this->violations_; + symbol_table.Root().ApplyPreOrder( + [&violations, &text_structure](const SymbolTableNode& node) { + for (const auto& itr : node.Value().local_references_to_bind) { + ABSL_DIE_IF_NULL(itr.LastLeaf())->ApplyPreOrder( + [&violations, &text_structure](const ReferenceComponent& node) { + // Skip unresolved symbols (implicit declarations are pre-resolved) + if (node.resolved_symbol == nullptr) { + return; + } + + const auto& resolved_symbol_node = + *ABSL_DIE_IF_NULL(node.resolved_symbol); + const auto& resolved_symbol = + resolved_symbol_node.Value(); + const auto& resolved_symbol_identifier = + *ABSL_DIE_IF_NULL(resolved_symbol_node.Key()); + + // Skip pre-resolved symbols that have explicit declarations + if (resolved_symbol.declared_type.implicit == false) { + return; + } + + // Only report reference that caused implicit declarations + if (node.identifier.begin() == resolved_symbol_identifier.begin()) { + const auto offset = std::distance(text_structure.Contents().begin(), + node.identifier.begin()); + CHECK_GE(offset, 0); + auto range = text_structure.TokenRangeSpanningOffsets(offset, offset); + auto token = range.begin(); + CHECK(token != text_structure.TokenStream().end()); + const auto& token_info = *token; + violations.insert(LintViolation(token_info, kMessage)); + } + }); + } + }); +} + +LintRuleStatus ForbidImplicitDeclarationsRule::Report() const { + return LintRuleStatus(violations_, Name(), GetStyleGuideCitation(kTopic)); +} + +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/checkers/forbid_implicit_declarations_rule.h b/verilog/analysis/checkers/forbid_implicit_declarations_rule.h new file mode 100644 index 000000000..33e5c81b2 --- /dev/null +++ b/verilog/analysis/checkers/forbid_implicit_declarations_rule.h @@ -0,0 +1,70 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_FORBID_IMPLICIT_DECLARATIONS_RULE_H_ +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_FORBID_IMPLICIT_DECLARATIONS_RULE_H_ + +#include +#include + +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/matcher/core_matchers.h" +#include "common/analysis/matcher/matcher.h" +#include "common/analysis/matcher/matcher_builders.h" +#include "common/analysis/text_structure_lint_rule.h" +#include "common/text/symbol.h" +#include "common/text/syntax_tree_context.h" +#include "common/text/tree_context_visitor.h" +#include "common/util/auto_pop_stack.h" +#include "verilog/CST/verilog_matchers.h" // IWYU pragma: keep +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/symbol_table.h" +#include "verilog/analysis/verilog_project.h" + +namespace verilog { +namespace analysis { + +// ForbidImplicitDeclarationsRule detect implicitly declared nets +class ForbidImplicitDeclarationsRule : public verible::TextStructureLintRule { + //public ScopeTreeVisitor { + public: + using rule_type = verible::TextStructureLintRule; + static absl::string_view Name(); + + // Returns the description of the rule implemented formatted for either the + // helper flag or markdown depending on the parameter type. + static std::string GetDescription(DescriptionType); + + // Analyze text structure for violations. + void Lint(const verible::TextStructureView& text_structure, + absl::string_view filename) override; + + verible::LintRuleStatus Report() const override; + + private: + + private: + // Link to style guide rule. + static const char kTopic[]; + + // Diagnostic message. + static const char kMessage[]; + + std::set violations_; +}; + +} // namespace analysis +} // namespace verilog + +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_FORBID_IMPLICIT_DECLARATIONS_RULE_H_ diff --git a/verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc b/verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc new file mode 100644 index 000000000..f4a5513c6 --- /dev/null +++ b/verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc @@ -0,0 +1,234 @@ +// Copyright 2017-2020 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. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/forbid_implicit_declarations_rule.h" + +#include + +#include "gtest/gtest.h" +#include "common/analysis/linter_test_utils.h" +#include "common/analysis/text_structure_linter_test_utils.h" +#include "common/text/symbol.h" +#include "verilog/CST/verilog_nonterminals.h" +#include "verilog/CST/verilog_treebuilder_utils.h" +#include "verilog/analysis/verilog_analyzer.h" + +namespace verilog { +namespace analysis { +namespace { + +using verible::LintTestCase; +using verible::RunLintTestCases; + +TEST(ForbidImplicitDeclarationsRule, FunctionFailures) { + auto kToken = SymbolIdentifier; + const std::initializer_list ForbidImplicitDeclarationsTestCases = { + {""}, + {"module m;\nendmodule\n"}, + {"module m;\nassign ", {kToken, "a1"}, " = 1'b0;\nendmodule"}, + {"module m;\n" + " wire a1;\n" + " assign a1 = 1'b0;\n" + "endmodule"}, + {"module m;\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + " module foo;\n" + " endmodule\n" + "endmodule"}, + {"module m;\n" + " module foo;\n" + " endmodule\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + "endmodule"}, + {"module m;\n" + " wire a1;\n" + " module foo;\n" + " assign a1 = 1'b0;\n" + " endmodule;\n" + "endmodule"}, + + // declaration and assignement separated by module block + {"module m;\n" + " wire a1;\n" + " module foo;\n" + " endmodule;\n" + " assign a1 = 1'b0;\n" + "endmodule"}, + {"module m;\n" + " wire a1;\n" + " module foo;\n" + " assign a1 = 1'b0;\n" + " endmodule\n" + " assign a1 = 1'b0;\n" + "endmodule"}, + + // overlapping net + {"module m;\n" + " wire a1;\n" + " module foo;\n" + " wire a1;\n" + " assign a1 = 1'b0;\n" + " endmodule\n" + " assign a1 = 1'b0;\n" + "endmodule"}, + + // multiple declarations + {"module m;\n" + " wire a0, a1;\n" + " assign a0 = 1'b0;\n" + " assign a1 = 1'b1;\n" + "endmodule"}, + {"module m;\n" + " wire a0, a2;\n" + " assign a0 = 1'b0;\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + "endmodule"}, + + // multiple net assignments + {"module m;\n" + " assign ", {kToken, "a"}, " = b, ", {kToken, "c"}, " = d;\n" + "endmodule"}, + + // concatenated + {"module m;\n" + " assign {", {kToken, "a"}, "} = 1'b0;\n" + "endmodule"}, + {"module m;\n" + " assign {", {kToken, "a"}, ",", {kToken, "b"}, "} = 2'b01;\n" + "endmodule"}, + {"module m;\n" + " assign {", {kToken, "a"}, ",", {kToken, "b"}, ",", {kToken, "c"}, "} = 3'b010;\n" + "endmodule"}, + {"module m;\n" + " wire b;\n" + " assign {", {kToken, "a"}, ", b,", {kToken, "c"}, "} = 3'b010;\n" + "endmodule"}, + + // out-of-scope + {"module m;\n" + " module foo;\n" + " wire a1;\n" + " endmodule\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + "endmodule"}, + {"module m;\n" + " module foo;\n" + " wire a1;\n" + " assign a1 = 1'b0;\n" + " endmodule\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + "endmodule"}, + {"module m;\n" + " wire a1;\n" + " module foo;\n" + " assign a1 = 1'b0;\n" + " endmodule\n" + " assign a1 = 1'b1;\n" + "endmodule"}, + + // multi-level module blocks + {"module m1;\n" + " wire x1;\n" + " module m2;\n" + " wire x2;\n" + " module m3;\n" + " wire x3;\n" + " module m4;\n" + " wire x4;\n" + " assign x4 = 1'b0;\n" + " assign x3 = 1'b0;\n" + " assign x2 = 1'b0;\n" + " assign x1 = 1'b0;\n" + " endmodule\n" + " assign ", {kToken, "x4"}, " = 1'b0;\n" + " assign x3 = 1'b1;\n" + " assign x2 = 1'b0;\n" + " assign x1 = 1'b0;\n" + " endmodule\n" + " assign ", {kToken, "x4"}, " = 1'b0;\n" + " assign ", {kToken, "x3"}, " = 1'b0;\n" + " assign x2 = 1'b0;\n" + " assign x1 = 1'b0;\n" + " endmodule\n" + " assign ", {kToken, "x4"}, " = 1'b0;\n" + " assign ", {kToken, "x3"}, " = 1'b0;\n" + " assign ", {kToken, "x2"}, " = 1'b0;\n" + " assign x1 = 1'b1;\n" + "endmodule"}, + + // generate block, TODO: multi-level + {"module m;\ngenerate\nendgenerate\nendmodule"}, + {"module m;\n" + " wire a1;\n" + " assign a1 = 1'b1;\n" + " generate\n" + " endgenerate\n" + " assign a1 = 1'b0;\n" + "endmodule"}, + {"module m;\n" + " generate\n" + " wire a1;\n" + " assign a1 = 1'b1;\n" + " endgenerate\n" + "endmodule"}, + {"module m;\n" + " generate\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + " endgenerate\n" + "endmodule"}, + {"module m;\n" + " generate\n" + " wire a1;\n" + " assign a1 = 1'b1;\n" + " endgenerate\n" + " assign a1 = 1'b1;\n" + "endmodule"}, + {"module m;\n" + " wire a1;\n" + " assign a1 = 1'b1;\n" + " generate\n" + " assign a1 = 1'b1;\n" + " endgenerate\n" + " assign a1 = 1'b0;\n" + "endmodule"}, + {"module m;\n" + " wire a1;\n" + " generate\n" + " wire a2\n" + " assign a1 = 1'b1;\n" + " endgenerate\n" + " assign ", {kToken, "a2"}, " = 1'b0;\n" + "endmodule"}, + {"module m;\n" + " wire a1;\n" + " generate\n" + " wire a2;\n" + " assign a1 = 1'b1;\n" + " assign a2 = a1;\n" + " endgenerate\n" + " assign a2 = 1'b0;\n" + " assign a1 = a2;\n" + "endmodule"}, + + // TODO: nets declared inside terminal/port connection list + // TODO: assignments/connections inside loop and conditional generate constructs + }; + + RunLintTestCases( + ForbidImplicitDeclarationsTestCases); +} + +} // namespace +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/default_rules.h b/verilog/analysis/default_rules.h index 620e5fc53..55e45f459 100644 --- a/verilog/analysis/default_rules.h +++ b/verilog/analysis/default_rules.h @@ -70,6 +70,7 @@ constexpr const char* kDefaultRuleSet[] = { // TODO(b/117131903): "proper-parameter-declaration", // TODO(b/131637160): "signal-name-style", // TODO(b/120784977): "numeric-format-string-style", + // TODO(b/138353038): "forbid-implicit-declaration", // TODO: "one-module-per-file", // TODO: "banned-declared-name-patterns", // TODO: "port-name-suffix", diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 5e328165c..421bf76d4 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -34,6 +34,7 @@ _linter_test_configs = [ ("explicit-task-lifetime", "explicit_task_lifetime", True), ("forbid-consecutive-null-statements", "forbid_consecutive_null_statements", True), ("forbid-line-continuations", "forbid_line_continuations", True), + ("forbid-implicit-declarations", "forbid_implicit_declarations", False), ("generate-label", "generate_label_module", True), ("generate-label", "generate-label-module-body", True), # uses parse directive ("generate-label-prefix", "generate_label_prefix", True), diff --git a/verilog/tools/lint/testdata/forbid_implicit_declarations.sv b/verilog/tools/lint/testdata/forbid_implicit_declarations.sv new file mode 100644 index 000000000..b1d4dcc61 --- /dev/null +++ b/verilog/tools/lint/testdata/forbid_implicit_declarations.sv @@ -0,0 +1,3 @@ +module forbid_implicit_declarations; + assign a1 = 1'b0; // [Style: implicit-declarations] [forbid-implicit-declarations] +endmodule diff --git a/verilog/tools/lint/testdata/suggest_parentheses_example.sv b/verilog/tools/lint/testdata/suggest_parentheses_example.sv index ce3d1bc6e..6c363ab98 100644 --- a/verilog/tools/lint/testdata/suggest_parentheses_example.sv +++ b/verilog/tools/lint/testdata/suggest_parentheses_example.sv @@ -1,3 +1,5 @@ module suggest_parentheses_example; + wire foo; + assign foo = condition_a? condition_b? a : b : c; endmodule