Make scaladoc rule handle transient deps better (and add some tests) (#1584)

* fix scaladoc rule to handle transient deps better
Also Refactors and tests

* lint fixes

Author: crt-31

scala/private/rules/scala_doc.bzl

@@ -3,9 +3,9 @@
 load("@io_bazel_rules_scala//scala/private:common.bzl", "collect_plugin_paths")
 
 ScaladocAspectInfo = provider(fields = [
-    "src_files",
-    "compile_jars",
-    "plugins",
+    "src_files",  #depset[File]
+    "compile_jars",  #depset[File]
+    "plugins",  #depset[Target]
 ])
 
 def _scaladoc_intransitive_aspect_impl(target, ctx):
@@ -15,40 +15,39 @@ def _scaladoc_intransitive_aspect_impl(target, ctx):
 def _scaladoc_aspect_impl(target, ctx, transitive = True):
     """Collect source files and compile_jars from JavaInfo-returning deps."""
 
+    src_files = depset()
+    plugins = depset()
+    compile_jars = depset()
+
     # We really only care about visited targets with srcs, so only look at those.
     if hasattr(ctx.rule.attr, "srcs"):
         # Collect only Java and Scala sources enumerated in visited targets, including src_files in deps.
-        direct_deps = [file for file in ctx.rule.files.srcs if file.extension.lower() in ["java", "scala"]]
-
-        # Sometimes we only want to generate scaladocs for a single target and not all of its
-        # dependencies
-        if transitive:
-            transitive_deps = [dep[ScaladocAspectInfo].src_files for dep in ctx.rule.attr.deps if ScaladocAspectInfo in dep]
-        else:
-            transitive_deps = []
-
-        src_files = depset(direct = direct_deps, transitive = transitive_deps)
-
-        # Collect compile_jars from visited targets' deps.
-        compile_jars = depset(
-            direct = [file for file in ctx.rule.files.deps],
-            transitive = (
-                [dep[JavaInfo].compile_jars for dep in ctx.rule.attr.deps if JavaInfo in dep] +
-                [dep[ScaladocAspectInfo].compile_jars for dep in ctx.rule.attr.deps if ScaladocAspectInfo in dep]
-            ),
-        )
+        src_files = depset([file for file in ctx.rule.files.srcs if file.extension.lower() in ["java", "scala"]])
+
+        compile_jars = target[JavaInfo].transitive_compile_time_jars
 
-        plugins = depset()
         if hasattr(ctx.rule.attr, "plugins"):
-            plugins = depset(direct = ctx.rule.attr.plugins)
-
-        return [ScaladocAspectInfo(
-            src_files = src_files,
-            compile_jars = compile_jars,
-            plugins = plugins,
-        )]
-    else:
-        return []
+            plugins = depset(ctx.rule.attr.plugins)
+
+    # Sometimes we only want to generate scaladocs for a single target and not all of its
+    # dependencies
+    transitive_srcs = depset()
+    transitive_compile_jars = depset()
+    transitive_plugins = depset()
+
+    if transitive:
+        for dep in ctx.rule.attr.deps:
+            if ScaladocAspectInfo in dep:
+                aspec_info = dep[ScaladocAspectInfo]
+                transitive_srcs = aspec_info.src_files
+                transitive_compile_jars = aspec_info.compile_jars
+                transitive_plugins = aspec_info.plugins
+
+    return [ScaladocAspectInfo(
+        src_files = depset(transitive = [src_files, transitive_srcs]),
+        compile_jars = depset(transitive = [compile_jars, transitive_compile_jars]),
+        plugins = depset(transitive = [plugins, transitive_plugins]),
+    )]
 
 _scaladoc_transitive_aspect = aspect(
     implementation = _scaladoc_aspect_impl,

test/shell/test_scaladoc.sh

@@ -0,0 +1,63 @@
+# shellcheck source=./test_runner.sh
+
+dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+. "${dir}"/test_runner.sh
+. "${dir}"/test_helper.sh
+runner=$(get_test_runner "${1:-local}")
+
+_check_file_existence() {
+  local filepath=$1
+  local expect=$2 #1 for yes, 0 for no
+  
+  if [[ -f "$filepath" ]] ; then 
+    if [[ $expect == 0 ]]; then 
+      echo "Error: Unexpected scaladoc file found: ${filepath} "
+      exit 1;
+    fi    
+  else
+    if [[ $expect == 1 ]]; then
+      echo "Error: Expected scaladoc file not found: ${filepath} "
+      exit 1;
+    fi
+  fi    
+}
+
+test_scaladoc_transitive() {
+  #Verify that docs are generated for B AND A. (B depends on A).
+  set -e
+
+  bazel build //test_expect_failure/scaladoc:scaladoc_transitive --extra_toolchains=//test/toolchains:ast_plus_one_deps_unused_deps_warn
+
+  local scaladoc_outpath="$(bazel cquery //test_expect_failure/scaladoc:scaladoc_transitive --extra_toolchains=//test/toolchains:ast_plus_one_deps_unused_deps_warn --output=files)"  
+
+  _check_file_existence ${scaladoc_outpath}/"B$.html" 1
+  _check_file_existence ${scaladoc_outpath}/"A$.html" 1  
+}
+
+test_scaladoc_intransitive() {
+  #Verify that docs only generated for B. (B depends on A)
+
+  set -e
+
+  bazel build //test_expect_failure/scaladoc:scaladoc_intransitive  --extra_toolchains=//test/toolchains:ast_plus_one_deps_unused_deps_warn
+  
+  local scaladoc_outpath="$(bazel cquery //test_expect_failure/scaladoc:scaladoc_intransitive --extra_toolchains=//test/toolchains:ast_plus_one_deps_unused_deps_warn --output=files)"  
+
+  _check_file_existence ${scaladoc_outpath}/"B$.html" 1
+  _check_file_existence ${scaladoc_outpath}/"A$.html" 0  
+}
+
+test_scaladoc_works_with_transitive_external_deps() {
+  #Tests absense of a bug where scaladoc rule wasn't handling transitive dependencies that aren't scala_xxxx (i.e. don't hav a srcs attribute)
+  #Note: need to use a plus-one toolchain to enable transitive deps.
+
+  set -e
+  
+  #Just make sure it builds correctly
+  bazel build //test_expect_failure/scaladoc:scaladoc_C --extra_toolchains=//test/toolchains:ast_plus_one_deps_unused_deps_warn
+   
+}
+
+$runner test_scaladoc_intransitive
+$runner test_scaladoc_transitive
+$runner test_scaladoc_works_with_transitive_external_deps

test_expect_failure/scaladoc/A.scala

@@ -0,0 +1,6 @@
+
+object A {
+  def main() {
+    println("hi")
+  }
+}

test_expect_failure/scaladoc/B.scala

@@ -0,0 +1,5 @@
+object B {
+  def myfunc() {
+    A.main()
+  }
+}

test_expect_failure/scaladoc/BUILD

@@ -0,0 +1,55 @@
+load("scaladoc.bzl", "scala_doc_intransitive")
+load("//scala:scala.bzl", "scala_doc", "scala_library")
+load("//scala:scala_import.bzl", "scala_import")
+
+package(default_testonly = 1)
+
+scala_library(
+    name = "A",
+    srcs = ["A.scala"],
+    deps = [],
+)
+
+#B depends on A
+scala_library(
+    name = "B",
+    srcs = ["B.scala"],
+    deps = ["A"],
+)
+
+#Simulate A & B as if they were imported libraries (in particular they are targets that do not have srcs attribute)
+scala_import(
+    name = "ImportedA",
+    jars = ["A"],
+)
+
+scala_import(
+    name = "ImportedB",
+    jars = ["B"],
+    deps = ["ImportedA"],
+)
+
+#Setup this scala_library target so that it has dependency on a transitive external lib.
+scala_library(
+    name = "C",
+    srcs = ["C.scala"],
+    deps = [
+        "ImportedB",
+        #For the test, don't include ImportedA here... we want it to be loaded transitively
+    ],
+)
+
+scala_doc(
+    name = "scaladoc_transitive",
+    deps = ["B"],
+)
+
+scala_doc_intransitive(
+    name = "scaladoc_intransitive",
+    deps = ["B"],
+)
+
+scala_doc(
+    name = "scaladoc_C",
+    deps = ["C"],
+)

test_expect_failure/scaladoc/C.scala

@@ -0,0 +1,7 @@
+//import cats.syntax.all._
+
+object C {
+  def myfunc()  = {
+    A.main() //Call into A, which is a transitive dependency of this lib    
+  }
+}

test_expect_failure/scaladoc/scaladoc.bzl

@@ -0,0 +1,3 @@
+load("//scala:scala.bzl", "make_scala_doc_rule", "scaladoc_intransitive_aspect")
+
+scala_doc_intransitive = make_scala_doc_rule(scaladoc_intransitive_aspect)  #Only scaladoc specified deps

test_rules_scala.sh

@@ -57,3 +57,4 @@ $runner bazel build //test_statsfile:SimpleNoStatsFile_statsfile --extra_toolcha
 . "${test_dir}"/test_inherited_environment.sh
 . "${test_dir}"/test_persistent_worker.sh
 . "${test_dir}"/test_semanticdb.sh
+. "${test_dir}"/test_scaladoc.sh