JsLibrary: Don't remove macro deps from build deps

Summary:
For legacy reasons, we were removing *all* but the worker dependency from build rule params when creating a `JsBundle`, where in reality we only wanted to remove declared deps and query deps.

This lead to dependencies coming from `extraJson` being unintentionally removed.

Here, we add new tests to guarantee the behavior we want, and only remove js_library dependencies from build rule params.

Reviewed By: fromcelticpark

fbshipit-source-id: cad3b34

src/com/facebook/buck/js/JsLibraryDescription.java

@@ -41,8 +41,13 @@
 import com.facebook.buck.core.util.immutables.BuckStyleImmutable;
 import com.facebook.buck.io.file.MorePaths;
 import com.facebook.buck.io.filesystem.ProjectFilesystem;
+import com.facebook.buck.rules.macros.BuildTargetMacro;
+import com.facebook.buck.rules.macros.MacroContainer;
+import com.facebook.buck.rules.macros.StringWithMacros;
+import com.facebook.buck.rules.query.Query;
 import com.facebook.buck.rules.query.QueryUtils;
 import com.facebook.buck.shell.WorkerTool;
+import com.facebook.buck.util.RichStream;
 import com.facebook.buck.util.types.Either;
 import com.facebook.buck.util.types.Pair;
 import com.google.common.base.Preconditions;
@@ -61,6 +66,7 @@
 import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import java.util.function.Function;
+import java.util.function.Predicate;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
 import org.immutables.value.Value;
