Fix rsc compile transitive dep bug introduced in #7742 (#7825)

### Problem

#7742 removed the method `self._on_invalid_compile_dependency()` from `JvmCompile`, as it seemed complex and it wasn't clear if it was still needed. We were able to reproduce an error internally which made it clear that logic was still necessary, so that method was added back, along with some testing to avoid further regression.

### Solution

- Re-add `self._on_invalid_compile_dependency()`.
- Refactor rsc compile integration testing to remove the manual split between between hermetic and nonhermetic testing.
- Add a test case for a `zinc-java` target depending transitively on a scala target compiled with `rsc-and-zinc`.

### Result

The rsc compile task passes our internal CI job which compiles some code using rsc and zinc!

src/python/pants/backend/jvm/subsystems/resolve_subsystem.py

@@ -19,7 +19,10 @@ class JvmResolveSubsystem(Subsystem):
   """
   options_scope = 'resolver'
 
+  # TODO: Convert to an enum.
+  CHOICES = ['ivy', 'coursier']
+
   @classmethod
   def register_options(cls, register):
     super(JvmResolveSubsystem, cls).register_options(register)
-    register('--resolver', choices=['ivy', 'coursier'], default='ivy', help='Resolver to use for external jvm dependencies.')
+    register('--resolver', choices=cls.CHOICES, default='ivy', help='Resolver to use for external jvm dependencies.')

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py

@@ -761,12 +761,21 @@ def predicate(target):
         return True
       if target in invalid_target_set:
         invalid_dependencies.add(target)
-        return False
+        return self._on_invalid_compile_dependency(target, compile_target, compile_contexts)
       return True
 
     compile_target.walk(work, predicate)
     return invalid_dependencies
 
+  def _on_invalid_compile_dependency(self, dep, compile_target, compile_contexts):
+    """Decide whether to continue searching for invalid targets to use in the execution graph.
+
+    By default, don't recurse because once we have an invalid dependency, we can rely on its
+    dependencies having been compiled already.
+
+    Override to adjust this behavior."""
+    return False
+
   def _create_context_jar(self, compile_context):
     """Jar up the compile_context to its output jar location.
 

src/python/pants/backend/jvm/tasks/jvm_compile/rsc/rsc_compile.py

@@ -703,3 +703,22 @@ def _jdk_libs_paths_and_digest(self, hermetic_dist):
   @memoized_method
   def _jdk_libs_abs(self, nonhermetic_dist):
     return nonhermetic_dist.find_libs(self._JDK_LIB_NAMES)
+
+  # TODO: rename this, make it public, and explain what "searching for invalid targets" refers to!
+  def _on_invalid_compile_dependency(self, dep, compile_target, contexts):
+    """Decide whether to continue searching for invalid targets to use in the execution graph.
+
+    If a necessary dep is a rsc-and-zinc dep and the root is a zinc-only one, continue to recurse
+    because otherwise we'll drop the path between Zinc compile of the zinc-only target and a Zinc
+    compile of a transitive rsc-and-zinc dependency.
+
+    This is only an issue for graphs like J -> S1 -> S2, where J is a zinc-only target,
+    S1/2 are rsc-and-zinc targets and S2 must be on the classpath to compile J successfully.
+    """
+    def dep_has_rsc_compile():
+      return contexts[dep].rsc_cc.workflow == self.JvmCompileWorkflowType.rsc_and_zinc
+    return contexts[compile_target].rsc_cc.workflow.resolve_for_enum_variant({
+      'zinc-java': dep_has_rsc_compile,
+      'zinc-only': dep_has_rsc_compile,
+      'rsc-and-zinc': lambda: False
+    })()

src/python/pants/util/objects.py

@@ -115,6 +115,12 @@ def __new__(cls, *args, **kwargs):
       # more ergonomic.
       type_failure_msgs = []
       for field_name, field_constraint in fields_with_constraints.items():
