Update clippy and rustfmt aspects to require CrateInfo providers (#1772)

* Update clippy and rustfmt aspects to require CrateInfo providers

* Updated rustfmt rules to work with `rust_shared/static_library`

* Regenerate documentation

Author: UebelAndre

.bazelci/presubmit.yml

@@ -292,7 +292,7 @@ tasks:
     name: Negative Rustfmt Tests
     platform: ubuntu2004
     run_targets:
-      - "//test/rustfmt:test_runner"
+      - "//test/rustfmt:rustfmt_integration_test_suite.test_runner"
   rust_analyzer_tests:
     name: Rust-Analyzer Tests
     platform: ubuntu2004

docs/flatten.md

@@ -2082,7 +2082,6 @@ This aspect is used to gather information about a crate for use in rustfmt and p
 
 Output Groups:
 
-- `rustfmt_manifest`: A manifest used by rustfmt binaries to provide crate specific settings.
 - `rustfmt_checks`: Executes `rustfmt --check` on the specified target.
 
 The build setting `@rules_rust//:rustfmt.toml` is used to control the Rustfmt [configuration settings][cs]

docs/rust_fmt.md

@@ -97,7 +97,6 @@ This aspect is used to gather information about a crate for use in rustfmt and p
 
 Output Groups:
 
-- `rustfmt_manifest`: A manifest used by rustfmt binaries to provide crate specific settings.
 - `rustfmt_checks`: Executes `rustfmt --check` on the specified target.
 
 The build setting `@rules_rust//:rustfmt.toml` is used to control the Rustfmt [configuration settings][cs]

rust/private/clippy.bzl

@@ -45,7 +45,7 @@ clippy_flags = rule(
     build_setting = config.string_list(flag = True),
 )
 
-def _get_clippy_ready_crate_info(target, aspect_ctx):
+def _get_clippy_ready_crate_info(target, aspect_ctx = None):
     """Check that a target is suitable for clippy and extract the `CrateInfo` provider from it.
 
     Args:
@@ -72,11 +72,13 @@ def _get_clippy_ready_crate_info(target, aspect_ctx):
                 return None
 
     # Obviously ignore any targets that don't contain `CrateInfo`
-    if rust_common.crate_info not in target:
+    if rust_common.crate_info in target:
+        return target[rust_common.crate_info]
+    elif rust_common.test_crate_info in target:
+        return target[rust_common.test_crate_info].crate
+    else:
         return None
 
-    return target[rust_common.crate_info]
-
 def _clippy_aspect_impl(target, ctx):
     crate_info = _get_clippy_ready_crate_info(target, ctx)
     if not crate_info:
@@ -225,6 +227,10 @@ rust_clippy_aspect = aspect(
         ),
     },
     provides = [ClippyInfo],
+    required_providers = [
+        [rust_common.crate_info],
+        [rust_common.test_crate_info],
+    ],
     toolchains = [
         str(Label("//rust:toolchain_type")),
         "@bazel_tools//tools/cpp:toolchain_type",

rust/private/rustfmt.bzl

@@ -2,25 +2,41 @@
 
 load(":common.bzl", "rust_common")
 
-def _find_rustfmtable_srcs(target, aspect_ctx = None):
-    """Parse a target for rustfmt formattable sources.
+def _get_rustfmt_ready_crate_info(target):
+    """Check that a target is suitable for rustfmt and extract the `CrateInfo` provider from it.
 
     Args:
         target (Target): The target the aspect is running on.
-        aspect_ctx (ctx, optional): The aspect's context object.
 
     Returns:
-        list: A list of formattable sources (`File`).
+        CrateInfo, optional: A `CrateInfo` provider if clippy should be run or `None`.
     """
-    if rust_common.crate_info not in target:
-        return []
 
     # Ignore external targets
     if target.label.workspace_root.startswith("external"):
-        return []
+        return None
+
+    # Obviously ignore any targets that don't contain `CrateInfo`
+    if rust_common.crate_info in target:
+        return target[rust_common.crate_info]
+    elif rust_common.test_crate_info in target:
+        return target[rust_common.test_crate_info].crate
+    else:
+        return None
+
+def _find_rustfmtable_srcs(crate_info, aspect_ctx = None):
+    """Parse a `CrateInfo` provider for rustfmt formattable sources.
+
+    Args:
+        crate_info (CrateInfo): A `CrateInfo` provider.
+        aspect_ctx (ctx, optional): The aspect's context object.
+
+    Returns:
+        list: A list of formattable sources (`File`).
+    """
 
+    # Targets with specific tags will not be formatted
     if aspect_ctx:
-        # Targets with specifc tags will not be formatted
         ignore_tags = [
             "no-format",
             "no-rustfmt",
@@ -31,8 +47,6 @@ def _find_rustfmtable_srcs(target, aspect_ctx = None):
             if tag in aspect_ctx.rule.attr.tags:
                 return []
 
-    crate_info = target[rust_common.crate_info]
-
     # Filter out any generated files
     srcs = [src for src in crate_info.srcs.to_list() if src.is_source]
 
@@ -83,21 +97,23 @@ def _perform_check(edition, srcs, ctx):
     return marker
 
 def _rustfmt_aspect_impl(target, ctx):
-    srcs = _find_rustfmtable_srcs(target, ctx)
+    crate_info = _get_rustfmt_ready_crate_info(target)
+
+    if not crate_info:
+        return []
+
+    srcs = _find_rustfmtable_srcs(crate_info, ctx)
 
     # If there are no formattable sources, do nothing.
     if not srcs:
         return []
 
-    # Parse the edition to use for formatting from the target
-    edition = target[rust_common.crate_info].edition
+    edition = crate_info.edition
 
-    manifest = _generate_manifest(edition, srcs, ctx)
     marker = _perform_check(edition, srcs, ctx)
 
     return [
         OutputGroupInfo(
-            rustfmt_manifest = depset([manifest]),
             rustfmt_checks = depset([marker]),
         ),
     ]
@@ -109,7 +125,6 @@ This aspect is used to gather information about a crate for use in rustfmt and p
 
 Output Groups:
 
-- `rustfmt_manifest`: A manifest used by rustfmt binaries to provide crate specific settings.
 - `rustfmt_checks`: Executes `rustfmt --check` on the specified target.
 
 The build setting `@rules_rust//:rustfmt.toml` is used to control the Rustfmt [configuration settings][cs]
@@ -134,6 +149,48 @@ generated source files are also ignored by this aspect.
             default = Label("//util/process_wrapper"),
         ),
     },
