From 30f1c907282a7ee3bc4a9ba5b6f79e9d88339f2d Mon Sep 17 00:00:00 2001 From: Tomasz Gorochowik Date: Fri, 21 Feb 2020 14:09:36 -0800 Subject: [PATCH] PR #204: Forbid parameter names starting with 'disable', suggest 'enable' instead. Fixes #162 This is affected by #203 so I think it makes sense to wait for that to be fixed first. GitHub PR https://github.com/google/verible/pull/204 Copybara import of the project: - 8da43b623fb79013a36a7085f2b9c96ae4ed5bcd Add positive meaning parameter name linter rule by Tomasz Gorochowik - 616a4185ca9b041723dbc6292b0b3ad7d8cf1ce0 Add unit tests for positive meaning paramater name rule by Tomasz Gorochowik - 4b45b130ee9ce7969465bd135e45ec49441de759 Add integration test for positive meaning paramater name ... by Tomasz Gorochowik Closes #204 PiperOrigin-RevId: 296508575 --- verilog/analysis/checkers/BUILD | 40 +++++ .../positive_meaning_parameter_name_rule.cc | 83 +++++++++ .../positive_meaning_parameter_name_rule.h | 67 ++++++++ ...sitive_meaning_parameter_name_rule_test.cc | 159 ++++++++++++++++++ verilog/tools/lint/BUILD | 1 + .../positive_meaning_parameter_name.sv | 2 + 6 files changed, 352 insertions(+) create mode 100644 verilog/analysis/checkers/positive_meaning_parameter_name_rule.cc create mode 100644 verilog/analysis/checkers/positive_meaning_parameter_name_rule.h create mode 100644 verilog/analysis/checkers/positive_meaning_parameter_name_rule_test.cc create mode 100644 verilog/tools/lint/testdata/positive_meaning_parameter_name.sv diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 9bb7271c0..b263f2b0c 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -48,6 +48,7 @@ cc_library( ":parameter_name_style_rule", ":parameter_type_name_style_rule", ":plusarg_assignment_rule", + ":positive_meaning_parameter_name_rule", ":posix_eof_rule", ":proper_parameter_declaration_rule", ":signal_name_style_rule", @@ -121,6 +122,45 @@ cc_test( ], ) +cc_library( + name = "positive_meaning_parameter_name_rule", + srcs = ["positive_meaning_parameter_name_rule.cc"], + hdrs = ["positive_meaning_parameter_name_rule.h"], + deps = [ + "//common/analysis:citation", + "//common/analysis:lint_rule_status", + "//common/analysis:syntax_tree_lint_rule", + "//common/analysis/matcher", + "//common/analysis/matcher:bound_symbol_manager", + "//common/analysis/matcher:matcher_builders", + "//common/strings:naming_utils", + "//common/text:symbol", + "//common/text:syntax_tree_context", + "//common/text:token_info", + "//verilog/CST:parameters", + "//verilog/CST:verilog_matchers", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint_rule_registry", + "//verilog/parser:verilog_token_enum", + "@com_google_absl//absl/strings", + ], + alwayslink = 1, +) + +cc_test( + name = "positive_meaning_parameter_name_rule_test", + srcs = ["positive_meaning_parameter_name_rule_test.cc"], + deps = [ + ":positive_meaning_parameter_name_rule", + "//common/analysis:linter_test_utils", + "//common/analysis:syntax_tree_linter_test_utils", + "//common/text:symbol", + "//verilog/CST:verilog_nonterminals", + "//verilog/analysis:verilog_analyzer", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "posix_eof_rule", srcs = ["posix_eof_rule.cc"], diff --git a/verilog/analysis/checkers/positive_meaning_parameter_name_rule.cc b/verilog/analysis/checkers/positive_meaning_parameter_name_rule.cc new file mode 100644 index 000000000..e9a3437ec --- /dev/null +++ b/verilog/analysis/checkers/positive_meaning_parameter_name_rule.cc @@ -0,0 +1,83 @@ +// 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/positive_meaning_parameter_name_rule.h" + +#include +#include + +#include "absl/strings/match.h" +#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/strings/naming_utils.h" +#include "common/text/symbol.h" +#include "common/text/syntax_tree_context.h" +#include "common/text/token_info.h" +#include "verilog/CST/parameters.h" +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/lint_rule_registry.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { + +using verible::GetStyleGuideCitation; +using verible::LintRuleStatus; +using verible::LintViolation; +using verible::SyntaxTreeContext; + +// Register PositiveMeaningParameterNameRule. +VERILOG_REGISTER_LINT_RULE(PositiveMeaningParameterNameRule); + +absl::string_view PositiveMeaningParameterNameRule::Name() { + return "positive-meaning-parameter-name"; +} +const char PositiveMeaningParameterNameRule::kTopic[] = "binary-parameters"; + +const char PositiveMeaningParameterNameRule::kMessage[] = + "Use positive naming for parameters, start the name with enable instead."; + +std::string PositiveMeaningParameterNameRule::GetDescription( + DescriptionType description_type) { + return absl::StrCat( + "Checks that no parameter name starts with 'disable', using positive " + "naming (starting with 'enable') is recommended. See ", + GetStyleGuideCitation(kTopic), "."); +} + +void PositiveMeaningParameterNameRule::HandleSymbol( + const verible::Symbol& symbol, const SyntaxTreeContext& context) { + verible::matcher::BoundSymbolManager manager; + if (matcher_.Matches(symbol, &manager)) { + if (IsParamTypeDeclaration(symbol)) return; + + auto identifiers = GetAllParameterNameTokens(symbol); + for (const auto& id : identifiers) { + const auto param_name = id->text; + + if (absl::StartsWithIgnoreCase(param_name, "disable")) + violations_.insert(LintViolation(*id, kMessage, context)); + } + } +} + +LintRuleStatus PositiveMeaningParameterNameRule::Report() const { + return LintRuleStatus(violations_, Name(), GetStyleGuideCitation(kTopic)); +} + +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/checkers/positive_meaning_parameter_name_rule.h b/verilog/analysis/checkers/positive_meaning_parameter_name_rule.h new file mode 100644 index 000000000..27559b3ce --- /dev/null +++ b/verilog/analysis/checkers/positive_meaning_parameter_name_rule.h @@ -0,0 +1,67 @@ +// 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_POSITIVE_MEANING_PARAMETER_NAME_RULE_H_ // NOLINT +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_POSITIVE_MEANING_PARAMETER_NAME_RULE_H_ // NOLINT + +#include +#include + +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/matcher/matcher.h" +#include "common/analysis/matcher/matcher_builders.h" +#include "common/analysis/syntax_tree_lint_rule.h" +#include "common/text/symbol.h" +#include "common/text/syntax_tree_context.h" +#include "verilog/CST/verilog_matchers.h" // IWYU pragma: keep +#include "verilog/analysis/descriptions.h" + +namespace verilog { +namespace analysis { + +// PositiveMeaningParameterNameRule checks that no parameter/localparam +// name starts with 'disable'. If so, it suggests using 'enable' instead. +class PositiveMeaningParameterNameRule : public verible::SyntaxTreeLintRule { + public: + using rule_type = verible::SyntaxTreeLintRule; + 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); + + void HandleSymbol(const verible::Symbol& symbol, + const verible::SyntaxTreeContext& context) override; + + verible::LintRuleStatus Report() const override; + + private: + // Link to style guide rule. + static const char kTopic[]; + + // Diagnostic message for parameter name violations. + static const char kMessage[]; + + using Matcher = verible::matcher::Matcher; + + Matcher matcher_ = NodekParamDeclaration(); + + std::set violations_; +}; + +} // namespace analysis +} // namespace verilog + +// clang-format off +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_POSITIVE_MEANING_PARAMETER_NAME_RULE_H_ // NOLINT diff --git a/verilog/analysis/checkers/positive_meaning_parameter_name_rule_test.cc b/verilog/analysis/checkers/positive_meaning_parameter_name_rule_test.cc new file mode 100644 index 000000000..2ed6e10e3 --- /dev/null +++ b/verilog/analysis/checkers/positive_meaning_parameter_name_rule_test.cc @@ -0,0 +1,159 @@ +// 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/positive_meaning_parameter_name_rule.h" + +#include + +#include "gtest/gtest.h" +#include "common/analysis/linter_test_utils.h" +#include "common/analysis/syntax_tree_linter_test_utils.h" +#include "common/text/symbol.h" +#include "verilog/CST/verilog_nonterminals.h" +#include "verilog/analysis/verilog_analyzer.h" + +namespace verilog { +namespace analysis { +namespace { + +using verible::LintTestCase; +using verible::RunLintTestCases; + +// Tests that ParameterNameStyleRule correctly accepts valid names. +TEST(PositiveMeaningParameterNameRuleTest, AcceptTests) { + const std::initializer_list kTestCases = { + {""}, + /* various parameter declarations */ + {"module foo; endmodule"}, + {"module foo (input bar); endmodule"}, + {"module foo; localparam type Bar_1 = 1; endmodule"}, + {"module foo; localparam Bar = 1; endmodule"}, + {"module foo; localparam int Bar = 1; endmodule"}, + {"module foo; parameter int HelloWorld = 1; endmodule"}, + {"module foo #(parameter int HelloWorld_1 = 1); endmodule"}, + {"module foo #(parameter type Foo); endmodule"}, + {"module foo #(int Foo = 8); endmodule"}, + {"module foo; localparam int Bar = 1; localparam int BarSecond = 2; " + "endmodule"}, + {"class foo; localparam int Bar = 1; endclass"}, + {"class foo #(parameter int Bar = 1); endclass"}, + {"package foo; parameter Bar = 1; endpackage"}, + {"package foo; parameter int HELLO_WORLD = 1; endpackage"}, + {"package foo; parameter int Bar = 1; endpackage"}, + {"parameter int Foo = 1;"}, + {"parameter type FooBar;"}, + {"parameter Foo = 1;"}, + {"module foo; localparam type Bar_Hello_1 = 1; endmodule"}, + {"module foo #(parameter type Bar_1_Hello__); endmodule"}, + {"package foo; parameter type Hello_world; endpackage"}, + + /* parameters using the enable prefix */ + {"module foo; parameter int EnableHelloWorld = 1; endmodule"}, + {"module foo (input enable_bar); endmodule"}, + {"module foo; localparam EnAbLeBar = 1; endmodule"}, + + /* Parameter type name should not trigger this */ + {"module foo #(parameter type disable_foo); endmodule"}, + {"module foo #(parameter type DisableFoo); endmodule"}, + {"module foo #(parameter type diSaBle_Foo); endmodule"}, + }; + RunLintTestCases( + kTestCases); +} + +// Tests that ParameterNameStyleRule rejects invalid names. +TEST(PositiveMeaningParameterNameRuleTest, RejectTests) { + constexpr int kToken = SymbolIdentifier; + const std::initializer_list kTestCases = { + {"module foo; parameter int ", + {kToken, "disable_foo"}, + " = 1; endmodule"}, + {"module foo; parameter int ", {kToken, "DisableFoo"}, " = 1; endmodule"}, + {"module foo; parameter int ", + {kToken, "DiSaBle_Foo"}, + " = 1; endmodule"}, + + {"module foo; localparam ", {kToken, "disable_bar"}, " = 1; endmodule"}, + {"module foo; localparam ", {kToken, "DisableBar"}, " = 1; endmodule"}, + {"module foo; localparam ", {kToken, "dIsaBleBar"}, " = 1; endmodule"}, + + {"module foo #(parameter int ", + {kToken, "disable_hello"}, + " = 1); endmodule"}, + {"module foo #(parameter int ", + {kToken, "disableHello"}, + " = 1); endmodule"}, + {"module foo #(parameter int ", + {kToken, "DiSable_hello"}, + " = 1); endmodule"}, + {"module foo #(parameter int ", + {kToken, "disable_hello"}, + "); endmodule"}, + {"module foo #(parameter int ", {kToken, "disableHello"}, "); endmodule"}, + {"module foo #(parameter int ", + {kToken, "DiSable_hello"}, + "); endmodule"}, + + {"module foo #(parameter int enable_f = 1, ", + {kToken, "disable_s"}, + " = 1); endmodule"}, + + {"class foo #(parameter int ", + {kToken, "disable_opt"}, + " = 1); endclass"}, + {"class foo #(parameter int ", {kToken, "disableOpt"}, " = 1); endclass"}, + {"class foo #(parameter int ", + {kToken, "DiSABle_Opt"}, + " = 1); endclass"}, + {"class foo #(parameter int ", {kToken, "disable_opt"}, "); endclass"}, + {"class foo #(parameter int ", {kToken, "disableOpt"}, "); endclass"}, + {"class foo #(parameter int ", {kToken, "DiSABle_Opt"}, "); endclass"}, + + {"package foo; parameter int ", + {kToken, "disable_sth"}, + " = 1; endpackage"}, + + {"module foo #(parameter int ", + {kToken, "DiSable_hello"}, + "); " + " parameter int ", + {kToken, "disableAbC"}, + " = 1;" + "endmodule"}, + + {"module foo #(parameter int ", + {kToken, "DiSable_hello"}, + "); " + " parameter int enableABC = 0, ", + {kToken, "disableXYZ"}, + " = 1;" + "endmodule"}, + + {"module foo #(parameter int ", + {kToken, "DiSable_hello"}, + "); " + " parameter int ", + {kToken, "disableAbC"}, + " = 1, ", + {kToken, "disableXYZ"}, + " = 1;" + "endmodule"}, + }; + RunLintTestCases( + kTestCases); +} + +} // namespace +} // namespace analysis +} // namespace verilog diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index f5afff1cb..05a362a50 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -44,6 +44,7 @@ _linter_test_configs = [ ("parameter-type-name-style", "parameter_type_name_style", False), ("parameter-type-name-style", "localparam_type_name_style", False), ("plusarg-assignment", "plusarg_assignment", True), + ("positive-meaning-parameter-name", "positive_meaning_parameter_name", False), ("proper-parameter-declaration", "proper_parameter_declaration", False), ("proper-parameter-declaration", "proper_localparam_declaration", False), ("module-parameter", "instance_parameters", True), diff --git a/verilog/tools/lint/testdata/positive_meaning_parameter_name.sv b/verilog/tools/lint/testdata/positive_meaning_parameter_name.sv new file mode 100644 index 000000000..f2ed5cb64 --- /dev/null +++ b/verilog/tools/lint/testdata/positive_meaning_parameter_name.sv @@ -0,0 +1,2 @@ +// Forbid parameter names starting with 'disable', use 'enable' instead. +module positive_meaning_parameter_name #(parameter int DISABLE_FOO = 1); endmodule