+        # TODO: figure out how to disallow users from accessing datatype fields by index!
+        # TODO: gettattr() with a specific `field_name` against a `namedtuple` is apparently
+        # converted into a __getitem__() call with the argument being the integer index of the field
+        # with that name -- this indirection is not shown in the stack trace when overriding
+        # __getitem__() to raise on `int` inputs. See https://stackoverflow.com/a/6738724 for the
+        # greater context of how `namedtuple` differs from other "normal" python classes.
         field_value = getattr(this_object, field_name)
         try:
           field_constraint.validate_satisfied_by(field_value)

testprojects/src/scala/org/pantsbuild/testproject/javadepsonscalatransitive/BUILD

@@ -1,14 +1,33 @@
 # Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
 # Licensed under the Apache License, Version 2.0 (see LICENSE).
 
+# This java_library() target transitively depending on an rsc-compiled scala_library() target
+# triggers some logic to traverse more deeply through dependencies to get the complete classpath.
+java_library(
+  name = 'java-in-different-package',
+  sources = ['Java.java'],
+  dependencies = [':scala-in-different-package'],
+  strict_deps = True,
+)
+
+scala_library(
+  name = 'scala-in-different-package',
+  sources = ['ScalaInDifferentPackage.scala'],
+  dependencies = [':scala'],
+  exports = [':scala'],
+  strict_deps = True,
+)
+
 scala_library(
   name = 'scala',
   sources = ['Scala.scala'],
   dependencies = [':scala-2'],
-  exports = [':scala-2']
+  exports = [':scala-2'],
+  tags = {'use-compiler:rsc-and-zinc'},
 )
 
 scala_library(
   name = 'scala-2',
-  sources = ['Scala2.scala']
+  sources = ['Scala2.scala'],
+  tags = {'use-compiler:rsc-and-zinc'},
 )

testprojects/src/scala/org/pantsbuild/testproject/javadepsonscalatransitive/Java.java

@@ -0,0 +1,19 @@
+// Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
+// Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+package org.pantsbuild.testproject.java.javadepsonscalatransitive;
+
+import org.pantsbuild.testproject.javadepsonscalatransitive.Scala;
+import org.pantsbuild.testproject.javadepsonscalatransitive.Scala2;
+
+public class Java {
+
+  public String doStuff() {
+      // This should not trigger a missing dependency warning
+      // since we actually depend on the scala library.
+    Scala scala = new Scala();
+    Scala2 scala2 = new Scala2();
+    return scala.toString() + scala2.toString();
+  }
+
+}

testprojects/src/scala/org/pantsbuild/testproject/javadepsonscalatransitive/ScalaInDifferentPackage.scala

@@ -0,0 +1,6 @@
+// Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
+// Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+package org.pantsbuild.testproject.scala.javadepsonscalatransitive
+
+object ScalaInDifferentPackage

tests/python/pants_test/backend/jvm/tasks/jvm_compile/rsc/test_rsc_compile.py