+    incompatible_use_toolchain_transition = True,
+    required_providers = [
+        [rust_common.crate_info],
+        [rust_common.test_crate_info],
+    ],
+    fragments = ["cpp"],
+    host_fragments = ["cpp"],
+    toolchains = [
+        str(Label("//rust/rustfmt:toolchain_type")),
+    ],
+)
+
+def _rustfmt_test_manifest_aspect_impl(target, ctx):
+    crate_info = _get_rustfmt_ready_crate_info(target)
+
+    if not crate_info:
+        return []
+
+    # Parse the edition to use for formatting from the target
+    edition = crate_info.edition
+
+    srcs = _find_rustfmtable_srcs(crate_info, ctx)
+    manifest = _generate_manifest(edition, srcs, ctx)
+
+    return [
+        OutputGroupInfo(
+            rustfmt_manifest = depset([manifest]),
+        ),
+    ]
+
+# This aspect contains functionality split out of `rustfmt_aspect` which broke when
+# `required_providers` was added to it. Aspects which have `required_providers` seems
+# to not function with attributes that also require providers.
+_rustfmt_test_manifest_aspect = aspect(
+    implementation = _rustfmt_test_manifest_aspect_impl,
+    doc = """\
+This aspect is used to gather information about a crate for use in `rustfmt_test`
+
+Output Groups:
+
+- `rustfmt_manifest`: A manifest used by rustfmt binaries to provide crate specific settings.
+""",
     incompatible_use_toolchain_transition = True,
     fragments = ["cpp"],
     host_fragments = ["cpp"],
@@ -158,8 +215,13 @@ def _rustfmt_test_impl(ctx):
         is_executable = True,
     )
 
