RELNOTES: Allow private headers in the srcs attribute. Add a warning if a file is in both srcs and hdrs.

This uses a very restrictive definition of private headers. They may only imported by sources in the same target and other private headers. They are not transitively available to dependers, even indirectly, though this may change in the future.

--
MOS_MIGRATED_REVID=101028706

7 files changed, 122 insertions, 31 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationArtifacts.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationArtifacts.java
index 7d1377b5a5..5c8c5956af 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationArtifacts.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationArtifacts.java
@@ -27,8 +27,11 @@ import com.google.devtools.build.lib.actions.Artifact;
  */
 final class CompilationArtifacts {
   static class Builder {
+    // TODO(bazel-team): Should these be sets instead of just iterables?
     private Iterable<Artifact> srcs = ImmutableList.of();
     private Iterable<Artifact> nonArcSrcs = ImmutableList.of();
+    private Iterable<Artifact> additionalHdrs = ImmutableList.of();
+    private Iterable<Artifact> privateHdrs = ImmutableList.of();
     private Optional<Artifact> pchFile;
     private IntermediateArtifacts intermediateArtifacts;
 
@@ -42,6 +45,25 @@ final class CompilationArtifacts {
       return this;
     }
 
+    /**
+     * Adds header artifacts that should be directly accessible to dependers, but aren't specified
+     * in the hdrs attribute. {@code additionalHdrs} should not be a {@link NestedSet}, as it will
+     * be flattened when added.
+     */
+    Builder addAdditionalHdrs(Iterable<Artifact> additionalHdrs) {
+      this.additionalHdrs = Iterables.concat(this.additionalHdrs, additionalHdrs);
+      return this;
+    }
+
+    /**
+     * Adds header artifacts that should not be directly accessible to dependers.
+     * {@code privateHdrs} should not be a {@link NestedSet}, as it will be flattened when added.
+     */
+    Builder addPrivateHdrs(Iterable<Artifact> privateHdrs) {
+      this.privateHdrs = Iterables.concat(this.privateHdrs, privateHdrs);
+      return this;
+    }
+
     Builder setPchFile(Optional<Artifact> pchFile) {
       Preconditions.checkState(this.pchFile == null,
           "pchFile is already set to: %s", this.pchFile);
@@ -61,23 +83,30 @@ final class CompilationArtifacts {
       if (!Iterables.isEmpty(srcs) || !Iterables.isEmpty(nonArcSrcs)) {
         archive = Optional.of(intermediateArtifacts.archive());
       }
-      return new CompilationArtifacts(srcs, nonArcSrcs, archive, pchFile);
+      return new CompilationArtifacts(
+          srcs, nonArcSrcs, additionalHdrs, privateHdrs, archive, pchFile);
     }
   }
 
   private final Iterable<Artifact> srcs;
   private final Iterable<Artifact> nonArcSrcs;
   private final Optional<Artifact> archive;
+  private final Iterable<Artifact> additionalHdrs;
+  private final Iterable<Artifact> privateHdrs;
   private final Optional<Artifact> pchFile;
   private final boolean hasSwiftSources;
 
   private CompilationArtifacts(
       Iterable<Artifact> srcs,
       Iterable<Artifact> nonArcSrcs,
+      Iterable<Artifact> additionalHdrs,
+      Iterable<Artifact> privateHdrs,
       Optional<Artifact> archive,
       Optional<Artifact> pchFile) {
     this.srcs = Preconditions.checkNotNull(srcs);
     this.nonArcSrcs = Preconditions.checkNotNull(nonArcSrcs);
+    this.additionalHdrs = Preconditions.checkNotNull(additionalHdrs);
+    this.privateHdrs = Preconditions.checkNotNull(privateHdrs);
     this.archive = Preconditions.checkNotNull(archive);
     this.pchFile = Preconditions.checkNotNull(pchFile);
     this.hasSwiftSources = Iterables.any(this.srcs, new Predicate<Artifact>() {
@@ -96,6 +125,21 @@ final class CompilationArtifacts {
     return nonArcSrcs;
   }
 
+  /**
+   * Returns the public headers that aren't included in the hdrs attribute.
+   */
+  public Iterable<Artifact> getAdditionalHdrs() {
+    return additionalHdrs;
+  }
+
+  /**
+   * Returns the private headers from the srcs attribute, which may by imported by any source or
+   * header in this target, but not by sources or headers of dependers.
+   */
+  public Iterable<Artifact> getPrivateHdrs() {
+    return privateHdrs;
+  }
+
   public Optional<Artifact> getArchive() {
     return archive;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
index c76f35baa2..b26a702fa5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
@@ -31,7 +31,9 @@ import static com.google.devtools.build.lib.rules.objc.ObjcProvider.SDK_FRAMEWOR
 import static com.google.devtools.build.lib.rules.objc.ObjcProvider.WEAK_SDK_FRAMEWORK;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.CLANG;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.CLANG_PLUSPLUS;
+import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.COMPILABLE_SRCS_TYPE;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.DSYMUTIL;
+import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.HEADERS;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.NON_ARC_SRCS_TYPE;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.SRCS_TYPE;
 import static com.google.devtools.build.lib.rules.objc.ObjcRuleClasses.STRIP;
@@ -44,8 +46,10 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.ActionConstructionContext;
@@ -55,6 +59,7 @@ import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs;
 import com.google.devtools.build.lib.rules.java.J2ObjcConfiguration;
 import com.google.devtools.build.lib.rules.objc.ObjcCommon.CompilationAttributes;
@@ -63,7 +68,9 @@ import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 /**
  * Support for rules that compile sources. Provides ways to determine files that should be output,
@@ -72,7 +79,7 @@ import java.util.List;
  *
  * <p>Methods on this class can be called in any order without impacting the result.
  */
-final class CompilationSupport {
+public final class CompilationSupport {
 
   @VisibleForTesting
   static final String ABSOLUTE_INCLUDES_PATH_FORMAT =
@@ -98,18 +105,23 @@ final class CompilationSupport {
     }
   }
 
+  @VisibleForTesting
+  static final String FILE_IN_SRCS_AND_HDRS_WARNING_FORMAT =
+      "File '%s' is in both srcs and hdrs.";
+
   /**
    * Returns information about the given rule's compilation artifacts.
    */
   // TODO(bazel-team): Remove this information from ObjcCommon and move it internal to this class.
   static CompilationArtifacts compilationArtifacts(RuleContext ruleContext) {
+    PrerequisiteArtifacts srcs = ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET)
+        .errorsForNonMatching(SRCS_TYPE);
     return new CompilationArtifacts.Builder()
-        .addSrcs(ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET)
-            .errorsForNonMatching(SRCS_TYPE)
-            .list())
+        .addSrcs(srcs.filter(COMPILABLE_SRCS_TYPE).list())
         .addNonArcSrcs(ruleContext.getPrerequisiteArtifacts("non_arc_srcs", Mode.TARGET)
             .errorsForNonMatching(NON_ARC_SRCS_TYPE)
             .list())
+        .addPrivateHdrs(srcs.filter(HEADERS).list())
         .setIntermediateArtifacts(ObjcRuleClasses.intermediateArtifacts(ruleContext))
         .setPchFile(Optional.fromNullable(ruleContext.getPrerequisiteArtifact("pch", Mode.TARGET)))
         .build();
@@ -121,7 +133,7 @@ final class CompilationSupport {
   /**
    * Creates a new compilation support for the given rule.
    */
-  CompilationSupport(RuleContext ruleContext) {
+  public CompilationSupport(RuleContext ruleContext) {
     this.ruleContext = ruleContext;
     this.attributes = new CompilationAttributes(ruleContext);
   }
@@ -216,8 +228,7 @@ final class CompilationSupport {
 
     commandLine
         .add(IosSdkCommands.compileFlagsForClang(objcConfiguration))
-        .add(IosSdkCommands.commonLinkAndCompileFlagsForClang(
-            objcProvider, objcConfiguration))
+        .add(IosSdkCommands.commonLinkAndCompileFlagsForClang(objcProvider, objcConfiguration))
         .add(objcConfiguration.getCoptsForCompilationMode())
         .addBeforeEachPath(
             "-iquote", ObjcCommon.userHeaderSearchPaths(ruleContext.getConfiguration()))
@@ -232,6 +243,7 @@ final class CompilationSupport {
         .addExecPath("-c", sourceFile)
         .addExecPath("-o", objFile);
 
+    // TODO(bazel-team): Remote private headers from inputs once they're added to the provider.
     ruleContext.registerAction(ObjcRuleClasses.spawnOnDarwinActionBuilder()
         .setMnemonic("ObjcCompile")
         .setExecutable(CLANG)
@@ -241,6 +253,7 @@ final class CompilationSupport {
         .addOutput(objFile)
         .addOutputs(gcnoFiles.build())
         .addTransitiveInputs(objcProvider.get(HEADER))
+        .addInputs(compilationArtifacts.getPrivateHdrs())
         .addTransitiveInputs(objcProvider.get(FRAMEWORK_FILE))
         .addInputs(compilationArtifacts.getPchFile().asSet())
         .build(ruleContext));
@@ -732,6 +745,17 @@ final class CompilationSupport {
           "includes", String.format(ABSOLUTE_INCLUDES_PATH_FORMAT, absoluteInclude));
     }
 
+    // Check for overlap between srcs and hdrs.
+    if (ruleContext.attributes().has("srcs", Type.LABEL_LIST)) {
+      Set<Artifact> hdrsSet = new HashSet<>(attributes.hdrs());
+      Set<Artifact> srcsSet =
+          new HashSet<>(ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET).list());
+      for (Artifact header : Sets.intersection(hdrsSet, srcsSet)) {
+        String path = header.getRootRelativePath().toString();
+        ruleContext.attributeWarning(
+            "srcs", String.format(FILE_IN_SRCS_AND_HDRS_WARNING_FORMAT, path));
+      }
+    }
     return this;
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java
index 030164eaf4..3a2ce46dd0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java
@@ -93,6 +93,10 @@ public final class ObjcCommon {
     }
 
     ImmutableList<Artifact> hdrs() {
+      // Some rules may compile but not have the "hdrs" attribute.
+      if (!ruleContext.attributes().has("hdrs", Type.LABEL_LIST)) {
+        return ImmutableList.of();
+      }
       return ImmutableList.copyOf(CcCommon.getHeaders(ruleContext));
     }
 
@@ -241,7 +245,6 @@ public final class ObjcCommon {
     private Iterable<ObjcProvider> directDepObjcProviders = ImmutableList.of();
     private Iterable<String> defines = ImmutableList.of();
     private Iterable<PathFragment> userHeaderSearchPaths = ImmutableList.of();
-    private Iterable<Artifact> headers = ImmutableList.of();
     private IntermediateArtifacts intermediateArtifacts;
     private boolean alwayslink;
     private Iterable<Artifact> extraImportLibraries = ImmutableList.of();
@@ -326,11 +329,6 @@ public final class ObjcCommon {
       return this;
     }
 
-    public Builder addHeaders(Iterable<Artifact> headers) {
-      this.headers = Iterables.concat(this.headers, headers);
-      return this;
-    }
-
     Builder setIntermediateArtifacts(IntermediateArtifacts intermediateArtifacts) {
       this.intermediateArtifacts = intermediateArtifacts;
       return this;
@@ -397,7 +395,6 @@ public final class ObjcCommon {
           .addAll(FRAMEWORK_DIR, uniqueContainers(frameworkImports, FRAMEWORK_CONTAINER_TYPE))
           .addAll(INCLUDE, userHeaderSearchPaths)
           .addAll(DEFINE, defines)
-          .addAll(HEADER, headers)
           .addTransitiveAndPropagate(depObjcProviders)
           .addTransitiveWithoutPropagating(directDepObjcProviders);
 
@@ -453,8 +450,12 @@ public final class ObjcCommon {
       for (CompilationArtifacts artifacts : compilationArtifacts.asSet()) {
         Iterable<Artifact> allSources =
             Iterables.concat(artifacts.getSrcs(), artifacts.getNonArcSrcs());
-        objcProvider.addAll(LIBRARY, artifacts.getArchive().asSet());
-        objcProvider.addAll(SOURCE, allSources);
+        // TODO(bazel-team): Add private headers to the provider when we have module maps to enforce
+        // them.
+        objcProvider
+            .addAll(HEADER, artifacts.getAdditionalHdrs())
+            .addAll(LIBRARY, artifacts.getArchive().asSet())
+            .addAll(SOURCE, allSources);
         BuildConfiguration configuration = context.getConfiguration();
         RegexFilter filter = configuration.getInstrumentationFilter();
         if (configuration.isCodeCoverageEnabled()
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java
index 2510fcb80b..ce1709a5ab 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibrary.java
@@ -153,6 +153,8 @@ public class ObjcProtoLibrary implements RuleConfiguredTargetFactory {
         .addNonArcSrcs(protoGeneratedSources)
         .setIntermediateArtifacts(intermediateArtifacts)
         .setPchFile(Optional.<Artifact>absent())
+        .addAdditionalHdrs(protoGeneratedHeaders)
+        .addAdditionalHdrs(protoGeneratedSources)
         .build();
 
     ImmutableSet.Builder<PathFragment> searchPathEntriesBuilder =
@@ -175,8 +177,6 @@ public class ObjcProtoLibrary implements RuleConfiguredTargetFactory {
         .addDepObjcProviders(ruleContext.getPrerequisites(
             ObjcProtoLibraryRule.LIBPROTOBUF_ATTR, Mode.TARGET, ObjcProvider.class))
         .setIntermediateArtifacts(intermediateArtifacts)
-        .addHeaders(protoGeneratedHeaders)
-        .addHeaders(protoGeneratedSources)
         .build();
 
     NestedSetBuilder<Artifact> filesToBuild = NestedSetBuilder.<Artifact>stableOrder()
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
index 413ed2d5b2..24f33e479c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcProvider.java
@@ -76,6 +76,9 @@ public final class ObjcProvider implements TransitiveInfoProvider {
    */
   public static final Key<String> FORCE_LOAD_FOR_XCODEGEN = new Key<>(LINK_ORDER);
 
+  /**
+   * Contains all header files. These may be either public or private headers.
+   */
   public static final Key<Artifact> HEADER = new Key<>(STABLE_ORDER);
 
   /**
@@ -90,7 +93,7 @@ public final class ObjcProvider implements TransitiveInfoProvider {
 
   /**
    * Contains all .gcno files one for every source file if in coverage mode.
-   * It contains information to reconstruct the basic block graphs and assign source line numbers 
+   * It contains information to reconstruct the basic block graphs and assign source line numbers
    * to blocks.
    */
   public static final Key<Artifact> GCNO = new Key<>(STABLE_ORDER);
@@ -334,7 +337,7 @@ public final class ObjcProvider implements TransitiveInfoProvider {
     /**
      * Add elements from providers, but don't propagate them to any dependers on this ObjcProvider.
      * These elements will be exposed to {@link #get(Key)} calls, but not to any ObjcProviders
-     * which add this provider to themself.
+     * which add this provider to themselves.
      */
     public Builder addTransitiveWithoutPropagating(Iterable<ObjcProvider> providers) {
       for (ObjcProvider provider : providers) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java
index ad32d2e36b..ec3f27950b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java
@@ -317,10 +317,24 @@ public class ObjcRuleClasses {
   private static final FileType NON_CPP_SOURCES = FileType.of(".m", ".c", ".s", ".asm");
 
   static final FileType PREPROCESSED_ASSEMBLY_SOURCES = FileType.of(".S");
-  
+
   static final FileType SWIFT_SOURCES = FileType.of(".swift");
 
+  /**
+   * Header files, which are not compiled directly, but may be included/imported from source files.
+   */
+  static final FileType HEADERS = FileType.of(".h", ".inc");
+
+  /**
+   * Files allowed in the srcs attribute. This includes private headers.
+   */
   static final FileTypeSet SRCS_TYPE = FileTypeSet.of(NON_CPP_SOURCES, CPP_SOURCES,
+      PREPROCESSED_ASSEMBLY_SOURCES, SWIFT_SOURCES, HEADERS);
+
+  /**
+   * Files that should actually be compiled.
+   */
+  static final FileTypeSet COMPILABLE_SRCS_TYPE = FileTypeSet.of(NON_CPP_SOURCES, CPP_SOURCES,
       PREPROCESSED_ASSEMBLY_SOURCES, SWIFT_SOURCES);
 
   static final FileTypeSet NON_ARC_SRCS_TYPE = FileTypeSet.of(FileType.of(".m", ".mm"));
@@ -514,8 +528,9 @@ public class ObjcRuleClasses {
     public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
       return builder
           /* <!-- #BLAZE_RULE($objc_compile_dependency_rule).ATTRIBUTE(hdrs) -->
-          The list of Objective-C files that are included as headers by source
-          files in this rule or by users of this library.
+          The list of C, C++, Objective-C, and Objective-C++ files that are
+          included as headers by source files in this rule or by users of this
+          library.
           ${SYNOPSIS}
           <!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
           .add(attr("hdrs", LABEL_LIST)
@@ -579,12 +594,14 @@ public class ObjcRuleClasses {
     public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
       return builder
           /* <!-- #BLAZE_RULE($objc_compiling_rule).ATTRIBUTE(srcs) -->
-          The list of C, C++, Objective-C, and Objective-C++ files that are
-          processed to create the library target.
+          The list of C, C++, Objective-C, and Objective-C++ source and header
+          files that are processed to create the library target.
           ${SYNOPSIS}
-          These are your checked-in source files, plus any generated files.
-          These are compiled into .o files with Clang, so headers should not go
-          here (see the hdrs attribute).
+          These are your checked-in files, plus any generated files.
+          Source files are compiled into .o files with Clang. Header files
+          may be included/imported by any source or header in the srcs attribute
+          of this target, but not by headers in hdrs or any targets that depend
+          on this rule.
           <!-- #END_BLAZE_RULE.ATTRIBUTE -->*/
           .add(attr("srcs", LABEL_LIST)
               .direct_compile_time_input()
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/XcodeProvider.java b/src/main/java/com/google/devtools/build/lib/rules/objc/XcodeProvider.java
index 96a124c290..3e77023af2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/XcodeProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/XcodeProvider.java
@@ -526,6 +526,7 @@ public final class XcodeProvider implements TransitiveInfoProvider {
             .setName(label.getName())
             .setLabel(xcodeTargetName(label))
             .setProductType(productType.getIdentifier())
+            .addSupportFile(buildFilePath)
             .addAllImportedLibrary(Artifact.toExecPaths(objcProvider.get(IMPORTED_LIBRARY)))
             .addAllUserHeaderSearchPath(userHeaderSearchPaths)
             .addAllHeaderSearchPath(headerSearchPaths)
@@ -552,8 +553,7 @@ public final class XcodeProvider implements TransitiveInfoProvider {
             .addAllGeneralResourceFile(
                 Artifact.toExecPaths(objcProvider.get(GENERAL_RESOURCE_FILE)))
             .addAllGeneralResourceFile(
-                PathFragment.safePathStrings(objcProvider.get(GENERAL_RESOURCE_DIR)))
-            .addSupportFile(buildFilePath);
+                PathFragment.safePathStrings(objcProvider.get(GENERAL_RESOURCE_DIR)));
 
     if (CAN_LINK_PRODUCT_TYPES.contains(productType)) {
       // For builds with --ios_multi_cpus set, we may have several copies of some XCodeProviders
@@ -605,6 +605,8 @@ public final class XcodeProvider implements TransitiveInfoProvider {
     for (CompilationArtifacts artifacts : compilationArtifacts.asSet()) {
       targetControl
           .addAllSourceFile(Artifact.toExecPaths(artifacts.getSrcs()))
+          .addAllSupportFile(Artifact.toExecPaths(artifacts.getAdditionalHdrs()))
+          .addAllSupportFile(Artifact.toExecPaths(artifacts.getPrivateHdrs()))
           .addAllNonArcSourceFile(Artifact.toExecPaths(artifacts.getNonArcSrcs()));
 
       for (Artifact pchFile : artifacts.getPchFile().asSet()) {