@@ -112,14 +118,30 @@ public BuildRule createBuildRule(
 
     ProjectFilesystem projectFilesystem = context.getProjectFilesystem();
     CellPathResolver cellRoots = context.getCellPathResolver();
-    WorkerTool worker = graphBuilder.getRuleWithType(args.getWorker(), WorkerTool.class);
+    BuildTarget workerTarget = args.getWorker();
+    WorkerTool worker = graphBuilder.getRuleWithType(workerTarget, WorkerTool.class);
 
     // this params object is used as base for the JsLibrary build rule, but also for all dynamically
     // created JsFile rules.
     // For the JsLibrary case, we want to propagate flavors to library dependencies
     // For the JsFile case, we only want to depend on the worker, not on any libraries
+    Predicate<BuildTarget> isWorker = workerTarget::equals;
+    Predicate<BuildTarget> extraDepsFilter =
+        args.getExtraJson()
+            .map(StringWithMacros::getMacros)
+            .map(RichStream::from)
+            .map(JsLibraryDescription::getMacroTargets)
+            .map(macroTargets -> isWorker.or(macroTargets::contains))
+            .orElse(isWorker);
+    ImmutableSortedSet<BuildRule> workerAndMacrosExtraDeps =
+        params
+            .getExtraDeps()
+            .get()
+            .stream()
+            .filter(x -> extraDepsFilter.test(x.getBuildTarget()))
+            .collect(ImmutableSortedSet.toImmutableSortedSet(Ordering.natural()));
     BuildRuleParams baseParams =
-        JsUtil.paramsWithDeps(params, graphBuilder.getRule(args.getWorker()));
+        params.withoutDeclaredDeps().withExtraDeps(workerAndMacrosExtraDeps);
 
     if (file.isPresent()) {
       return buildTarget.getFlavors().contains(JsFlavors.RELEASE)
@@ -141,19 +163,17 @@ public BuildRule createBuildRule(
           .setSources(args.getSrcs())
           .build(projectFilesystem, worker);
     } else {
-      Stream<BuildTarget> deps = args.getDeps().stream();
-      if (args.getDepsQuery().isPresent()) {
-        // We allow the `deps_query` to contain different kinds of build targets, but filter out
-        // all targets that don't refer to a JsLibrary rule.
-        // That prevents users from having to wrap every query into "kind(js_library, ...)".
-        Stream<BuildTarget> jsLibraryTargetsInQuery =
-            args.getDepsQuery()
-                .get()
-                .getResolvedQuery()
-                .stream()
-                .filter(target -> JsUtil.isJsLibraryTarget(target, context.getTargetGraph()));
-        deps = Stream.concat(deps, jsLibraryTargetsInQuery);
-      }
+      // We allow the `deps_query` to contain different kinds of build targets, but filter out
+      // all targets that don't refer to a JsLibrary rule.
+      // That prevents users from having to wrap every query into "kind(js_library, ...)".
+      Stream<BuildTarget> queryDeps =
+          args.getDepsQuery()
+              .map(Query::getResolvedQuery)
+              .orElseGet(ImmutableSortedSet::of)
+              .stream()
+              .filter(target -> JsUtil.isJsLibraryTarget(target, context.getTargetGraph()));
+      Stream<BuildTarget> declaredDeps = args.getDeps().stream();
+      Stream<BuildTarget> deps = Stream.concat(declaredDeps, queryDeps);
       return new LibraryBuilder(context.getTargetGraph(), graphBuilder, buildTarget, baseParams)
           .setLibraryDependencies(deps)
           .build(projectFilesystem, worker);
@@ -405,6 +425,14 @@ private static BuildTarget withFileFlavorOnly(BuildTarget target) {
     return builder.build();
   }
 
+  private static ImmutableSet<BuildTarget> getMacroTargets(RichStream<MacroContainer> containers) {
+    return containers
+        .map(MacroContainer::getMacro)
+        .filter(BuildTargetMacro.class)
+        .map(BuildTargetMacro::getTarget)
+        .collect(ImmutableSet.toImmutableSet());
+  }
+
   private static Path changePathPrefix(
       SourcePath sourcePath,
       String basePath,

test/com/facebook/buck/js/JsLibraryBuilder.java

@@ -20,6 +20,7 @@
 import com.facebook.buck.core.model.targetgraph.AbstractNodeBuilder;
 import com.facebook.buck.core.sourcepath.SourcePath;
 import com.facebook.buck.io.filesystem.ProjectFilesystem;
+import com.facebook.buck.rules.macros.StringWithMacros;
 import com.facebook.buck.rules.query.Query;
 import com.facebook.buck.test.selectors.Nullable;
 import com.facebook.buck.util.types.Either;
@@ -47,6 +48,11 @@ JsLibraryBuilder setDepsQuery(@Nullable Query query) {
     return this;
   }
 
+  JsLibraryBuilder setExtraJson(Optional<StringWithMacros> extraJson) {
+    getArgForPopulating().setExtraJson(extraJson);
+    return this;
+  }
+
   JsLibraryBuilder setSrcs(ImmutableSet<Either<SourcePath, Pair<SourcePath, String>>> srcs) {
     getArgForPopulating().setSrcs(srcs);
     return this;

test/com/facebook/buck/js/JsLibraryDescriptionTest.java

@@ -40,6 +40,7 @@
 import com.facebook.buck.js.JsLibrary.Files;
 import com.facebook.buck.model.BuildTargetFactory;
 import com.facebook.buck.rules.FakeSourcePath;
+import com.facebook.buck.rules.macros.LocationMacro;
 import com.facebook.buck.rules.query.Query;
 import com.facebook.buck.util.RichStream;
 import com.facebook.buck.util.types.Pair;
@@ -369,7 +370,10 @@ public void supportsDepsQuery() {
             .appleLibraryWithDeps(l, a, c)
             .bundleWithDeps(x, l)
             .bundleWithDeps(BuildTargetFactory.newInstance("//query-deps:bundle"))
-            .library(target, Query.of(String.format("deps(%s)", x)))
+            .library(
+                target,
+                Query.of(String.format("deps(%s)", x)),
+                FakeSourcePath.of("arbitrary/source"))
             .build();
 
     TargetNode<?, ?> node = scenario.targetGraph.get(target);
@@ -386,6 +390,10 @@ public void supportsDepsQuery() {
         lib.getLibraryDependencies());
 
     assertThat(deps, everyItem(not(in(internalFileRule(scenario.graphBuilder).getBuildDeps()))));
+    findJsFileRules(scenario.graphBuilder)
+        .peek(jsFile -> assertThat(deps, everyItem(not(in(jsFile.getBuildDeps())))))
+        .findAny()
+        .orElseThrow(() -> new IllegalStateException("No JsFile dependencies found for " + target));
   }
 
   @Test
@@ -428,6 +436,30 @@ public void libraryRulesDoesNotDependOnGeneratedSources() {
     assertThat(generatedSourcesRules, everyItem(not(in(libRule.getBuildDeps()))));
   }
 
+  @Test
+  public void locationMacrosInExtraJsonAddBuildDeps() {
+    BuildTarget referencedTarget = BuildTargetFactory.newInstance("//:ref");
+    JsTestScenario scenario =
+        scenarioBuilder
+            .arbitraryRule(referencedTarget)
+            .library(
+                target,
+                ImmutableList.of(FakeSourcePath.of("a/file"), FakeSourcePath.of("another/file")),
+                "[\"%s\"]",
+                LocationMacro.of(referencedTarget))
+            .build();
+
+    BuildRule referenced = scenario.graphBuilder.getRule(referencedTarget);
+
+    assertThat(referenced, in(scenario.graphBuilder.getRule(target).getBuildDeps()));
+
+    RichStream<JsFile> jsFileRules = findJsFileRules(scenario.graphBuilder);
+    jsFileRules
+        .peek(jsFile -> assertThat(referenced, in(jsFile.getBuildDeps())))
+        .findAny()
+        .orElseThrow(() -> new IllegalStateException("No JsFile dependencies found for " + target));
+  }
+
   private JsTestScenario buildScenario(String basePath, SourcePath source) {
     return scenarioBuilder.library(target, basePath, source).build();
   }

test/com/facebook/buck/js/JsTestScenario.java

@@ -27,6 +27,9 @@
 import com.facebook.buck.io.filesystem.ProjectFilesystem;
 import com.facebook.buck.model.BuildTargetFactory;
 import com.facebook.buck.parser.exceptions.NoSuchBuildTargetException;
+import com.facebook.buck.rules.macros.Macro;
+import com.facebook.buck.rules.macros.StringWithMacros;
+import com.facebook.buck.rules.macros.StringWithMacrosUtils;
 import com.facebook.buck.rules.query.Query;
 import com.facebook.buck.shell.ExportFileBuilder;
 import com.facebook.buck.shell.FakeWorkerBuilder;
@@ -38,6 +41,7 @@
 import com.google.common.collect.Ordering;
 import java.util.Collection;
 import java.util.LinkedHashSet;
+import java.util.Optional;
 import java.util.Set;
 import java.util.function.Function;
 import java.util.stream.Stream;
@@ -119,14 +123,31 @@ public Builder bundle(BuildTarget target, ImmutableSortedSet<BuildTarget> deps)
     }
 
     public Builder library(BuildTarget target, BuildTarget... libraryDependencies) {
-      addLibrary(target, null, Stream.of(libraryDependencies), null, Stream.of());
+      addLibrary(target, null, Stream.of(libraryDependencies), null, Stream.of(), null);
       return this;
     }
 
     public Builder library(
         BuildTarget target, Query libraryDependenciesQuery, BuildTarget... libraryDependencies) {
       addLibrary(
-          target, null, Stream.of(libraryDependencies), libraryDependenciesQuery, Stream.of());
+          target,
+          null,
+          Stream.of(libraryDependencies),
+          libraryDependenciesQuery,
+          Stream.of(),
+          null);
+      return this;
+    }
+
+    public Builder library(
+        BuildTarget target, Query libraryDependenciesQuery, SourcePath... sources) {
+      addLibrary(
+          target,
+          null,
+          Stream.of(),
+          libraryDependenciesQuery,
+          Stream.of(sources).map(Either::ofLeft),
+          null);
       return this;
     }
 
@@ -136,24 +157,38 @@ public Builder library(BuildTarget target, SourcePath first, SourcePath... sourc
           null,
           Stream.of(),
           null,
-          Stream.concat(Stream.of(first), Stream.of(sources)).map(Either::ofLeft));
+          Stream.concat(Stream.of(first), Stream.of(sources)).map(Either::ofLeft),
+          null);
       return this;
     }
 
     public Builder library(BuildTarget target, String basePath, SourcePath... sources) {
-      addLibrary(target, basePath, Stream.of(), null, Stream.of(sources).map(Either::ofLeft));
+      addLibrary(target, basePath, Stream.of(), null, Stream.of(sources).map(Either::ofLeft), null);
       return this;
     }
 
     public Builder library(
         BuildTarget target, String basePath, Pair<SourcePath, String>... sources) {
-      addLibrary(target, basePath, Stream.of(), null, Stream.of(sources).map(Either::ofRight));
+      addLibrary(
+          target, basePath, Stream.of(), null, Stream.of(sources).map(Either::ofRight), null);
       return this;
     }
 
     public Builder library(
         BuildTarget target, Collection<BuildTarget> libDeps, Collection<SourcePath> sources) {
-      addLibrary(target, null, libDeps.stream(), null, sources.stream().map(Either::ofLeft));
+      addLibrary(target, null, libDeps.stream(), null, sources.stream().map(Either::ofLeft), null);
+      return this;
+    }
+
+    public Builder library(
+        BuildTarget target, Collection<SourcePath> sources, String macroFormat, Macro... macros) {
+      addLibrary(
+          target,
+          null,
+          Stream.of(),
+          null,
+          sources.stream().map(Either::ofLeft),
+          StringWithMacrosUtils.format(macroFormat, macros));
       return this;
     }
 
@@ -167,13 +202,15 @@ private void addLibrary(
         @Nullable String basePath,
         Stream<BuildTarget> libraries,
         @Nullable Query libraryDependenciesQuery,
-        Stream<Either<SourcePath, Pair<SourcePath, String>>> sources) {
+        Stream<Either<SourcePath, Pair<SourcePath, String>>> sources,
+        @Nullable StringWithMacros extraJson) {
       nodes.add(
           new JsLibraryBuilder(target, filesystem)
               .setBasePath(basePath)
               .setDeps(
                   libraries.collect(ImmutableSortedSet.toImmutableSortedSet(Ordering.natural())))
               .setDepsQuery(libraryDependenciesQuery)
+              .setExtraJson(Optional.ofNullable(extraJson))
               .setSrcs(sources.collect(ImmutableSet.toImmutableSet()))
               .setWorker(workerTarget)
               .build());