-    manifests = depset(transitive = [target[OutputGroupInfo].rustfmt_manifest for target in ctx.attr.targets])
-    srcs = [depset(_find_rustfmtable_srcs(target)) for target in ctx.attr.targets]
+    crate_infos = [_get_rustfmt_ready_crate_info(target) for target in ctx.attr.targets]
+    srcs = [depset(_find_rustfmtable_srcs(crate_info)) for crate_info in crate_infos if crate_info]
+
+    # Some targets may be included in tests but tagged as "no-format". In this
+    # case, there will be no manifest.
+    manifests = [getattr(target[OutputGroupInfo], "rustfmt_manifest", None) for target in ctx.attr.targets]
+    manifests = depset(transitive = [manifest for manifest in manifests if manifest])
 
     runfiles = ctx.runfiles(
         transitive_files = depset(transitive = srcs + [manifests]),
@@ -192,8 +254,11 @@ rustfmt_test = rule(
     attrs = {
         "targets": attr.label_list(
             doc = "Rust targets to run `rustfmt --check` on.",
-            providers = [rust_common.crate_info],
-            aspects = [rustfmt_aspect],
+            providers = [
+                [rust_common.crate_info],
+                [rust_common.test_crate_info],
+            ],
+            aspects = [_rustfmt_test_manifest_aspect],
         ),
         "_runner": attr.label(
             doc = "The rustfmt test runner",

test/rustfmt/BUILD.bazel

@@ -1,58 +1,9 @@
-load("@rules_rust//rust:defs.bzl", "rust_binary", "rustfmt_test")
+load(":rustfmt_integration_test_suite.bzl", "rustfmt_integration_test_suite")
 
 exports_files([
     "test_rustfmt.toml",
 ])
 
-rust_binary(
-    name = "formatted_2018",
-    srcs = ["srcs/2018/formatted.rs"],
-    edition = "2018",
-)
-
-rustfmt_test(
-    name = "test_formatted_2018",
-    targets = [":formatted_2018"],
-)
-
-rust_binary(
-    name = "unformatted_2018",
-    srcs = ["srcs/2018/unformatted.rs"],
-    edition = "2018",
-    tags = ["norustfmt"],
-)
-
-rustfmt_test(
-    name = "test_unformatted_2018",
-    tags = ["manual"],
-    targets = [":unformatted_2018"],
-)
-
-rust_binary(
-    name = "formatted_2015",
-    srcs = ["srcs/2015/formatted.rs"],
-    edition = "2015",
-)
-
-rustfmt_test(
-    name = "test_formatted_2015",
-    targets = [":formatted_2015"],
-)
-
-rust_binary(
-    name = "unformatted_2015",
-    srcs = ["srcs/2015/unformatted.rs"],
-    edition = "2015",
-    tags = ["norustfmt"],
-)
-
-rustfmt_test(
-    name = "test_unformatted_2015",
-    tags = ["manual"],
-    targets = [":unformatted_2015"],
-)
-
-sh_binary(
-    name = "test_runner",
-    srcs = ["rustfmt_failure_test.sh"],
+rustfmt_integration_test_suite(
+    name = "rustfmt_integration_test_suite",
 )

test/rustfmt/rustfmt_failure_test.sh

@@ -36,6 +36,7 @@ function check_build_result() {
 function test_all_and_apply() {
   local -r TEST_OK=0
   local -r TEST_FAILED=3
+  local -r VARIANTS=(rust_binary rust_library rust_shared_library rust_static_library)
 
   temp_dir="$(mktemp -d -t ci-XXXXXXXXXX)"
   new_workspace="${temp_dir}/rules_rust_test_rustfmt"
@@ -58,30 +59,35 @@ EOF
   else
     SEDOPTS=(-i)
   fi
-  sed ${SEDOPTS[@]} 's/"norustfmt"//' "${new_workspace}/test/rustfmt/BUILD.bazel"
+  sed ${SEDOPTS[@]} 's/"norustfmt"//' "${new_workspace}/test/rustfmt/rustfmt_integration_test_suite.bzl"
+  sed ${SEDOPTS[@]} 's/"manual"//' "${new_workspace}/test/rustfmt/rustfmt_integration_test_suite.bzl"
 
   pushd "${new_workspace}"
 
-  check_build_result $TEST_FAILED test_unformatted_2015
-  check_build_result $TEST_FAILED test_unformatted_2018
-  check_build_result $TEST_OK test_formatted_2015
-  check_build_result $TEST_OK test_formatted_2018
+  for variant in ${VARIANTS[@]}; do
+    check_build_result $TEST_FAILED ${variant}_unformatted_2015_test
+    check_build_result $TEST_FAILED ${variant}_unformatted_2018_test
+    check_build_result $TEST_OK ${variant}_formatted_2015_test
+    check_build_result $TEST_OK ${variant}_formatted_2018_test
+  done
 
   # Format a specific target
-  bazel run @rules_rust//tools/rustfmt -- //test/rustfmt:unformatted_2018
+  for variant in ${VARIANTS[@]}; do
+    bazel run @rules_rust//tools/rustfmt -- //test/rustfmt:${variant}_unformatted_2018
+  done
 
-  check_build_result $TEST_FAILED test_unformatted_2015
-  check_build_result $TEST_OK test_unformatted_2018
-  check_build_result $TEST_OK test_formatted_2015
-  check_build_result $TEST_OK test_formatted_2018
+  for variant in ${VARIANTS[@]}; do
+    check_build_result $TEST_FAILED ${variant}_unformatted_2015_test
+    check_build_result $TEST_OK ${variant}_unformatted_2018_test
+    check_build_result $TEST_OK ${variant}_formatted_2015_test
+    check_build_result $TEST_OK ${variant}_formatted_2018_test
+  done
 
   # Format all targets
   bazel run @rules_rust//tools/rustfmt --@rules_rust//:rustfmt.toml=//test/rustfmt:test_rustfmt.toml
 
-  check_build_result $TEST_OK test_unformatted_2015
-  check_build_result $TEST_OK test_unformatted_2018
-  check_build_result $TEST_OK test_formatted_2015
-  check_build_result $TEST_OK test_formatted_2018
+  # Ensure all tests pass
+  check_build_result $TEST_OK "*"
 
   popd