From 82728e73f13ef14591b01bed78b4602a1f6e3c4d Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Thu, 13 Jun 2024 23:41:24 +1000 Subject: [PATCH] Adding function to parse and return a regex for lint rule configuration --- .github/bin/smoke-test.sh | 30 ++-- .github/workflows/verible-ci.yml | 43 +++++- common/analysis/BUILD | 3 - common/text/BUILD | 2 + common/text/config_utils.cc | 16 +++ common/text/config_utils.h | 6 + common/text/config_utils_test.cc | 30 ++++ verilog/analysis/checkers/BUILD | 5 +- .../checkers/case_missing_default_rule.cc | 3 +- .../proper_parameter_declaration_rule.cc | 76 ++++++++-- .../proper_parameter_declaration_rule.h | 7 + .../proper_parameter_declaration_rule_test.cc | 133 +++++++++++++++--- verilog/parser/verilog.y | 14 +- verilog/tools/lint/testdata/endif_comment.sv | 2 +- 14 files changed, 317 insertions(+), 53 deletions(-) diff --git a/.github/bin/smoke-test.sh b/.github/bin/smoke-test.sh index deb238915..9d32e57e8 100755 --- a/.github/bin/smoke-test.sh +++ b/.github/bin/smoke-test.sh @@ -99,7 +99,8 @@ readonly TEST_GIT_PROJECTS="https://github.com/lowRISC/ibex \ https://github.com/rsd-devel/rsd \ https://github.com/syntacore/scr1 \ https://github.com/olofk/serv \ - https://github.com/bespoke-silicon-group/basejump_stl" + https://github.com/bespoke-silicon-group/basejump_stl \ + https://github.com/gtaylormb/opl3_fpga" ## # Some of the files in the projects will have issues. @@ -131,22 +132,22 @@ declare -A ExpectedFailCount ExpectedFailCount[syntax:ibex]=14 ExpectedFailCount[lint:ibex]=14 -ExpectedFailCount[project:ibex]=199 -ExpectedFailCount[preprocessor:ibex]=373 +ExpectedFailCount[project:ibex]=211 +ExpectedFailCount[preprocessor:ibex]=385 ExpectedFailCount[syntax:opentitan]=36 ExpectedFailCount[lint:opentitan]=36 -ExpectedFailCount[project:opentitan]=825 -ExpectedFailCount[preprocessor:opentitan]=2337 +ExpectedFailCount[project:opentitan]=800 +ExpectedFailCount[preprocessor:opentitan]=2238 ExpectedFailCount[syntax:sv-tests]=77 ExpectedFailCount[lint:sv-tests]=76 ExpectedFailCount[project:sv-tests]=187 ExpectedFailCount[preprocessor:sv-tests]=139 -ExpectedFailCount[syntax:caliptra-rtl]=24 -ExpectedFailCount[lint:caliptra-rtl]=24 -ExpectedFailCount[project:caliptra-rtl]=333 +ExpectedFailCount[syntax:caliptra-rtl]=25 +ExpectedFailCount[lint:caliptra-rtl]=25 +ExpectedFailCount[project:caliptra-rtl]=335 ExpectedFailCount[preprocessor:caliptra-rtl]=777 ExpectedFailCount[syntax:Cores-VeeR-EH2]=2 @@ -154,10 +155,10 @@ ExpectedFailCount[lint:Cores-VeeR-EH2]=2 ExpectedFailCount[project:Cores-VeeR-EH2]=42 ExpectedFailCount[preprocessor:Cores-VeeR-EH2]=43 -ExpectedFailCount[syntax:cva6]=6 -ExpectedFailCount[lint:cva6]=6 -ExpectedFailCount[project:cva6]=92 -ExpectedFailCount[preprocessor:cva6]=129 +ExpectedFailCount[syntax:cva6]=7 +ExpectedFailCount[lint:cva6]=7 +ExpectedFailCount[project:cva6]=86 +ExpectedFailCount[preprocessor:cva6]=124 ExpectedFailCount[syntax:uvm]=1 ExpectedFailCount[lint:uvm]=1 @@ -212,6 +213,11 @@ ExpectedFailCount[project:basejump_stl]=581 ExpectedFailCount[formatter:basejump_stl]=1 ExpectedFailCount[preprocessor:basejump_stl]=615 +ExpectedFailCount[syntax:opl3_fpga]=2 +ExpectedFailCount[lint:opl3_fpga]=2 +ExpectedFailCount[project:opl3_fpga]=4 +ExpectedFailCount[preprocessor:opl3_fpga]=4 + # Ideally, we expect all tools to process all files with a zero exit code. # However, that is not always the case, so we document the current # state, so that we can see regressions. diff --git a/.github/workflows/verible-ci.yml b/.github/workflows/verible-ci.yml index 1a4eb48ad..fe22374de 100644 --- a/.github/workflows/verible-ci.yml +++ b/.github/workflows/verible-ci.yml @@ -124,6 +124,47 @@ jobs: name: "diag" path: "**/plot_*.svg" + RunBantBuildCleaneer: + # Running http://bant.build/ to check all dependencies in BUILD files. + runs-on: ubuntu-24.04 + steps: + - name: Checkout code + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Build Project genrules + run: | + # Fetch all dependency and run genrules for bant to see every file + # that makes it into the compile. + bazel fetch ... + bazel build \ + //common/analysis:command-file-lexer \ + //common/lsp:jcxxgen-testfile-gen \ + //common/lsp:lsp-protocol-gen \ + //common/util:version-header \ + //verilog/CST:verilog-nonterminals-foreach-gen \ + //verilog/parser:gen-verilog-token-enum \ + //verilog/parser:verilog-lex \ + //verilog/parser:verilog-parse-interface \ + //verilog/parser:verilog-y \ + //verilog/parser:verilog-y-final \ + //verilog/tools/kythe:verilog-extractor-indexing-fact-type-foreach-gen + + - name: Get Bant + run: | + # TODO: provide this as action where we simply say with version=... + VERSION="v0.1.3" + STATIC_VERSION="bant-${VERSION}-linux-static-x86_64" + wget "https://github.com/hzeller/bant/releases/download/${VERSION}/${STATIC_VERSION}.tar.gz" + tar xvzf "${STATIC_VERSION}.tar.gz" + mkdir -p bin + ln -sf ../"${STATIC_VERSION}/bin/bant" bin/ + bin/bant -V + + - name: Run bant build-cleaner + run: | + bin/bant dwyu ... Check: container: ubuntu:jammy @@ -435,7 +476,7 @@ jobs: apt -qq -y install npm nodejs WindowsBuild: - runs-on: windows-2022 + runs-on: windows-2019 steps: - uses: actions/checkout@v3 diff --git a/common/analysis/BUILD b/common/analysis/BUILD index 8525ccb6e..c6a7ff9be 100644 --- a/common/analysis/BUILD +++ b/common/analysis/BUILD @@ -199,7 +199,6 @@ cc_library( "//common/util:algorithm", "//common/util:logging", "@com_google_absl//absl/strings:string_view", - "@com_google_googletest//:gtest", # for library testonly ], ) @@ -266,7 +265,6 @@ cc_library( "//common/text:text-structure", "//common/util:logging", "@com_google_absl//absl/strings:string_view", - "@com_google_googletest//:gtest", # for library testonly ], ) @@ -322,7 +320,6 @@ cc_library( "//common/text:text-structure", "//common/util:logging", "@com_google_absl//absl/strings:string_view", - "@com_google_googletest//:gtest", # for library testonly ], ) diff --git a/common/text/BUILD b/common/text/BUILD index 516e46ab2..151be07a5 100644 --- a/common/text/BUILD +++ b/common/text/BUILD @@ -132,6 +132,7 @@ cc_library( "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", + "@com_googlesource_code_re2//:re2", ], ) @@ -143,6 +144,7 @@ cc_test( "@com_google_absl//absl/status", "@com_google_absl//absl/strings", "@com_google_absl//absl/strings:string_view", + "@com_googlesource_code_re2//:re2", "@com_google_googletest//:gtest", "@com_google_googletest//:gtest_main", ], diff --git a/common/text/config_utils.cc b/common/text/config_utils.cc index eaec129a3..6df74bf67 100644 --- a/common/text/config_utils.cc +++ b/common/text/config_utils.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,7 @@ #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" #include "common/util/logging.h" +#include "re2/re2.h" namespace verible { using absl::string_view; @@ -184,5 +186,19 @@ ConfigValueSetter SetNamedBits( }; } +ConfigValueSetter SetRegex(std::unique_ptr *regex) { + CHECK(regex) << "Must provide pointer to a RE2 to store."; + return [regex](string_view v) { + *regex = std::make_unique(v, re2::RE2::Quiet); + if((*regex)->ok()) { + return absl::OkStatus(); + } + + std::string error_msg = + absl::StrCat("Failed to parse regular expression: ", (*regex)->error()); + return absl::InvalidArgumentError(error_msg); + }; +} + } // namespace config } // namespace verible diff --git a/common/text/config_utils.h b/common/text/config_utils.h index 82d59bef2..a18d4ec34 100644 --- a/common/text/config_utils.h +++ b/common/text/config_utils.h @@ -17,11 +17,13 @@ #include #include #include +#include #include #include #include "absl/status/status.h" #include "absl/strings/string_view.h" +#include "re2/re2.h" namespace verible { namespace config { @@ -94,6 +96,10 @@ ConfigValueSetter SetStringOneOf(std::string *value, // up to 32 choices. ConfigValueSetter SetNamedBits(uint32_t *value, const std::vector &choices); + +// Set a Regex +ConfigValueSetter SetRegex(std::unique_ptr *regex); + } // namespace config } // namespace verible diff --git a/common/text/config_utils_test.cc b/common/text/config_utils_test.cc index 319ebb9b1..106f6d011 100644 --- a/common/text/config_utils_test.cc +++ b/common/text/config_utils_test.cc @@ -21,8 +21,10 @@ #include "absl/status/status.h" #include "absl/strings/match.h" +#include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "gtest/gtest.h" +#include "re2/re2.h" namespace verible { namespace config { @@ -93,6 +95,19 @@ TEST(ConfigUtilsTest, ParseBool) { absl::StartsWith(s.message(), "baz: Boolean value should be one of")); } +TEST(ConfigUtilsTest, ParseRegex) { + absl::Status s; + std::unique_ptr regex; + s = ParseNameValues("regex:[a-b0-9_]", {{"regex", SetRegex(®ex)}}); + EXPECT_TRUE(s.ok()); + EXPECT_EQ(regex->pattern(), "[a-b0-9_]"); + + s = ParseNameValues("regex:[a-b0-9_", {{"regex", SetRegex(®ex)}}); + EXPECT_FALSE(s.ok()); + EXPECT_EQ(s.message(), + "regex: Failed to parse regular expression: missing ]: [a-b0-9_"); +} + TEST(ConfigUtilsTest, ParseString) { absl::Status s; std::string str; @@ -170,6 +185,21 @@ TEST(ConfigUtilsTest, ParseMultipleParameters) { EXPECT_TRUE(s.ok()) << s.message(); EXPECT_TRUE(panic); EXPECT_EQ(answer, 43); + + std::string str1; + std::string str2; + s = ParseNameValues("baz:hello world;fry:multiple spaces in this one", + {{"baz", SetString(&str1)}, {"fry", SetString(&str2)}}); + EXPECT_TRUE(s.ok()); + EXPECT_EQ(str1, "hello world"); + EXPECT_EQ(str2, "multiple spaces in this one"); + + std::unique_ptr regex; + s = ParseNameValues("baz:some text string;regex:[A-B0-9_]", + {{"baz", SetString(&str1)}, {"regex", SetRegex(®ex)}}); + EXPECT_TRUE(s.ok()); + EXPECT_EQ(str1, "some text string"); + EXPECT_EQ(regex->pattern(), "[A-B0-9_]"); } } // namespace config } // namespace verible diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index f00ee7efe..b032b4bd4 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1457,10 +1457,11 @@ cc_library( "//common/analysis:syntax-tree-lint-rule", "//common/analysis/matcher", "//common/analysis/matcher:bound-symbol-manager", - "//common/analysis/matcher:core-matchers", "//common/analysis/matcher:matcher-builders", + "//common/text:concrete-syntax-tree", "//common/text:symbol", "//common/text:syntax-tree-context", + "//common/text:tree-utils", "//verilog/CST:verilog-matchers", "//verilog/CST:verilog-nonterminals", "//verilog/analysis:descriptions", @@ -1744,6 +1745,7 @@ cc_library( "//common/analysis:syntax-tree-lint-rule", "//common/analysis/matcher", "//common/analysis/matcher:bound-symbol-manager", + "//common/text:config-utils", "//common/text:symbol", "//common/text:syntax-tree-context", "//verilog/CST:context-functions", @@ -1752,6 +1754,7 @@ cc_library( "//verilog/analysis:descriptions", "//verilog/analysis:lint-rule-registry", "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/status", "@com_google_absl//absl/strings:string_view", ], alwayslink = 1, diff --git a/verilog/analysis/checkers/case_missing_default_rule.cc b/verilog/analysis/checkers/case_missing_default_rule.cc index a3261d603..2132c3534 100644 --- a/verilog/analysis/checkers/case_missing_default_rule.cc +++ b/verilog/analysis/checkers/case_missing_default_rule.cc @@ -19,11 +19,12 @@ #include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" #include "common/analysis/matcher/bound_symbol_manager.h" -#include "common/analysis/matcher/core_matchers.h" #include "common/analysis/matcher/matcher.h" #include "common/analysis/matcher/matcher_builders.h" +#include "common/text/concrete_syntax_tree.h" #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" +#include "common/text/tree_utils.h" #include "verilog/CST/verilog_matchers.h" #include "verilog/CST/verilog_nonterminals.h" #include "verilog/analysis/descriptions.h" diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc index 73489f366..3d5f36bb9 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc @@ -16,10 +16,12 @@ #include +#include "absl/status/status.h" #include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" #include "common/analysis/matcher/bound_symbol_manager.h" #include "common/analysis/matcher/matcher.h" +#include "common/text/config_utils.h" #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" #include "verilog/CST/context_functions.h" @@ -40,12 +42,21 @@ using verible::matcher::Matcher; // Register ProperParameterDeclarationRule VERILOG_REGISTER_LINT_RULE(ProperParameterDeclarationRule); -static constexpr absl::string_view kParameterMessage = - "\'parameter\' declarations should only be within packages or in the " - "formal parameter list of modules/classes."; -static constexpr absl::string_view kLocalParamMessage = - "\'localparam\' declarations should only be within modules\' or classes\' " +static constexpr absl::string_view kParameterNotInPackageMessage = + "\'parameter\' declarations should only be in the formal parameter list of " + "modules/classes."; +static constexpr absl::string_view kParameterAllowPackageMessage = + "\'parameter\' declarations should only be in the formal parameter list of " + "modules and classes or in package definition bodies."; +static constexpr absl::string_view kLocalParamNotInPackageMessage = + "\'localparam\' declarations should only be within modules or class " "definition bodies."; +static constexpr absl::string_view kLocalParamAllowPackageMessage = + "\'localparam\' declarations should only be within modules, packages or " + "class definition bodies."; + +static absl::string_view kParameterMessage = kParameterNotInPackageMessage; +static absl::string_view kLocalParamMessage = kLocalParamAllowPackageMessage; const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { static const LintRuleDescriptor d{ @@ -53,12 +64,44 @@ const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { .topic = "constants", .desc = "Checks that every `parameter` declaration is inside a " - "package or in the formal parameter list of modules/classes and " - "every `localparam` declaration is inside a module or class.", - }; + "formal parameter list of modules/classes and " + "every `localparam` declaration is inside a module, class or " + "package.", + .param = { + { + .name = "package_allow_parameter", + .default_value = "false", + .description = "Allow parameters in packages (treated as a " + "synonym for localparam).", + }, + { + .name = "package_allow_localparam", + .default_value = "true", + .description = "Allow localparams in packages.", + }, + }}; return d; } +absl::Status ProperParameterDeclarationRule::Configure( + absl::string_view configuration) { + using verible::config::SetBool; + auto status = verible::ParseNameValues( + configuration, + { + {"package_allow_parameter", SetBool(&package_allow_parameter_)}, + {"package_allow_localparam", SetBool(&package_allow_localparam_)}, + }); + + // Change the message slightly + kParameterMessage = package_allow_parameter_ ? kParameterAllowPackageMessage + : kParameterNotInPackageMessage; + kLocalParamMessage = package_allow_localparam_ + ? kLocalParamAllowPackageMessage + : kLocalParamNotInPackageMessage; + return status; +} + static const Matcher &ParamDeclMatcher() { static const Matcher matcher(NodekParamDeclaration()); return matcher; @@ -79,11 +122,24 @@ void ProperParameterDeclarationRule::HandleSymbol( } else if (ContextIsInsideModule(context) && !ContextIsInsideFormalParameterList(context)) { violations_.insert(LintViolation(symbol, kParameterMessage, context)); + } else if (ContextIsInsidePackage(context)) { + if (!package_allow_parameter_) { + violations_.insert(LintViolation(symbol, kParameterMessage, context)); + } } } else if (param_decl_token == TK_localparam) { - // If the context is not inside a class or module, report violation. + // Raise violation if the context is not inside a class, package or + // module, report violation. if (!ContextIsInsideClass(context) && !ContextIsInsideModule(context)) { - violations_.insert(LintViolation(symbol, kLocalParamMessage, context)); + if (!ContextIsInsidePackage(context)) { + violations_.insert( + LintViolation(symbol, kLocalParamMessage, context)); + } else { + if (!package_allow_localparam_) { + violations_.insert( + LintViolation(symbol, kLocalParamMessage, context)); + } + } } } } diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.h b/verilog/analysis/checkers/proper_parameter_declaration_rule.h index a53e63b7c..410459154 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.h +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.h @@ -17,6 +17,8 @@ #include +#include "absl/status/status.h" +#include "absl/strings/string_view.h" #include "common/analysis/lint_rule_status.h" #include "common/analysis/syntax_tree_lint_rule.h" #include "common/text/symbol.h" @@ -40,8 +42,13 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule { verible::LintRuleStatus Report() const final; + absl::Status Configure(absl::string_view configuration) final; + private: std::set violations_; + + bool package_allow_parameter_ = false; + bool package_allow_localparam_ = true; }; } // namespace analysis diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc index fba4b3b1c..2ad3c69d4 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc @@ -27,6 +27,7 @@ namespace analysis { namespace { using verible::LintTestCase; +using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; // Tests that ProperParameterDeclarationRule does not report a violation when @@ -41,12 +42,18 @@ TEST(ProperParameterDeclarationRuleTest, BasicTests) { RunLintTestCases(kTestCases); } -// Tests that the expected number of parameter usage violations are found. -TEST(ProperParameterDeclarationRuleTest, ParameterTests) { +// Tests rejection of package parameters and allow package localparams +TEST(ProperParameterDeclarationRuleTest, RejectPackageParameters) { const std::initializer_list kTestCases = { {"parameter int Foo = 1;"}, - {"package foo; parameter int Bar = 1; endpackage"}, - {"package foo; parameter int Bar = 1; parameter int Bar2 = 2; " + {"package foo; ", + {TK_parameter, "parameter"}, + " int Bar = 1; endpackage"}, + {"package foo; ", + {TK_parameter, "parameter"}, + " int Bar = 1; ", + {TK_parameter, "parameter"}, + " int Bar2 = 2; " "endpackage"}, {"module foo #(parameter int Bar = 1); endmodule"}, {"module foo #(int Bar = 1); endmodule"}, @@ -54,7 +61,9 @@ TEST(ProperParameterDeclarationRuleTest, ParameterTests) { {"module foo #(parameter type Foo); endmodule"}, {"module foo; ", {TK_parameter, "parameter"}, " int Bar = 1; endmodule"}, {"class foo; ", {TK_parameter, "parameter"}, " int Bar = 1; endclass"}, - {"package foo; class bar; endclass parameter int HelloWorld = 1; " + {"package foo; class bar; endclass ", + {TK_parameter, "parameter"}, + " int HelloWorld = 1; " "endpackage"}, {"package foo; class bar; ", {TK_parameter, "parameter"}, @@ -69,7 +78,17 @@ TEST(ProperParameterDeclarationRuleTest, ParameterTests) { {"module foo #(parameter type Bar); ", {TK_parameter, "parameter"}, " type Bar2; endmodule"}, + {"module foo #(parameter type Bar);" + "module innerFoo #(" + "parameter type innerBar,", + "localparam int j = 2)();", + {TK_parameter, "parameter"}, + " int i = 1;" + "localparam int j = 2;" + "endmodule " + "endmodule"}, }; + RunLintTestCases(kTestCases); } @@ -83,18 +102,17 @@ TEST(ProperParameterDeclarationRuleTest, LocalParamTests) { {"module foo #(localparam int Bar = 1); endmodule"}, {"module foo #(localparam type Bar); endmodule"}, {"class foo #(localparam int Bar = 1); endclass"}, - {{TK_localparam, "localparam"}, " int Bar = 1;"}, - {"package foo; ", - {TK_localparam, "localparam"}, - " int Bar = 1; endpackage"}, - {"package foo; class bar; endclass ", - {TK_localparam, "localparam"}, - " int HelloWorld = 1; " + {{TK_localparam, "localparam"}, + " int Bar = 1;"}, // localparam defined outside a module or package + {"package foo; localparam int Bar = 1; endpackage"}, + {"package foo; class bar; endclass localparam int HelloWorld = 1; " "endpackage"}, {"package foo; class bar; localparam int HelloWorld = 1; endclass " "endpackage"}, }; - RunLintTestCases(kTestCases); + RunConfiguredLintTestCases( + kTestCases, + "package_allow_parameter:false;package_allow_localparam:true"); } // Tests that the expected number of localparam and parameter usage violations @@ -104,9 +122,10 @@ TEST(ProperParameterDeclarationRuleTest, CombinationParametersTest) { {"parameter int Foo = 1; ", {TK_localparam, "localparam"}, " int Bar = 1;"}, - {"package foo; parameter int Bar = 1; ", - {TK_localparam, "localparam"}, - " int Bar2 = 2; " + {"package foo; ", + {TK_parameter, "parameter"}, + " int Bar = 1; ", + "localparam int Bar2 = 2; " "endpackage"}, {"module foo #(parameter int Bar = 1); localparam int Bar2 = 2; " "endmodule"}, @@ -121,18 +140,88 @@ TEST(ProperParameterDeclarationRuleTest, CombinationParametersTest) { {"class foo; ", {TK_parameter, "parameter"}, " int Bar = 1; localparam int Bar2 = 2; endclass"}, - {"package foo; class bar; localparam int Bar2 = 2; endclass parameter " - "int HelloWorld = 1; " + {"package foo; class bar; localparam int Bar2 = 2; endclass ", + {TK_parameter, "parameter"}, + " int HelloWorld = 1; " "endpackage"}, {"package foo; ", - {TK_localparam, "localparam"}, - " int Bar2 = 2; class bar; endclass parameter " - "int HelloWorld = 1; " + "localparam", + " int Bar2 = 2; class bar; endclass ", + {TK_parameter, "parameter"}, + " int HelloWorld = 1; " "endpackage"}, {"parameter int Foo = 1; module bar; localparam int Bar2 = 2; " "endmodule"}, }; - RunLintTestCases(kTestCases); + RunConfiguredLintTestCases( + kTestCases, + "package_allow_parameter:false;package_allow_localparam:true"); +} + +// package parameters allowed, package localparam's are rejected +TEST(ProperParameterDeclarationRuleTest, AllowPackageParameters) { + const std::initializer_list kTestCases = { + {"parameter int Foo = 1;"}, + {"package foo; parameter int Bar = 1; endpackage"}, + {"package foo; ", + {TK_localparam, "localparam"}, + " int Bar = 1; endpackage"}, + {"package foo; parameter int Bar = 1; " + "parameter int Bar2 = 2; " + "endpackage"}, + {"module foo #(parameter int Bar = 1); endmodule"}, + {"module foo #(int Bar = 1); endmodule"}, + {"class foo #(parameter int Bar = 1); endclass"}, + {"module foo #(parameter type Foo); endmodule"}, + {"module foo; ", {TK_parameter, "parameter"}, " int Bar = 1; endmodule"}, + {"class foo; ", {TK_parameter, "parameter"}, " int Bar = 1; endclass"}, + {"package foo; class bar; endclass ", + "parameter int HelloWorld = 1; " + "endpackage"}, + {"package foo; class bar; ", + {TK_parameter, "parameter"}, + " int HelloWorld = 1; endclass " + "endpackage"}, + {"module foo #(parameter int Bar = 1); ", + {TK_parameter, "parameter"}, + " int HelloWorld = 1; " + "endmodule"}, + {"module foo #(parameter type Foo, parameter int Bar = 1); " + "endmodule"}, + {"module foo #(parameter type Bar); ", + {TK_parameter, "parameter"}, + " type Bar2; endmodule"}, + {"module foo #(parameter type Bar);" + "module innerFoo #(" + "parameter type innerBar,", + "localparam int j = 2)();", + {TK_parameter, "parameter"}, + " int i = 1;" + "localparam int j = 2;" + "endmodule " + "endmodule"}, + {"module foo; localparam int Bar = 1; endmodule"}, + {"class foo; localparam int Bar = 1; endclass"}, + {"module foo; localparam int Bar = 1; localparam int Bar2 = 2; " + "endmodule"}, + {"module foo #(localparam int Bar = 1); endmodule"}, + {"module foo #(localparam type Bar); endmodule"}, + {"class foo #(localparam int Bar = 1); endclass"}, + {{TK_localparam, "localparam"}, " int Bar = 1;"}, + {"package foo; ", + {TK_localparam, "localparam"}, + " int Bar = 1; endpackage"}, + {"package foo; class bar; endclass ", + {TK_localparam, "localparam"}, + " int HelloWorld = 1; " + "endpackage"}, + {"package foo; class bar; localparam int HelloWorld = 1; endclass " + "endpackage"}, + }; + + RunConfiguredLintTestCases( + kTestCases, + "package_allow_parameter:true;package_allow_localparam:false"); } } // namespace diff --git a/verilog/parser/verilog.y b/verilog/parser/verilog.y index 24d1d37ad..afb771626 100644 --- a/verilog/parser/verilog.y +++ b/verilog/parser/verilog.y @@ -3516,8 +3516,18 @@ instantiation_type instantiation_base : instantiation_type non_anonymous_gate_instance_or_register_variable_list { $$ = MakeInstantiationBase($1, $2); } - | reference call_base ',' gate_instance_or_register_variable_list - {$$ = MakeInstantiationBase(ReinterpretReferenceAsDataTypePackedDimensions($1), ExtendNode($4,$3,$2)); } + /* + * TODO: support mixed anonymous declarations + * + * This production rule was commented out because it caused + * verible-verilog-syntax to crash for some inputs. It may be necessary to + * re-enable it in the future to support declarations that mix anonymous and + * named instances. + * + * For more details, see https://github.com/chipsalliance/verible/issues/2181 + */ + // | reference call_base ',' gate_instance_or_register_variable_list + // {$$ = MakeInstantiationBase(ReinterpretReferenceAsDataTypePackedDimensions($1), ExtendNode($4,$3,$2)); } | reference_or_call_base {$$ = MakeTaggedNode(N::kFunctionCall,$1); } ; diff --git a/verilog/tools/lint/testdata/endif_comment.sv b/verilog/tools/lint/testdata/endif_comment.sv index 50d23bcd8..f125e1cba 100644 --- a/verilog/tools/lint/testdata/endif_comment.sv +++ b/verilog/tools/lint/testdata/endif_comment.sv @@ -1,5 +1,5 @@ package endif_comment; `ifdef FOOBAR - parameter int P = 4; + localparam int P = 4; `endif endpackage