@@ -202,7 +202,8 @@ def test_rsc_dep_for_scala_java_and_test_targets(self):
                      zinc[rsc-and-zinc](scala/classpath:scala_lib) -> {}
                      rsc(scala/classpath:scala_dep) -> {
                        rsc(scala/classpath:scala_lib),
-                       zinc[rsc-and-zinc](scala/classpath:scala_lib)
+                       zinc[rsc-and-zinc](scala/classpath:scala_lib),
+                       zinc[zinc-only](scala/classpath:scala_test)
                      }
                      zinc[rsc-and-zinc](scala/classpath:scala_dep) -> {
                        zinc[zinc-java](java/classpath:java_lib)

tests/python/pants_test/backend/jvm/tasks/jvm_compile/rsc/test_rsc_compile_integration.py

@@ -7,127 +7,82 @@
 import os
 from functools import wraps
 
+from pants.backend.jvm.subsystems.resolve_subsystem import JvmResolveSubsystem
+from pants.backend.jvm.tasks.jvm_compile.rsc.rsc_compile import RscCompile
 from pants.util.contextutil import temporary_dir
 from pants_test.backend.jvm.tasks.jvm_compile.base_compile_integration_test import BaseCompileIT
 
 
-def _execution_strategies(strategies, workers_range=[1]):
-  def wrapper(func):
-    @wraps(func)
-    def wrapper_self(*args, **kwargs):
-      for worker_count in workers_range:
-        for strategy in strategies:
-          func(*args, execution_strategy=strategy, worker_count=worker_count, **kwargs)
-    return wrapper_self
-  return wrapper
+def _for_all_supported_execution_environments(func):
+  @wraps(func)
+  def wrapper_self(*args, **kwargs):
+    for worker_count in [1, 2]:
+      for resolver in JvmResolveSubsystem.CHOICES:
+        for execution_strategy in RscCompile.ExecutionStrategy.all_variants:
+          with temporary_dir() as cache_dir:
+            config = {
+              'cache.compile.rsc': {'write_to': [cache_dir]},
+              'jvm-platform': {'compiler': 'rsc'},
+              'compile.rsc': {
+                'execution_strategy': execution_strategy.value,
+                'worker_count': worker_count,
+              },
+              'resolver': {
+                'resolver': resolver,
+              }
+            }
 
+            def populate_necessary_hermetic_options():
+              config['compile.rsc'].update({
+                'incremental': False,
+                'use_classpath_jars': False,
+              })
+              if resolver == 'ivy':
+                resolver_scope = 'resolve.ivy'
+              else:
+                assert resolver == 'coursier'
+                resolver_scope = 'resolve.coursier'
+              config[resolver_scope] = {
+                'capture_snapshots': True,
+              }
 
-class RscCompileIntegration(BaseCompileIT):
-  @_execution_strategies(['nailgun', 'subprocess'])
-  def test_basic_binary_nonhermetic(self, execution_strategy, worker_count):
-    with temporary_dir() as cache_dir:
-      config = {
-        'cache.compile.rsc': {'write_to': [cache_dir]},
-        'jvm-platform': {'compiler': 'rsc'},
-        'compile.rsc': {
-          'execution_strategy': execution_strategy,
-          'worker_count': worker_count,
-        },
-      }
+            execution_strategy.resolve_for_enum_variant({
+              'nailgun': lambda: None,
+              'subprocess': lambda: None,
+              'hermetic': populate_necessary_hermetic_options,
+            })()
 
-      pants_run = self.run_pants(
-        ['compile',
-         'testprojects/src/scala/org/pantsbuild/testproject/mutual:bin',
-         ],
-        config)
-      self.assert_success(pants_run)
+            func(*args, config=config, **kwargs)
+  return wrapper_self
 
-  def test_basic_binary_hermetic(self):
-    with temporary_dir() as cache_dir:
-      config = {
-        'cache.compile.rsc': {'write_to': [cache_dir]},
-        'jvm-platform': {'compiler': 'rsc'},
-        'compile.rsc': {
-          'execution_strategy': 'hermetic',
-          'incremental': False,
-        }
-      }
 
-      with self.temporary_workdir() as workdir:
-        pants_run = self.run_pants_with_workdir(
-          ['compile',
-           'testprojects/src/scala/org/pantsbuild/testproject/mutual:bin',
-           ],
-          workdir, config)
-        self.assert_success(pants_run)
-        path = os.path.join(
-          workdir,
-          'compile/rsc/current/testprojects.src.scala.org.pantsbuild.testproject.mutual.mutual/current/zinc',
-          'classes/org/pantsbuild/testproject/mutual/A.class')
-        self.assertTrue(os.path.exists(path))
-        path = os.path.join(
-          workdir,
-          'compile/rsc/current/testprojects.src.scala.org.pantsbuild.testproject.mutual.mutual/current/rsc',
-          'm.jar')
-        self.assertTrue(os.path.exists(path))
+class RscCompileIntegration(BaseCompileIT):
 
-  @_execution_strategies(['nailgun', 'subprocess'], [2])
-  def test_executing_multi_target_binary_nonhermetic(self, execution_strategy, worker_count):
-    with temporary_dir() as cache_dir:
-      config = {
-        'cache.compile.rsc': {'write_to': [cache_dir]},
-        'jvm-platform': {'compiler': 'rsc'},
-        'compile.rsc': {
-          'execution_strategy': execution_strategy,
-          'worker_count': worker_count,
-        }
-      }
-      with self.temporary_workdir() as workdir:
-        pants_run = self.run_pants_with_workdir(
-          ['run',
-           'examples/src/scala/org/pantsbuild/example/hello/exe',
-           ],
-          workdir, config)
-        self.assert_success(pants_run)
-        self.assertIn('Hello, Resource World!', pants_run.stdout_data)
+  @_for_all_supported_execution_environments
+  def test_basic_binary(self, config):
+    with self.do_command_yielding_workdir(
+        'compile', 'testprojects/src/scala/org/pantsbuild/testproject/mutual:bin',
+        config=config) as pants_run:
+      zinc_compiled_classfile = os.path.join(
+        pants_run.workdir,
+        'compile/rsc/current/testprojects.src.scala.org.pantsbuild.testproject.mutual.mutual/current/zinc',
+        'classes/org/pantsbuild/testproject/mutual/A.class')
+      self.assertIsFile(zinc_compiled_classfile)
+      rsc_header_jar = os.path.join(
+        pants_run.workdir,
+        'compile/rsc/current/testprojects.src.scala.org.pantsbuild.testproject.mutual.mutual/current/rsc',
+        'm.jar')
+      self.assertIsFile(rsc_header_jar)
 
-  def test_executing_multi_target_binary_hermetic(self):
-    with temporary_dir() as cache_dir:
-      config = {
-        'cache.compile.rsc': {'write_to': [cache_dir]},
-        'jvm-platform': {'compiler': 'rsc'},
-        'compile.rsc': {
-          'execution_strategy': 'hermetic',
-          'incremental': False
-        },
-        'resolve.ivy': {
-          'capture_snapshots': True
-        },
-      }
-      with self.temporary_workdir() as workdir:
-        pants_run = self.run_pants_with_workdir(
-          ['run',
-            'examples/src/scala/org/pantsbuild/example/hello/exe',
-          ],
-          workdir, config)
-        self.assert_success(pants_run)
-        self.assertIn('Hello, Resource World!', pants_run.stdout_data)
+  @_for_all_supported_execution_environments
+  def test_executing_multi_target_binary(self, config):
+    pants_run = self.do_command(
+      'run', 'examples/src/scala/org/pantsbuild/example/hello/exe',
+      config=config)
+    self.assertIn('Hello, Resource World!', pants_run.stdout_data)
 
-  @_execution_strategies(['nailgun', 'subprocess'], [2])
-  def test_java_with_transitive_exported_scala_dep_nonhermetic(self, execution_strategy, worker_count):
-    with temporary_dir() as cache_dir:
-      config = {
-        'cache.compile.rsc': {'write_to': [cache_dir]},
-        'jvm-platform': {'compiler': 'rsc'},
-        'compile.rsc': {
-          'execution_strategy': execution_strategy,
-          'worker_count': worker_count,
-        },
-      }
-      with self.temporary_workdir() as workdir:
-        pants_run = self.run_pants_with_workdir(
-          ['compile',
-            'testprojects/src/scala/org/pantsbuild/testproject/javadepsonscalatransitive:scala',
-          ],
-          workdir, config)
-        self.assert_success(pants_run)
+  @_for_all_supported_execution_environments
+  def test_java_with_transitive_exported_scala_dep(self, config):
+    self.do_command(
+      'compile', 'testprojects/src/scala/org/pantsbuild/testproject/javadepsonscalatransitive:java-in-different-package',
+      config=config)