From cb8ed4f1802a7de9ca3c418b7d2f386e90b2edea Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Tue, 30 Apr 2024 21:39:37 +1000 Subject: [PATCH 1/6] The default behaviour for packages has changed to be more in line with the LRM, where a violation is riased if the parameter keyword is used within a package. Configuration has been added to the rule to allow users to revert back to the original behaviour. --- bazel/proto-add-module-bazel.patch | 519 ------------------ common/analysis/line_lint_rule.h | 2 +- common/analysis/syntax_tree_lint_rule.h | 2 +- common/analysis/text_structure_lint_rule.h | 2 +- common/analysis/token_stream_lint_rule.h | 2 +- common/analysis/violation_handler.h | 4 +- common/formatting/tree_unwrapper.h | 2 +- common/lexer/flex_lexer_adapter.h | 4 +- common/lexer/token_stream_adapter_test.cc | 4 +- common/text/text_structure_test.cc | 2 +- common/text/token_info.h | 2 - common/text/tree_context_visitor.h | 4 +- verilog/analysis/checkers/BUILD | 1 + .../proper_parameter_declaration_rule.cc | 74 ++- .../proper_parameter_declaration_rule.h | 5 + .../proper_parameter_declaration_rule_test.cc | 114 +++- .../analysis/verilog_linter_configuration.h | 3 - .../tools/ls/verilog-language-server_test.cc | 8 +- verilog/tools/ls/vscode/src/download-ls.ts | 35 +- verilog/tools/ls/vscode/src/extension.ts | 3 + 20 files changed, 215 insertions(+), 577 deletions(-) delete mode 100644 bazel/proto-add-module-bazel.patch diff --git a/bazel/proto-add-module-bazel.patch b/bazel/proto-add-module-bazel.patch deleted file mode 100644 index f1542c639..000000000 --- a/bazel/proto-add-module-bazel.patch +++ /dev/null @@ -1,519 +0,0 @@ -diff --git a/BUILD.bazel b/BUILD.bazel -index 23efee4..1c7ff6e 100644 ---- a/BUILD.bazel -+++ b/BUILD.bazel -@@ -5,7 +5,7 @@ load("@rules_java//java:defs.bzl", "java_lite_proto_library", "java_proto_librar - load("@rules_pkg//:mappings.bzl", "pkg_files", "strip_prefix") - load("@rules_proto//proto:defs.bzl", "proto_lang_toolchain", "proto_library") - load("//build_defs:cpp_opts.bzl", "COPTS", "LINK_OPTS") --load(":protobuf.bzl", "internal_objc_proto_library", "internal_php_proto_library", "internal_py_proto_library", "internal_ruby_proto_library") -+load(":protobuf.bzl", "internal_objc_proto_library", "internal_php_proto_library", "internal_py_proto_library") - - licenses(["notice"]) - -@@ -150,17 +150,6 @@ filegroup( - visibility = ["//visibility:public"], - ) - --internal_ruby_proto_library( -- name = "well_known_ruby_protos", -- srcs = [":well_known_protos"], -- default_runtime = "", -- includes = ["src"], -- visibility = [ -- "//conformance:__pkg__", -- "//ruby:__subpackages__", -- ], --) -- - ################################################################################ - # Protocol Buffers Compiler - ################################################################################ -@@ -525,33 +514,6 @@ internal_php_proto_library( - ], - ) - --internal_ruby_proto_library( -- name = "test_messages_proto2_ruby_proto", -- testonly = 1, -- srcs = ["//src/google/protobuf:test_messages_proto2.proto"], -- includes = ["src/google/protobuf"], -- visibility = [ -- "//conformance:__pkg__", -- "//ruby:__subpackages__", -- ], --) -- --internal_ruby_proto_library( -- name = "test_messages_proto3_ruby_proto", -- testonly = 1, -- srcs = ["//src/google/protobuf:test_messages_proto3.proto"], -- includes = [ -- "src/google/protobuf", -- # The above must come first. -- "src", -- ], -- visibility = [ -- "//conformance:__pkg__", -- "//ruby:__subpackages__", -- ], -- deps = [":well_known_ruby_protos"], --) -- - filegroup( - name = "bzl_srcs", - srcs = glob(["**/*.bzl"]), -diff --git a/MODULE.bazel b/MODULE.bazel -index 2d43e46..2185fc4 100644 ---- a/MODULE.bazel -+++ b/MODULE.bazel -@@ -1,2 +1,49 @@ --# TODO: migrate all dependencies from WORKSPACE to MODULE.bazel --# https://github.com/protocolbuffers/protobuf/issues/14313 -+module( -+ name = "protobuf", -+ compatibility_level = 1, -+ version = "25.2", -+) -+ -+bazel_dep(name = "bazel_skylib", version = "1.0.3") -+bazel_dep(name = "rules_python", version = "0.10.2") -+bazel_dep(name = "rules_cc", version = "0.0.1") -+bazel_dep(name = "rules_proto", version = "4.0.0") -+bazel_dep(name = "rules_java", version = "4.0.0") -+bazel_dep(name = "rules_pkg", version = "0.7.0") -+bazel_dep(name = "platforms", version = "0.0.8") -+bazel_dep(name = "abseil-cpp", repo_name = "com_google_absl", version = "20240116.1") -+bazel_dep(name = "zlib", version = "1.2.11") -+bazel_dep(name = "upb", version = "0.0.0-20230516-61a97ef") -+bazel_dep(name = "rules_ruby", version = "0.6.0") -+ -+# Do not take the effort to convert utf8_range to Bzlmod as this has been moved to protobuf/third_party -+# See https://github.com/protocolbuffers/utf8_range/commit/1d1ea7e3fedf482d4a12b473c1ed25fe0f371a45 -+non_module_deps = use_extension("//:non_module_deps.bzl", "non_module_deps") -+use_repo(non_module_deps, "utf8_range") -+ -+# TODO: Add missing rules_kotlin -+ -+# Maven dependencies -+bazel_dep(name = "rules_jvm_external", version = "4.4.2") -+ -+maven = use_extension("@rules_jvm_external//:extensions.bzl", "maven") -+ -+maven.install( -+ name = "maven", -+ artifacts = [ -+ "com.google.code.findbugs:jsr305:3.0.2", -+ "com.google.code.gson:gson:2.8.9", -+ "com.google.errorprone:error_prone_annotations:2.3.2", -+ "com.google.j2objc:j2objc-annotations:1.3", -+ "com.google.guava:guava:31.1-jre", -+ "com.google.guava:guava-testlib:31.1-jre", -+ "com.google.truth:truth:1.1.2", -+ "junit:junit:4.13.2", -+ "org.mockito:mockito-core:4.3.1", -+ ], -+) -+ -+use_repo(maven, "maven") -+ -+# Dependencies needed in tests -+bazel_dep(name = "googletest", version = "1.14.0.bcr.1", repo_name="com_google_googletest") -diff --git a/conformance/BUILD.bazel b/conformance/BUILD.bazel -index c495504..26f37d4 100644 ---- a/conformance/BUILD.bazel -+++ b/conformance/BUILD.bazel -@@ -2,7 +2,8 @@ - - load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library", "cc_proto_library", "objc_library") - load("@rules_ruby//ruby:defs.bzl", "ruby_binary") --load("//:protobuf.bzl", "internal_csharp_proto_library", "internal_objc_proto_library", "internal_php_proto_library", "internal_py_proto_library", "internal_ruby_proto_library") -+load("//ruby:defs.bzl", "internal_ruby_proto_library") -+load("//:protobuf.bzl", "internal_csharp_proto_library", "internal_objc_proto_library", "internal_php_proto_library", "internal_py_proto_library") - load("//build_defs:internal_shell.bzl", "inline_sh_binary") - load( - "@rules_pkg//:mappings.bzl", -@@ -331,8 +332,7 @@ ruby_binary( - visibility = ["//ruby:__subpackages__"], - deps = [ - ":conformance_ruby_proto", -- "//:test_messages_proto2_ruby_proto", -- "//:test_messages_proto3_ruby_proto", -+ "//ruby:conformance_test_ruby_proto", - ], - ) - -diff --git a/examples/MODULE.bazel b/examples/MODULE.bazel -new file mode 100644 -index 0000000..7e7f44f ---- /dev/null -+++ b/examples/MODULE.bazel -@@ -0,0 +1,10 @@ -+bazel_dep(name = "rules_cc", version = "0.0.1") -+bazel_dep(name = "rules_proto", version = "4.0.0") -+bazel_dep(name = "rules_java", version = "4.0.0") -+bazel_dep(name = "rules_pkg", version = "0.7.0") -+bazel_dep(name = "protobuf", repo_name = "com_google_protobuf") -+ -+local_path_override( -+ module_name = "protobuf", -+ path = "..", -+) -diff --git a/non_module_deps.bzl b/non_module_deps.bzl -new file mode 100644 -index 0000000..5cc13d7 ---- /dev/null -+++ b/non_module_deps.bzl -@@ -0,0 +1,19 @@ -+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive") -+ -+def _github_archive(repo, commit, **kwargs): -+ repo_name = repo.split("/")[-1] -+ http_archive( -+ urls = [repo + "/archive/" + commit + ".zip"], -+ strip_prefix = repo_name + "-" + commit, -+ **kwargs -+ ) -+ -+def _non_module_deps_impl(ctx): -+ _github_archive( -+ name = "utf8_range", -+ repo = "https://github.com/protocolbuffers/utf8_range", -+ commit = "de0b4a8ff9b5d4c98108bdfe723291a33c52c54f", -+ sha256 = "5da960e5e5d92394c809629a03af3c7709d2d3d0ca731dacb3a9fb4bf28f7702", -+ ) -+ -+non_module_deps = module_extension(implementation = _non_module_deps_impl) -diff --git a/p.patch b/p.patch -new file mode 100644 -index 0000000..869fbff ---- /dev/null -+++ b/p.patch -@@ -0,0 +1,69 @@ -+From 2b8f46ed079f54cecfd29115d3be0296da2738a4 Mon Sep 17 00:00:00 2001 -+From: Benjamin Peterson -+Date: Mon, 31 Jul 2023 08:04:21 -0700 -+Subject: [PATCH 7/7] bazel: Get rid of exec_tools. (#13401) -+ -+Bazel has removed this attribute in bazelbuild/bazel@c061e57a7004a88eeb2f84d094d9a88b56c146b6. -+ -+Closes #13401 -+ -+COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/13401 from benjaminp:exec_tools 9e718ff0fd11ff1fe70ed3e2536373792504c9c5 -+PiperOrigin-RevId: 552482730 -+ -+Backport of commit 1bee4578b8a812faed90432798222670f6df2a9b -+--- -+ build_defs/internal_shell.bzl | 4 ++-- -+ objectivec/BUILD.bazel | 2 +- -+ src/google/protobuf/BUILD.bazel | 2 +- -+ 3 files changed, 4 insertions(+), 4 deletions(-) -+ -+diff --git a/build_defs/internal_shell.bzl b/build_defs/internal_shell.bzl -+index 649184a51..91628a5e5 100644 -+--- a/build_defs/internal_shell.bzl -++++ b/build_defs/internal_shell.bzl -+@@ -32,7 +32,7 @@ def inline_sh_binary( -+ native.genrule( -+ name = name + "_genrule", -+ srcs = srcs, -+- exec_tools = tools, -++ tools = tools, -+ outs = [name + ".sh"], -+ cmd = "cat <<'EOF' >$(OUTS)\n#!/bin/bash -exu\n%s\nEOF\n" % cmd, -+ visibility = ["//visibility:private"], -+@@ -77,7 +77,7 @@ def inline_sh_test( -+ native.genrule( -+ name = name + "_genrule", -+ srcs = srcs, -+- exec_tools = tools, -++ tools = tools, -+ outs = [name + ".sh"], -+ cmd = "cat <<'EOF' >$(OUTS)\n#!/bin/bash -exu\n%s\nEOF\n" % cmd, -+ visibility = ["//visibility:private"], -+diff --git a/objectivec/BUILD.bazel b/objectivec/BUILD.bazel -+index 107983806..c59bf0e60 100644 -+--- a/objectivec/BUILD.bazel -++++ b/objectivec/BUILD.bazel -+@@ -42,7 +42,7 @@ genrule( -+ for wkt in _OBJC_WKT_NAMES -+ for ext in _OBJC_EXTS -+ ]), -+- exec_tools = ["//:protoc"], -++ tools = ["//:protoc"], -+ tags = ["manual"], -+ ) -+ -+diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel -+index f7f1c7b48..735610170 100644 -+--- a/src/google/protobuf/BUILD.bazel -++++ b/src/google/protobuf/BUILD.bazel -+@@ -138,7 +138,7 @@ genrule( -+ --proto_path=$$(dirname $$(dirname $$(dirname $(location any.proto)))) \ -+ $(SRCS) -+ """, -+- exec_tools = ["//:protoc"], -++ tools = ["//:protoc"], -+ visibility = ["//visibility:private"], -+ ) -+ -+-- -+2.42.1 -diff --git a/protobuf.bzl b/protobuf.bzl -index d96eeb4..3f7e001 100644 ---- a/protobuf.bzl -+++ b/protobuf.bzl -@@ -2,7 +2,6 @@ load("@bazel_skylib//lib:versions.bzl", "versions") - load("@rules_cc//cc:defs.bzl", "objc_library") - load("@rules_proto//proto:defs.bzl", "ProtoInfo") - load("@rules_python//python:defs.bzl", "py_library") --load("@rules_ruby//ruby:defs.bzl", "ruby_library") - - def _GetPath(ctx, path): - if ctx.label.workspace_root: -@@ -490,11 +489,12 @@ def internal_objc_proto_library( - - def internal_ruby_proto_library( - name, -+ ruby_library, - srcs = [], - deps = [], - includes = ["."], -- default_runtime = "@com_google_protobuf//ruby:protobuf", -- protoc = "@com_google_protobuf//:protoc", -+ default_runtime = Label("//ruby:protobuf"), -+ protoc = Label("//:protoc"), - testonly = None, - visibility = ["//visibility:public"], - **kwargs): -@@ -506,6 +506,7 @@ def internal_ruby_proto_library( - - Args: - name: the name of the ruby_proto_library. -+ ruby_library: the ruby library rules to use. - srcs: the .proto files to compile. - deps: a list of dependency labels; must be a internal_ruby_proto_library. - includes: a string indicating the include path of the .proto files. -diff --git a/protobuf_deps.bzl b/protobuf_deps.bzl -index d055eed..c960909 100644 ---- a/protobuf_deps.bzl -+++ b/protobuf_deps.bzl -@@ -109,14 +109,6 @@ def protobuf_deps(): - sha256 = "f9e4f6acf82449324d56669bda4bdb28b48688ad2990d8b39fa5b93ed39c9ad1", - ) - -- if not native.existing_rule("rules_ruby"): -- _github_archive( -- name = "rules_ruby", -- repo = "https://github.com/protocolbuffers/rules_ruby", -- commit = "b7f3e9756f3c45527be27bc38840d5a1ba690436", -- sha256 = "347927fd8de6132099fcdc58e8f7eab7bde4eb2fd424546b9cd4f1c6f8f8bad8", -- ) -- - if not native.existing_rule("rules_jvm_external"): - _github_archive( - name = "rules_jvm_external", -diff --git a/ruby/BUILD.bazel b/ruby/BUILD.bazel -index cc4b477..ed523c8 100755 ---- a/ruby/BUILD.bazel -+++ b/ruby/BUILD.bazel -@@ -7,7 +7,8 @@ load("@bazel_skylib//rules:common_settings.bzl", "string_flag") - load("@rules_pkg//:mappings.bzl", "pkg_files", "strip_prefix") - load("@rules_ruby//ruby:defs.bzl", "ruby_library") - load("//build_defs:internal_shell.bzl", "inline_sh_binary") --load("//:protobuf.bzl", "internal_ruby_proto_library") -+load("//python:internal.bzl", "internal_copy_files") -+load("//ruby:defs.bzl", "internal_ruby_proto_library") - load("//conformance:defs.bzl", "conformance_test") - load("//:protobuf_version.bzl", "PROTOBUF_RUBY_VERSION") - -@@ -15,6 +16,49 @@ load("//:protobuf_version.bzl", "PROTOBUF_RUBY_VERSION") - # Ruby Runtime - ################################################################################ - -+internal_copy_files( -+ name = "copied_wkt_proto_files", -+ srcs = [ -+ "//:well_known_type_protos", -+ "//src/google/protobuf:descriptor_proto_srcs", -+ "//src/google/protobuf/compiler:plugin.proto", -+ ], -+ strip_prefix = "src", -+) -+ -+internal_ruby_proto_library( -+ name = "well_known_ruby_protos", -+ srcs = [":copied_wkt_proto_files"], -+ default_runtime = "", -+ includes = ["."], -+ visibility = [ -+ "//conformance:__pkg__", -+ "//ruby:__subpackages__", -+ ], -+) -+ -+internal_copy_files( -+ name = "copied_conformance_test_files", -+ testonly = 1, -+ srcs = [ -+ "//src/google/protobuf:test_messages_proto2.proto", -+ "//src/google/protobuf:test_messages_proto3.proto", -+ ], -+ strip_prefix = "src", -+) -+ -+internal_ruby_proto_library( -+ name = "conformance_test_ruby_proto", -+ testonly = 1, -+ srcs = [":copied_conformance_test_files"], -+ includes = ["."], -+ visibility = [ -+ "//conformance:__pkg__", -+ "//ruby:__subpackages__", -+ ], -+ deps = [":well_known_ruby_protos"], -+) -+ - string_flag( - name = "ffi", - build_setting_default = "disabled", -@@ -105,7 +149,7 @@ genrule( - "//ruby/lib/google:copy_jar", - "//ruby/lib/google:dist_files", - "//ruby/ext/google/protobuf_c:dist_files", -- "//:well_known_ruby_protos", -+ ":well_known_ruby_protos", - "google-protobuf.gemspec", - ], - outs = ["google-protobuf-" + PROTOBUF_RUBY_VERSION + "-java.gem"], -@@ -119,7 +163,7 @@ genrule( - for utf in $(execpaths @utf8_range//:utf8_range_srcs) $(execpath @utf8_range//:LICENSE); do - mv "tmp/$$utf" "tmp/ruby/ext/google/protobuf_c/third_party/utf8_range" - done -- for wkt in $(execpaths //:well_known_ruby_protos); do -+ for wkt in $(execpaths :well_known_ruby_protos); do - mv "tmp/$$wkt" "tmp/ruby/lib/google/protobuf/" - done - mv "tmp/$(execpath //ruby/lib/google:copy_jar)" "tmp/ruby/lib/google" -@@ -141,7 +185,7 @@ genrule( - srcs = [ - "@utf8_range//:utf8_range_srcs", - "@utf8_range//:LICENSE", -- "//:well_known_ruby_protos", -+ ":well_known_ruby_protos", - "//ruby/ext/google/protobuf_c:dist_files", - "//ruby/lib/google:dist_files", - "google-protobuf.gemspec", -@@ -157,7 +201,7 @@ genrule( - for utf in $(execpaths @utf8_range//:utf8_range_srcs) $(execpath @utf8_range//:LICENSE); do - mv "tmp/$$utf" "tmp/ruby/ext/google/protobuf_c/third_party/utf8_range" - done -- for wkt in $(execpaths //:well_known_ruby_protos); do -+ for wkt in $(execpaths :well_known_ruby_protos); do - mv "tmp/$$wkt" "tmp/ruby/lib/google/protobuf/" - done - cd tmp/ruby -@@ -198,7 +242,7 @@ internal_ruby_proto_library( - visibility = [ - "//ruby:__subpackages__", - ], -- deps = ["//:well_known_ruby_protos"], -+ deps = [":well_known_ruby_protos"], - ) - - conformance_test( -diff --git a/ruby/compatibility_tests/v3.0.0/tests/BUILD.bazel b/ruby/compatibility_tests/v3.0.0/tests/BUILD.bazel -index 34a5391..697f876 100644 ---- a/ruby/compatibility_tests/v3.0.0/tests/BUILD.bazel -+++ b/ruby/compatibility_tests/v3.0.0/tests/BUILD.bazel -@@ -1,6 +1,6 @@ - load("@rules_pkg//:mappings.bzl", "pkg_files", "strip_prefix") - load("@rules_ruby//ruby:defs.bzl", "ruby_test") --load("//:protobuf.bzl", "internal_ruby_proto_library") -+load("//ruby:defs.bzl", "internal_ruby_proto_library") - - internal_ruby_proto_library( - name = "test_ruby_protos", -diff --git a/ruby/defs.bzl b/ruby/defs.bzl -new file mode 100644 -index 0000000..7f60b47 ---- /dev/null -+++ b/ruby/defs.bzl -@@ -0,0 +1,24 @@ -+"""Wrapper around internal_ruby_proto_library to supply our rules_ruby""" -+ -+load("@rules_ruby//ruby:defs.bzl", "ruby_library") -+load("//:protobuf.bzl", _internal_ruby_proto_library = "internal_ruby_proto_library") -+ -+def internal_ruby_proto_library( -+ name, -+ **kwargs): -+ """Bazel rule to create a Ruby protobuf library from proto source files -+ -+ NOTE: the rule is only an internal workaround to generate protos. The -+ interface may change and the rule may be removed when bazel has introduced -+ the native rule. -+ -+ Args: -+ name: the name of the ruby_proto_library. -+ **kwargs: other keyword arguments that are passed to ruby_library. -+ -+ """ -+ _internal_ruby_proto_library( -+ name, -+ ruby_library, -+ **kwargs -+ ) -diff --git a/ruby/lib/google/BUILD.bazel b/ruby/lib/google/BUILD.bazel -index 18f1c18..8af2fa8 100644 ---- a/ruby/lib/google/BUILD.bazel -+++ b/ruby/lib/google/BUILD.bazel -@@ -76,9 +76,12 @@ ruby_library( - "//ruby:linux_ffi_enabled": ["libprotobuf_c_ffi.so"], - "//conditions:default": [], - }), -- includes = ["ruby/lib"], -+ includes = [ -+ "ruby", -+ "ruby/lib", -+ ], - visibility = ["//ruby:__pkg__"], -- deps = ["//:well_known_ruby_protos"] + select({ -+ deps = ["//ruby:well_known_ruby_protos"] + select({ - "//ruby:ffi_enabled": [ - "@protobuf_bundle//:ffi", - "@protobuf_bundle//:ffi-compiler", -diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel -index 8961ca6..88e545c 100644 ---- a/src/google/protobuf/BUILD.bazel -+++ b/src/google/protobuf/BUILD.bazel -@@ -777,6 +777,7 @@ exports_files( - visibility = [ - "//:__pkg__", - "//python:__pkg__", -+ "//ruby:__pkg__", - ], - ) - -diff --git a/src/google/protobuf/io/coded_stream.cc b/src/google/protobuf/io/coded_stream.cc -index fdfc2d0..665f831 100644 ---- a/src/google/protobuf/io/coded_stream.cc -+++ b/src/google/protobuf/io/coded_stream.cc -@@ -522,7 +522,7 @@ int CodedInputStream::ReadVarintSizeAsIntFallback() { - // Optimization: We're also safe if the buffer is non-empty and it ends - // with a byte that would terminate a varint. - (buffer_end_ > buffer_ && !(buffer_end_[-1] & 0x80))) { -- uint64_t temp; -+ uint64_t temp = 0; - ::std::pair p = ReadVarint64FromArray(buffer_, &temp); - if (!p.first || temp > static_cast(INT_MAX)) return -1; - buffer_ = p.second; -@@ -632,7 +632,7 @@ std::pair CodedInputStream::ReadVarint64Fallback() { - // Optimization: We're also safe if the buffer is non-empty and it ends - // with a byte that would terminate a varint. - (buffer_end_ > buffer_ && !(buffer_end_[-1] & 0x80))) { -- uint64_t temp; -+ uint64_t temp = 0; - ::std::pair p = ReadVarint64FromArray(buffer_, &temp); - if (!p.first) { - return std::make_pair(0, false); diff --git a/common/analysis/line_lint_rule.h b/common/analysis/line_lint_rule.h index 9f4adf30e..ec6da5788 100644 --- a/common/analysis/line_lint_rule.h +++ b/common/analysis/line_lint_rule.h @@ -26,7 +26,7 @@ namespace verible { class LineLintRule : public LintRule { public: - ~LineLintRule() override = default; + ~LineLintRule() override = default; // not yet final // Scans a single line during analysis. virtual void HandleLine(absl::string_view line) = 0; diff --git a/common/analysis/syntax_tree_lint_rule.h b/common/analysis/syntax_tree_lint_rule.h index 184fcdcb3..90c96fed1 100644 --- a/common/analysis/syntax_tree_lint_rule.h +++ b/common/analysis/syntax_tree_lint_rule.h @@ -38,7 +38,7 @@ namespace verible { // top of the stack/back of vector. class SyntaxTreeLintRule : public LintRule { public: - ~SyntaxTreeLintRule() override = default; + ~SyntaxTreeLintRule() override = default; // not yet final virtual void HandleLeaf(const SyntaxTreeLeaf &leaf, const SyntaxTreeContext &context) {} diff --git a/common/analysis/text_structure_lint_rule.h b/common/analysis/text_structure_lint_rule.h index 0116257f4..eb44e6836 100644 --- a/common/analysis/text_structure_lint_rule.h +++ b/common/analysis/text_structure_lint_rule.h @@ -38,7 +38,7 @@ namespace verible { class TextStructureLintRule : public LintRule { public: - ~TextStructureLintRule() override = default; + ~TextStructureLintRule() override = default; // not yet final // Analyze text structure for violations. virtual void Lint(const TextStructureView &text_structure, diff --git a/common/analysis/token_stream_lint_rule.h b/common/analysis/token_stream_lint_rule.h index 381ee2dbd..dd1e978e2 100644 --- a/common/analysis/token_stream_lint_rule.h +++ b/common/analysis/token_stream_lint_rule.h @@ -26,7 +26,7 @@ namespace verible { class TokenStreamLintRule : public LintRule { public: - ~TokenStreamLintRule() override = default; + ~TokenStreamLintRule() override = default; // not yet final // Scans a single token during analysis. virtual void HandleToken(const TokenInfo &token) = 0; diff --git a/common/analysis/violation_handler.h b/common/analysis/violation_handler.h index 96994c3b7..8886b314a 100644 --- a/common/analysis/violation_handler.h +++ b/common/analysis/violation_handler.h @@ -51,7 +51,7 @@ class ViolationPrinter : public ViolationHandler { void HandleViolations( const std::set& violations, - absl::string_view base, absl::string_view path) override; + absl::string_view base, absl::string_view path) final; protected: std::ostream* const stream_; @@ -68,7 +68,7 @@ class ViolationWaiverPrinter : public ViolationHandler { void HandleViolations( const std::set& violations, - absl::string_view base, absl::string_view path) override; + absl::string_view base, absl::string_view path) final; protected: std::ostream* const message_stream_; diff --git a/common/formatting/tree_unwrapper.h b/common/formatting/tree_unwrapper.h index cd7ba1525..c3f723db5 100644 --- a/common/formatting/tree_unwrapper.h +++ b/common/formatting/tree_unwrapper.h @@ -52,7 +52,7 @@ class TreeUnwrapper : public TreeContextVisitor { TreeUnwrapper& operator=(const TreeUnwrapper&) = delete; TreeUnwrapper& operator=(TreeUnwrapper&&) = delete; - ~TreeUnwrapper() override = default; + ~TreeUnwrapper() override = default; // not yet final. // Partitions the token stream (in text_structure_view_) into // unwrapped_lines_ by traversing the syntax tree representation. diff --git a/common/lexer/flex_lexer_adapter.h b/common/lexer/flex_lexer_adapter.h index 7b0ab6899..57a6900dd 100644 --- a/common/lexer/flex_lexer_adapter.h +++ b/common/lexer/flex_lexer_adapter.h @@ -77,7 +77,7 @@ class FlexLexerAdapter : private CodeStreamHolder, protected L, public Lexer { const TokenInfo &GetLastToken() const final { return last_token_; } // Returns next token and updates its location. - const TokenInfo &DoNextToken() override { + const TokenInfo &DoNextToken() override { // not yet final if (at_eof_) { // Do not call yylex(), because that will result in the fatal error: // "fatal flex scanner internal error--end of buffer missed" @@ -108,7 +108,7 @@ class FlexLexerAdapter : private CodeStreamHolder, protected L, public Lexer { } // Restart lexer by pointing to new input stream, and reset all state. - void Restart(absl::string_view code) override { + void Restart(absl::string_view code) override { // not yet final at_eof_ = false; code_ = code; code_stream_.str(std::string(code_)); diff --git a/common/lexer/token_stream_adapter_test.cc b/common/lexer/token_stream_adapter_test.cc index fe2608270..1c9054154 100644 --- a/common/lexer/token_stream_adapter_test.cc +++ b/common/lexer/token_stream_adapter_test.cc @@ -38,7 +38,9 @@ class FakeTokenSequenceLexer : public Lexer, public FakeLexer { void Restart(absl::string_view) final {} - bool TokenIsError(const TokenInfo &) const override { return false; } + bool TokenIsError(const TokenInfo &) const override { // not yet final. + return false; + } }; TEST(MakeTokenGeneratorTest, Generate) { diff --git a/common/text/text_structure_test.cc b/common/text/text_structure_test.cc index 99d34849f..3d47784d0 100644 --- a/common/text/text_structure_test.cc +++ b/common/text/text_structure_test.cc @@ -363,7 +363,7 @@ class TextStructureViewInternalsTest : public TextStructureViewPublicTest { // modifications. This is only appropriate for tests on private or protected // methods; public methods should always leave the structure in a consistent // state. - ~TextStructureViewInternalsTest() override { Clear(); } + ~TextStructureViewInternalsTest() override { Clear(); } // not yet final }; // Test that whole tree is returned with offset 0. diff --git a/common/text/token_info.h b/common/text/token_info.h index f7e2ce9f8..ace8c5460 100644 --- a/common/text/token_info.h +++ b/common/text/token_info.h @@ -73,8 +73,6 @@ class TokenInfo { Context(absl::string_view b, std::function translator) : base(b), token_enum_translator(std::move(translator)) {} - - Context(const Context&) = default; }; int token_enum() const { return token_enum_; } diff --git a/common/text/tree_context_visitor.h b/common/text/tree_context_visitor.h index b89ceb971..9b712436d 100644 --- a/common/text/tree_context_visitor.h +++ b/common/text/tree_context_visitor.h @@ -30,8 +30,8 @@ class TreeContextVisitor : public SymbolVisitor { TreeContextVisitor() = default; protected: - void Visit(const SyntaxTreeLeaf &leaf) override {} - void Visit(const SyntaxTreeNode &node) override; + void Visit(const SyntaxTreeLeaf &leaf) override {} // not yet final + void Visit(const SyntaxTreeNode &node) override; // not yet final const SyntaxTreeContext &Context() const { return current_context_; } diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index dc1177397..32064f1cc 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1743,6 +1743,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", diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc index 73489f366..6961fc728 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc @@ -20,6 +20,7 @@ #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 +41,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 +63,43 @@ 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( + const absl::string_view configuration) { + using verible::config::SetBool; + return 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; +} + static const Matcher &ParamDeclMatcher() { static const Matcher matcher(NodekParamDeclaration()); return matcher; @@ -79,11 +120,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..ae49150e0 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.h +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.h @@ -40,8 +40,13 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule { verible::LintRuleStatus Report() const final; + absl::Status Configure(const absl::string_view configuration); + 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..530d80ce0 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 @@ -45,8 +46,14 @@ TEST(ProperParameterDeclarationRuleTest, BasicTests) { TEST(ProperParameterDeclarationRuleTest, ParameterTests) { 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,6 +78,15 @@ 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); } @@ -84,12 +102,8 @@ TEST(ProperParameterDeclarationRuleTest, LocalParamTests) { {"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; " + {"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"}, @@ -104,9 +118,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,13 +136,15 @@ 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"}, @@ -135,6 +152,69 @@ TEST(ProperParameterDeclarationRuleTest, CombinationParametersTest) { RunLintTestCases(kTestCases); } +TEST(ProperParameterDeclarationRuleTest, AllowPackageParameters) { + constexpr int kToken = SymbolIdentifier; + const std::initializer_list kAllowPackageParametersTestCases = { + {"parameter int Foo = 1;"}, + {"package foo; ", "parameter 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( + kAllowPackageParametersTestCases, + "package_allow_parameter:true;package_allow_localparam:false"); +} + } // namespace } // namespace analysis } // namespace verilog diff --git a/verilog/analysis/verilog_linter_configuration.h b/verilog/analysis/verilog_linter_configuration.h index a858320ab..7cf5df90f 100644 --- a/verilog/analysis/verilog_linter_configuration.h +++ b/verilog/analysis/verilog_linter_configuration.h @@ -188,9 +188,6 @@ class LinterConfiguration { // Constructor has no rules enabled by default; LinterConfiguration() = default; - // This is copy-able. - LinterConfiguration(const LinterConfiguration &) = default; - void TurnOn(const analysis::LintRuleId &rule) { configuration_[rule] = {true, ""}; } diff --git a/verilog/tools/ls/verilog-language-server_test.cc b/verilog/tools/ls/verilog-language-server_test.cc index 2de8165d1..3d9c24fd9 100644 --- a/verilog/tools/ls/verilog-language-server_test.cc +++ b/verilog/tools/ls/verilog-language-server_test.cc @@ -123,7 +123,7 @@ class VerilogLanguageServerTest : public ::testing::Test { // Sets up the testing environment - creates Language Server object and // sends textDocument/initialize request. // It stores the response in initialize_response field for further processing - void SetUp() override { + void SetUp() override { // not yet final server_ = std::make_unique( [this](absl::string_view response) { response_stream_ << response; }); @@ -148,7 +148,7 @@ class VerilogLanguageServerTest : public ::testing::Test { class VerilogLanguageServerSymbolTableTest : public VerilogLanguageServerTest { public: - absl::Status InitializeCommunication() override { + absl::Status InitializeCommunication() final { json initialize_request = { {"jsonrpc", "2.0"}, {"id", 1}, @@ -158,7 +158,7 @@ class VerilogLanguageServerSymbolTableTest : public VerilogLanguageServerTest { } protected: - void SetUp() override { + void SetUp() final { absl::SetFlag(&FLAGS_rules_config_search, true); root_dir = verible::file::JoinPath( ::testing::TempDir(), @@ -168,7 +168,7 @@ class VerilogLanguageServerSymbolTableTest : public VerilogLanguageServerTest { VerilogLanguageServerTest::SetUp(); } - void TearDown() override { std::filesystem::remove(root_dir); } + void TearDown() final { std::filesystem::remove(root_dir); } // path to the project std::string root_dir; diff --git a/verilog/tools/ls/vscode/src/download-ls.ts b/verilog/tools/ls/vscode/src/download-ls.ts index 9cf2982eb..574476b7f 100644 --- a/verilog/tools/ls/vscode/src/download-ls.ts +++ b/verilog/tools/ls/vscode/src/download-ls.ts @@ -1,5 +1,5 @@ import * as vscode from "vscode"; -import { platform, arch } from "os"; +import { homedir, platform, arch } from "os"; import * as fs from "fs"; import * as path from "path"; import { IncomingMessage } from "http"; @@ -7,17 +7,24 @@ import { https as httpsFr } from "follow-redirects"; import { execSync } from "child_process"; import * as decompress from "decompress"; const decompressTargz = require("decompress-targz"); +const decompressUnzip = require("decompress-unzip"); const TAG = require("../package.json").repository.tag; function checkIfBinaryExists(binaryPath: string) { - let whichCommand: string, binaryExists: boolean; - if (platform() == "win32") whichCommand = "where"; - else whichCommand = "command -v"; + let whichCommand: string; + let binaryExists: boolean; + + if (platform() == "win32") { + let parsedBinPath = path.parse(binaryPath); + whichCommand = `where "${parsedBinPath.dir}:${parsedBinPath.base}"`; + } else { + whichCommand = `command -v ${binaryPath}`; + } binaryExists = true; try { - execSync(`${whichCommand} ${binaryPath}`, { windowsHide: true }); + execSync(whichCommand, { windowsHide: true }); } catch { binaryExists = false; } @@ -29,6 +36,16 @@ export async function checkAndDownloadBinaries( binaryPath: string, output: vscode.OutputChannel ): Promise { + + output.appendLine("Platform: '" + platform() + "'"); + + // Update home paths to an absolute path + if (platform() != "win32" && binaryPath.startsWith("~/")) { + binaryPath = binaryPath.replace("~", ""); + binaryPath = path.join(homedir(), binaryPath); + output.appendLine(`Adjusted server path: ${binaryPath}`); + } + if (checkIfBinaryExists(binaryPath)) { // Language server binary exists -- nothing to do return binaryPath; @@ -49,8 +66,7 @@ export async function checkAndDownloadBinaries( "verible-verilog-ls" + (platform() === "win32" ? ".exe" : "") ); if (checkIfBinaryExists(pluginBinaryPath)) { - // Language server binary already downloaded - output.appendLine(`Using executable from path: ${pluginBinaryPath}`); + output.appendLine("Language server binary already downloaded"); return pluginBinaryPath; } @@ -85,6 +101,7 @@ export async function checkAndDownloadBinaries( await new Promise((resolve, reject) => httpsFr.get(releaseUrl, (response: IncomingMessage) => { if (response.statusCode !== 200){ + output.appendLine("Download failed with status code " + response.statusCode); reject("Status code " + response.statusCode); } response.pipe(archive); @@ -94,6 +111,7 @@ export async function checkAndDownloadBinaries( resolve(); }); }).on("error", (_err) =>{ + output.appendLine("Failed to start download"); return binaryPath; }) ); @@ -105,7 +123,7 @@ export async function checkAndDownloadBinaries( file.path = path.basename(file.path); return file; }, - plugins: platform() === "win32" ? [] : [decompressTargz()], + plugins: platform() === "win32" ? [decompressUnzip()] : [decompressTargz()], }).catch((_err) => { return binaryPath; }) @@ -113,6 +131,5 @@ export async function checkAndDownloadBinaries( fs.rm(archivePath, () => null); }); - output.appendLine(`Using executable from path: ${pluginBinaryPath}`); return pluginBinaryPath; } diff --git a/verilog/tools/ls/vscode/src/extension.ts b/verilog/tools/ls/vscode/src/extension.ts index 0afcb09dc..ac0a4b1c9 100644 --- a/verilog/tools/ls/vscode/src/extension.ts +++ b/verilog/tools/ls/vscode/src/extension.ts @@ -10,6 +10,8 @@ async function initLanguageClient() { const config = vscode.workspace.getConfiguration('verible'); const binary_path: string = await checkAndDownloadBinaries(config.get('path') as string, output); + output.appendLine(`Using executable from path: ${binary_path}`); + const verible_ls: vscodelc.Executable = { command: binary_path, args: await config.get('arguments') @@ -26,6 +28,7 @@ async function initLanguageClient() { }; // Create the language client and start the client. + output.appendLine("Starting Language Server"); client = new vscodelc.LanguageClient( 'verible', 'Verible Language Server', From 0af484af8d53973c93cafc202927f43d303a0ec7 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Wed, 1 May 2024 00:14:27 +1000 Subject: [PATCH 2/6] Updated lint test to match change in default proper parameter rule change (packages should use localparam) --- verilog/tools/lint/testdata/endif_comment.sv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From ab6be5661f279b542ff78ca6888a3371c3c5516f Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Fri, 7 Jun 2024 18:12:20 +1000 Subject: [PATCH 3/6] Clean up issues reporte by CI tools --- .../analysis/checkers/proper_parameter_declaration_rule.cc | 3 ++- verilog/analysis/checkers/proper_parameter_declaration_rule.h | 4 +++- .../checkers/proper_parameter_declaration_rule_test.cc | 1 - 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc index 6961fc728..3af9cc20b 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc @@ -16,6 +16,7 @@ #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" @@ -83,7 +84,7 @@ const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { } absl::Status ProperParameterDeclarationRule::Configure( - const absl::string_view configuration) { + absl::string_view configuration) { using verible::config::SetBool; return verible::ParseNameValues( configuration, diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.h b/verilog/analysis/checkers/proper_parameter_declaration_rule.h index ae49150e0..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,7 +42,7 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule { verible::LintRuleStatus Report() const final; - absl::Status Configure(const absl::string_view configuration); + absl::Status Configure(absl::string_view configuration) final; private: std::set violations_; diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc index 530d80ce0..6e0fd9b70 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc @@ -153,7 +153,6 @@ TEST(ProperParameterDeclarationRuleTest, CombinationParametersTest) { } TEST(ProperParameterDeclarationRuleTest, AllowPackageParameters) { - constexpr int kToken = SymbolIdentifier; const std::initializer_list kAllowPackageParametersTestCases = { {"parameter int Foo = 1;"}, {"package foo; ", "parameter int Bar = 1; endpackage"}, From dad52e3aff2fc21566c4b604fefebe9574608c01 Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Fri, 7 Jun 2024 18:12:20 +1000 Subject: [PATCH 4/6] Clean up issues reporte by CI tools --- verilog/analysis/checkers/BUILD | 1 + .../analysis/checkers/proper_parameter_declaration_rule.cc | 3 ++- verilog/analysis/checkers/proper_parameter_declaration_rule.h | 4 +++- .../checkers/proper_parameter_declaration_rule_test.cc | 1 - 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 17084a2c6..b032b4bd4 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1754,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/proper_parameter_declaration_rule.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc index 6961fc728..3af9cc20b 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc @@ -16,6 +16,7 @@ #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" @@ -83,7 +84,7 @@ const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { } absl::Status ProperParameterDeclarationRule::Configure( - const absl::string_view configuration) { + absl::string_view configuration) { using verible::config::SetBool; return verible::ParseNameValues( configuration, diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.h b/verilog/analysis/checkers/proper_parameter_declaration_rule.h index ae49150e0..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,7 +42,7 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule { verible::LintRuleStatus Report() const final; - absl::Status Configure(const absl::string_view configuration); + absl::Status Configure(absl::string_view configuration) final; private: std::set violations_; diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc index 530d80ce0..6e0fd9b70 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc @@ -153,7 +153,6 @@ TEST(ProperParameterDeclarationRuleTest, CombinationParametersTest) { } TEST(ProperParameterDeclarationRuleTest, AllowPackageParameters) { - constexpr int kToken = SymbolIdentifier; const std::initializer_list kAllowPackageParametersTestCases = { {"parameter int Foo = 1;"}, {"package foo; ", "parameter int Bar = 1; endpackage"}, From 55b8c527b9465743e6b6810d2b304046419c71ac Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Fri, 7 Jun 2024 22:19:34 +1000 Subject: [PATCH 5/6] Fixed bug where the error message was not being set correctly. --- verilog/analysis/checkers/proper_parameter_declaration_rule.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc index 3af9cc20b..3d5f36bb9 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc @@ -86,7 +86,7 @@ const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { absl::Status ProperParameterDeclarationRule::Configure( absl::string_view configuration) { using verible::config::SetBool; - return verible::ParseNameValues( + auto status = verible::ParseNameValues( configuration, { {"package_allow_parameter", SetBool(&package_allow_parameter_)}, @@ -99,6 +99,7 @@ absl::Status ProperParameterDeclarationRule::Configure( kLocalParamMessage = package_allow_localparam_ ? kLocalParamAllowPackageMessage : kLocalParamNotInPackageMessage; + return status; } static const Matcher &ParamDeclMatcher() { From d33c0a95dc7f7bc2b34a3d035bb59b114dbce5ee Mon Sep 17 00:00:00 2001 From: Steven Conway Date: Mon, 10 Jun 2024 21:15:38 +1000 Subject: [PATCH 6/6] Reverting back to the original beaviour (expect package parameters and reject pacakge localparams) limiting the changes in this PR to adding rule configuration. --- .../proper_parameter_declaration_rule.cc | 4 +-- .../proper_parameter_declaration_rule.h | 4 +-- .../proper_parameter_declaration_rule_test.cc | 32 ++++++++++++------- verilog/tools/lint/testdata/endif_comment.sv | 2 +- 4 files changed, 26 insertions(+), 16 deletions(-) diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc index 3d5f36bb9..4a60b980c 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc @@ -70,13 +70,13 @@ const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { .param = { { .name = "package_allow_parameter", - .default_value = "false", + .default_value = "true", .description = "Allow parameters in packages (treated as a " "synonym for localparam).", }, { .name = "package_allow_localparam", - .default_value = "true", + .default_value = "false", .description = "Allow localparams in packages.", }, }}; diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.h b/verilog/analysis/checkers/proper_parameter_declaration_rule.h index 410459154..e52311157 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.h +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.h @@ -47,8 +47,8 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule { private: std::set violations_; - bool package_allow_parameter_ = false; - bool package_allow_localparam_ = true; + bool package_allow_parameter_ = true; + bool package_allow_localparam_ = false; }; } // 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 6e0fd9b70..31d3501b0 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc @@ -42,8 +42,8 @@ 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; ", @@ -88,7 +88,10 @@ TEST(ProperParameterDeclarationRuleTest, ParameterTests) { "endmodule " "endmodule"}, }; - RunLintTestCases(kTestCases); + + RunConfiguredLintTestCases( + kTestCases, + "package_allow_parameter:false;package_allow_localparam:true"); } // Tests that the expected number of localparam usage violations are found. @@ -101,14 +104,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;"}, + {{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 @@ -149,13 +155,19 @@ TEST(ProperParameterDeclarationRuleTest, CombinationParametersTest) { {"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 kAllowPackageParametersTestCases = { + const std::initializer_list kTestCases = { {"parameter int Foo = 1;"}, - {"package foo; ", "parameter int Bar = 1; endpackage"}, + {"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"}, @@ -209,9 +221,7 @@ TEST(ProperParameterDeclarationRuleTest, AllowPackageParameters) { "endpackage"}, }; - RunConfiguredLintTestCases( - kAllowPackageParametersTestCases, - "package_allow_parameter:true;package_allow_localparam:false"); + RunLintTestCases(kTestCases); } } // namespace diff --git a/verilog/tools/lint/testdata/endif_comment.sv b/verilog/tools/lint/testdata/endif_comment.sv index f125e1cba..50d23bcd8 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 - localparam int P = 4; + parameter int P = 4; `endif endpackage