Rollback of commit 16444baf21780c4bff00b9736b268e97157e6bc2.

*** Reason for rollback ***

Breaks targets. See []

Manually verified this unbreaks the target mentioned in the bug.

*** Original change description ***

Do not compile source files mentioned in 'hdrs'.

Currently for a library:
  cc_library(name='a', hdrs=['a.cc'])
we compile a.cc into a.pic.o and link that into its reverse dependencies.

With this change, a .cc file in hdrs will be treated like a .inc file or
a file in textual_hdrs.

--
MOS_MIGRATED_REVID=113568452

7 files changed, 93 insertions, 158 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index 3afa030e1c..ff5feff2bc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -45,6 +45,7 @@ import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.OsUtils;
 import com.google.devtools.build.lib.util.Pair;
@@ -152,7 +153,7 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
       boolean useTestOnlyFlags) throws InterruptedException {
     ruleContext.checkSrcsSamePackage(true);
     FeatureConfiguration featureConfiguration = CcCommon.configureFeatures(ruleContext);
-    CcCommon common = new CcCommon(ruleContext);
+    CcCommon common = new CcCommon(ruleContext, featureConfiguration);
     CppConfiguration cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
     PrecompiledFiles precompiledFiles = new PrecompiledFiles(ruleContext);
 
@@ -161,11 +162,16 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     List<String> linkopts = common.getLinkopts();
     LinkStaticness linkStaticness = getLinkStaticness(ruleContext, linkopts, cppConfiguration);
 
+    ImmutableList<Pair<Artifact, Label>> cAndCppSources = common.getCAndCppSources();
     CcLibraryHelper helper =
         new CcLibraryHelper(ruleContext, semantics, featureConfiguration)
             .fromCommon(common)
-            .addSources(common.getSources())
+            .addSources(cAndCppSources)
             .addDeps(ImmutableList.of(CppHelper.mallocForTarget(ruleContext)))
+            .addPrivateHeaders(
+                FileType.filter(
+                    ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET).list(),
+                    CppFileTypes.CPP_HEADER))
             .setFake(fake)
             .setLinkType(linkType)
             .addPrecompiledFiles(precompiledFiles);
@@ -278,16 +284,9 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
     // logical since all symlinked libraries will be linked anyway and would
     // not require manual loading but if we do, then we would need to collect
     // their names and use a different constructor below.
-    Runfiles runfiles =
-        collectRunfiles(
-            ruleContext,
-            linkingOutputs,
-            cppCompilationContext,
-            linkStaticness,
-            filesToBuild,
-            fakeLinkerInputs,
-            fake,
-            helper.getCompilationUnitSources());
+    Runfiles runfiles = collectRunfiles(
+        ruleContext, linkingOutputs, cppCompilationContext, linkStaticness, filesToBuild,
+        fakeLinkerInputs, fake, cAndCppSources);
     RunfilesSupport runfilesSupport = RunfilesSupport.withExecutable(
         ruleContext, runfiles, executable, ruleContext.getConfiguration().buildRunfiles());
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index 2ede292346..1825742e82 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -41,6 +41,7 @@ import com.google.devtools.build.lib.rules.test.InstrumentedFilesProviderImpl;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileType;
+import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -59,6 +60,13 @@ public final class CcCommon {
 
   private static final String NO_COPTS_ATTRIBUTE = "nocopts";
 
+  private static final FileTypeSet SOURCE_TYPES = FileTypeSet.of(
+      CppFileTypes.CPP_SOURCE,
+      CppFileTypes.CPP_HEADER,
+      CppFileTypes.C_SOURCE,
+      CppFileTypes.ASSEMBLER,
+      CppFileTypes.ASSEMBLER_WITH_C_PREPROCESSOR);
+
   /**
    * Collects all metadata files generated by C++ compilation actions that output the .o files
    * on the input.
@@ -89,11 +97,15 @@ public final class CcCommon {
   /** C++ configuration */
   private final CppConfiguration cppConfiguration;
   
+  /** Active toolchain features. */
+  private final FeatureConfiguration featureConfiguration;
+
   private final RuleContext ruleContext;
 
-  public CcCommon(RuleContext ruleContext) {
+  public CcCommon(RuleContext ruleContext, FeatureConfiguration featureConfiguration) {
     this.ruleContext = ruleContext;
     this.cppConfiguration = ruleContext.getFragment(CppConfiguration.class);
+    this.featureConfiguration = featureConfiguration;
   }
 
   /**
@@ -189,23 +201,39 @@ public final class CcCommon {
     return new TransitiveLipoInfoProvider(scannableBuilder.build());
   }
 
+  private boolean shouldProcessHeaders() {
+    return featureConfiguration.isEnabled(CppRuleClasses.PREPROCESS_HEADERS)
+        || featureConfiguration.isEnabled(CppRuleClasses.PARSE_HEADERS);
+  }
+
   /**
    * Returns a list of ({@link Artifact}, {@link Label}) pairs. Each pair represents an input
    * source file and the label of the rule that generates it (or the label of the source file
-   * itself if it is an input file).
+   * itself if it is an input file)
    */
-  List<Pair<Artifact, Label>> getSources() {
+  ImmutableList<Pair<Artifact, Label>> getCAndCppSources() {
     Map<Artifact, Label> map = Maps.newLinkedHashMap();
     Iterable<FileProvider> providers =
         ruleContext.getPrerequisites("srcs", Mode.TARGET, FileProvider.class);
+    // TODO(bazel-team): Move header processing logic down in the stack (to CcLibraryHelper or
+    // such).
+    boolean processHeaders = shouldProcessHeaders();
+    if (processHeaders && hasAttribute("hdrs", BuildType.LABEL_LIST)) {
+      providers = Iterables.concat(providers,
+          ruleContext.getPrerequisites("hdrs", Mode.TARGET, FileProvider.class));
+    }
     for (FileProvider provider : providers) {
-      for (Artifact artifact : provider.getFilesToBuild()) {
+      for (Artifact artifact : FileType.filter(provider.getFilesToBuild(), SOURCE_TYPES)) {
+        boolean isHeader = CppFileTypes.CPP_HEADER.matches(artifact.getExecPath());
+        if ((isHeader && !processHeaders)
+            || CppFileTypes.CPP_TEXTUAL_INCLUDE.matches(artifact.getExecPath())) {
+          continue;
+        }
+        Label oldLabel = map.put(artifact, provider.getLabel());
         // TODO(bazel-team): We currently do not warn for duplicate headers with
         // different labels, as that would require cleaning up the code base
         // without significant benefit; we should eventually make this
         // consistent one way or the other.
-        Label oldLabel = map.put(artifact, provider.getLabel());
-        boolean isHeader = CppFileTypes.CPP_HEADER.matches(artifact.getExecPath());
         if (!isHeader && oldLabel != null && !oldLabel.equals(provider.getLabel())) {
           ruleContext.attributeError("srcs", String.format(
               "Artifact '%s' is duplicated (through '%s' and '%s')",
@@ -218,6 +246,7 @@ public final class CcCommon {
     for (Map.Entry<Artifact, Label> entry : map.entrySet()) {
       result.add(Pair.of(entry.getKey(), entry.getValue()));
     }
+
     return result.build();
   }
 
@@ -226,44 +255,21 @@ public final class CcCommon {
    * warnings to the {@link RuleContext} as a side effect, and so should only be called once for any
    * given rule.
    */
-  public static List<Pair<Artifact, Label>> getHeaders(RuleContext ruleContext) {
-    Map<Artifact, Label> map = Maps.newLinkedHashMap();
+  public static List<Artifact> getHeaders(RuleContext ruleContext) {
+    List<Artifact> hdrs = new ArrayList<>();
     for (TransitiveInfoCollection target :
         ruleContext.getPrerequisitesIf("hdrs", Mode.TARGET, FileProvider.class)) {
       FileProvider provider = target.getProvider(FileProvider.class);
       for (Artifact artifact : provider.getFilesToBuild()) {
-        if (CppRuleClasses.DISALLOWED_HDRS_FILES.matches(artifact.getFilename())) {
+        if (!CppRuleClasses.DISALLOWED_HDRS_FILES.matches(artifact.getFilename())) {
+          hdrs.add(artifact);
+        } else {
           ruleContext.attributeWarning("hdrs", "file '" + artifact.getFilename()
               + "' from target '" + target.getLabel() + "' is not allowed in hdrs");
-          continue;
-        }
-        Label oldLabel = map.put(artifact, provider.getLabel());
-        if (oldLabel != null && !oldLabel.equals(provider.getLabel())) {
-          ruleContext.attributeWarning(
-              "hdrs",
-              String.format(
-                  "Artifact '%s' is duplicated (through '%s' and '%s')",
-                  artifact.getExecPathString(),
-                  oldLabel,
-                  provider.getLabel()));
         }
       }
     }
-    
-    ImmutableList.Builder<Pair<Artifact, Label>> result = ImmutableList.builder();
-    for (Map.Entry<Artifact, Label> entry : map.entrySet()) {
-      result.add(Pair.of(entry.getKey(), entry.getValue()));
-    }
-    return result.build();
-  }
-
-  /**
-   * Returns the files from headers and does some sanity checks. Note that this method reports
-   * warnings to the {@link RuleContext} as a side effect, and so should only be called once for any
-   * given rule.
-   */
-  public List<Pair<Artifact, Label>> getHeaders() {
-    return getHeaders(ruleContext);
+    return hdrs;
   }
 
   private static ImmutableList<String> getPackageCopts(RuleContext ruleContext) {
@@ -417,16 +423,12 @@ public final class CcCommon {
    */
   static NestedSet<Artifact> collectCompilationPrerequisites(
       RuleContext ruleContext, CppCompilationContext context) {
-    // TODO(bazel-team): Use context.getCompilationPrerequisites() instead; note that this will
-    // need cleaning up the prerequisites, as the compilation context currently collects them
-    // transitively (to get transitive headers), but source files are not transitive compilation
-    // prerequisites.
+    // TODO(bazel-team): Use context.getCompilationPrerequisites() instead.
     NestedSetBuilder<Artifact> prerequisites = NestedSetBuilder.stableOrder();
     if (ruleContext.attributes().has("srcs", BuildType.LABEL_LIST)) {
-      for (FileProvider provider :
-          ruleContext.getPrerequisites("srcs", Mode.TARGET, FileProvider.class)) {
-        prerequisites.addAll(
-            FileType.filter(provider.getFilesToBuild(), CcLibraryHelper.SOURCE_TYPES));
+      for (FileProvider provider : ruleContext
+          .getPrerequisites("srcs", Mode.TARGET, FileProvider.class)) {
+        prerequisites.addAll(FileType.filter(provider.getFilesToBuild(), SOURCE_TYPES));
       }
     }
     prerequisites.addTransitive(context.getDeclaredIncludeSrcs());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
index 764b7f4441..a8c9f4d827 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -40,6 +40,7 @@ import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
 import com.google.devtools.build.lib.syntax.Type;
+import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
@@ -111,15 +112,16 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
       boolean collectLinkstamp,
       boolean addDynamicRuntimeInputArtifactsToRunfiles) {
     FeatureConfiguration featureConfiguration = CcCommon.configureFeatures(ruleContext);
-    final CcCommon common = new CcCommon(ruleContext);
+    final CcCommon common = new CcCommon(ruleContext, featureConfiguration);
     PrecompiledFiles precompiledFiles = new PrecompiledFiles(ruleContext);
 
     CcLibraryHelper helper =
         new CcLibraryHelper(ruleContext, semantics, featureConfiguration)
             .fromCommon(common)
+
             .addLinkopts(common.getLinkopts())
-            .addSources(common.getSources())
-            .addPublicHeaders(common.getHeaders())
+            .addSources(common.getCAndCppSources())
+            .addPublicHeaders(CcCommon.getHeaders(ruleContext))
             .enableCcNativeLibrariesProvider()
             .enableCompileProviders()
             .enableInterfaceSharedObjects()
@@ -163,6 +165,9 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
     }
 
     if (ruleContext.getRule().isAttrDefined("srcs", BuildType.LABEL_LIST)) {
+      helper.addPrivateHeaders(FileType.filter(
+          ruleContext.getPrerequisiteArtifacts("srcs", Mode.TARGET).list(),
+          CppFileTypes.CPP_HEADER));
       ruleContext.checkSrcsSamePackage(true);
     }
     if (ruleContext.getRule().isAttrDefined("textual_hdrs", BuildType.LABEL_LIST)) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
index 6fd24c9659..200edc4913 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibraryHelper.java
@@ -16,7 +16,6 @@ package com.google.devtools.build.lib.rules.cpp;
 
 import com.google.common.base.Function;
 import com.google.common.base.Predicates;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -40,7 +39,6 @@ import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfig
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.HeadersCheckingMode;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
-import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -69,14 +67,6 @@ import javax.annotation.Nullable;
  * methods.
  */
 public final class CcLibraryHelper {
-  static final FileTypeSet SOURCE_TYPES =
-      FileTypeSet.of(
-          CppFileTypes.CPP_SOURCE,
-          CppFileTypes.CPP_HEADER,
-          CppFileTypes.C_SOURCE,
-          CppFileTypes.ASSEMBLER,
-          CppFileTypes.ASSEMBLER_WITH_C_PREPROCESSOR);
-  
   /** Function for extracting module maps from CppCompilationDependencies. */
   public static final Function<TransitiveInfoCollection, CppModuleMap> CPP_DEPS_TO_MODULES =
     new Function<TransitiveInfoCollection, CppModuleMap>() {
@@ -165,7 +155,7 @@ public final class CcLibraryHelper {
   private final List<Artifact> publicTextualHeaders = new ArrayList<>();
   private final List<Artifact> privateHeaders = new ArrayList<>();
   private final List<PathFragment> additionalExportedHeaders = new ArrayList<>();
-  private final List<Pair<Artifact, Label>> compilationUnitSources = new ArrayList<>();
+  private final List<Pair<Artifact, Label>> sources = new ArrayList<>();
   private final List<Artifact> objectFiles = new ArrayList<>();
   private final List<Artifact> picObjectFiles = new ArrayList<>();
   private final List<String> copts = new ArrayList<>();
@@ -224,39 +214,31 @@ public final class CcLibraryHelper {
   }
 
   /**
-   * Adds {@code headers} as public header files. These files will be made visible to dependent
-   * rules. They may be parsed/preprocessed or compiled into a header module depending on the
-   * configuration.
+   * Add the corresponding files as header files, i.e., these files will not be compiled, but are
+   * made visible as includes to dependent rules.
    */
   public CcLibraryHelper addPublicHeaders(Collection<Artifact> headers) {
-    for (Artifact header : headers) {
-      addHeader(header, ruleContext.getLabel());
-    }
+    this.publicHeaders.addAll(headers);
     return this;
   }
-  
+
   /**
-   * Adds {@code headers} as public header files. These files will be made visible to dependent
-   * rules. They may be parsed/preprocessed or compiled into a header module depending on the
-   * configuration.
+   * Add the corresponding files as public header files, i.e., these files will not be compiled, but
+   * are made visible as includes to dependent rules in module maps.
    */
   public CcLibraryHelper addPublicHeaders(Artifact... headers) {
-    addPublicHeaders(Arrays.asList(headers));
-    return this;
+    return addPublicHeaders(Arrays.asList(headers));
   }
-  
+
   /**
-   * Adds {@code headers} as public header files. These files will be made visible to dependent
-   * rules. They may be parsed/preprocessed or compiled into a header module depending on the
-   * configuration.
+   * Add the corresponding files as private header files, i.e., these files will not be compiled,
+   * but are not made visible as includes to dependent rules in module maps.
    */
-  public CcLibraryHelper addPublicHeaders(Iterable<Pair<Artifact, Label>> headers) {
-    for (Pair<Artifact, Label> header : headers) {
-      addHeader(header.first, header.second);
-    }
+  public CcLibraryHelper addPrivateHeaders(Iterable<Artifact> privateHeaders) {
+    Iterables.addAll(this.privateHeaders, privateHeaders);
     return this;
   }
-  
+
   /**
    * Add the corresponding files as public header files, i.e., these files will not be compiled, but
    * are made visible as includes to dependent rules in module maps.
@@ -283,9 +265,10 @@ public final class CcLibraryHelper {
    * Add the corresponding files as source files. These may also be header files, in which case
    * they will not be compiled, but also not made visible as includes to dependent rules.
    */
+  // TODO(bazel-team): This is inconsistent with the documentation on CppModel.
   public CcLibraryHelper addSources(Collection<Artifact> sources) {
     for (Artifact source : sources) {
-      addSource(source, ruleContext.getLabel());
+      this.sources.add(Pair.of(source, ruleContext.getLabel()));
     }
     return this;
   }
@@ -294,10 +277,9 @@ public final class CcLibraryHelper {
    * Add the corresponding files as source files. These may also be header files, in which case
    * they will not be compiled, but also not made visible as includes to dependent rules.
    */
+  // TODO(bazel-team): This is inconsistent with the documentation on CppModel.
   public CcLibraryHelper addSources(Iterable<Pair<Artifact, Label>> sources) {
-    for (Pair<Artifact, Label> source : sources) {
-      addSource(source.first, source.second);
-    }
+    Iterables.addAll(this.sources, sources);
     return this;
   }
 
@@ -308,50 +290,6 @@ public final class CcLibraryHelper {
   public CcLibraryHelper addSources(Artifact... sources) {
     return addSources(Arrays.asList(sources));
   }
-  
-  /**
-   * Adds a header to {@code publicHeaders} and in case header processing is switched on for the
-   * file type also to compilationUnitSources.
-   */
-  private void addHeader(Artifact header, Label label) {
-    boolean isHeader = CppFileTypes.CPP_HEADER.matches(header.getExecPath());
-    boolean isTextualInclude = CppFileTypes.CPP_TEXTUAL_INCLUDE.matches(header.getExecPath());
-    publicHeaders.add(header);
-    if (isTextualInclude || !isHeader || !shouldProcessHeaders()) {
-      return;
-    }
-    compilationUnitSources.add(Pair.of(header, label));
-  }
-
-  /**
-   * Adds a source to {@code compilationUnitSources} if it is a compiled file type (including
-   * parsed/preprocessed header) and to {@code privateHeaders} if it is a header.
-   */
-  private void addSource(Artifact source, Label label) {
-    boolean isHeader = CppFileTypes.CPP_HEADER.matches(source.getExecPath());
-    boolean isTextualInclude = CppFileTypes.CPP_TEXTUAL_INCLUDE.matches(source.getExecPath());
-    boolean isCompiledSource = SOURCE_TYPES.matches(source.getExecPathString());
-    if (isHeader || isTextualInclude) {
-      privateHeaders.add(source);
-    }
-    if (isTextualInclude || !isCompiledSource || (isHeader && !shouldProcessHeaders())) {
-      return;
-    }
-    compilationUnitSources.add(Pair.of(source, label));
-  }
-
-  private boolean shouldProcessHeaders() {
-    return featureConfiguration.isEnabled(CppRuleClasses.PREPROCESS_HEADERS)
-        || featureConfiguration.isEnabled(CppRuleClasses.PARSE_HEADERS);
-  }
-
-  /**
-   * Returns the compilation unit sources. That includes all compiled source files as well
-   * as headers that will be parsed or preprocessed.
-   */
-  public ImmutableList<Pair<Artifact, Label>> getCompilationUnitSources() {
-    return ImmutableList.copyOf(this.compilationUnitSources);
-  }
 
   /**
    * Add the corresponding files as linker inputs for non-PIC links. If the corresponding files are
@@ -775,7 +713,7 @@ public final class CcLibraryHelper {
    */
   private CppModel initializeCppModel() {
     return new CppModel(ruleContext, semantics)
-        .addCompilationUnitSources(compilationUnitSources)
+        .addSources(sources)
         .addCopts(copts)
         .setLinkTargetType(linkType)
         .setNeverLink(neverlink)
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index 91ab7d89f1..1ddbff3789 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -124,9 +124,9 @@ public final class CppModel {
 
   /**
    * Adds a single source file to be compiled. Note that this should only be called for primary
-   * compilation units, including module files or headers to be parsed or preprocessed.
+   * compilation units, not for header files or files that are otherwise included.
    */
-  public CppModel addCompilationUnitSources(Iterable<Artifact> sourceFiles, Label sourceLabel) {
+  public CppModel addSources(Iterable<Artifact> sourceFiles, Label sourceLabel) {
     for (Artifact sourceFile : sourceFiles) {
       this.sourceFiles.add(Pair.of(sourceFile, sourceLabel));
     }
@@ -134,10 +134,10 @@ public final class CppModel {
   }
 
   /**
-   * Adds all the source files. Note that this should only be called for primary
-   * compilation units, including module files or headers to be parsed or preprocessed.
+   * Adds all the source files. Note that this should only be called for primary compilation units,
+   * not for header files or files that are otherwise included.
    */
-  public CppModel addCompilationUnitSources(Iterable<Pair<Artifact, Label>> sources) {
+  public CppModel addSources(Iterable<Pair<Artifact, Label>> sources) {
     Iterables.addAll(this.sourceFiles, sources);
     return this;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/IosFramework.java b/src/main/java/com/google/devtools/build/lib/rules/objc/IosFramework.java
index d60a62f4d9..0c553b2bfb 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/IosFramework.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/IosFramework.java
@@ -32,11 +32,9 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.rules.apple.DottedVersion;
 import com.google.devtools.build.lib.rules.cpp.CcCommon;
 import com.google.devtools.build.lib.rules.objc.ReleaseBundlingSupport.SplitArchTransition.ConfigurationDistinguisher;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 /**
@@ -86,8 +84,7 @@ public class IosFramework extends ReleaseBundlingTargetFactory {
     IntermediateArtifacts intermediateArtifacts =
         ObjcRuleClasses.intermediateArtifacts(ruleContext);
 
-    ImmutableList<Pair<Artifact, Label>> headers =
-        ImmutableList.copyOf(CcCommon.getHeaders(ruleContext));
+    ImmutableList<Artifact> headers = ImmutableList.copyOf(CcCommon.getHeaders(ruleContext));
 
     ImmutableMap.Builder<Artifact, Artifact> builder = new ImmutableMap.Builder<>();
 
@@ -97,11 +94,11 @@ public class IosFramework extends ReleaseBundlingTargetFactory {
     builder.put(intermediateArtifacts.combinedArchitectureBinary(), frameworkBinary);
 
     // Create framework headers
-    for (Pair<Artifact, Label> header : headers) {
+    for (Artifact header : headers) {
       Artifact frameworkHeader =
-          outputArtifact(ruleContext, new PathFragment("Headers/" + header.first.getFilename()));
+          outputArtifact(ruleContext, new PathFragment("Headers/" + header.getFilename()));
 
-      builder.put(header.first, frameworkHeader);
+      builder.put(header, frameworkHeader);
     }
 
     return builder.build();
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 4b8412ee36..b469b22c5c 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
@@ -61,7 +61,6 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
-import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.rules.apple.AppleToolchain;
 import com.google.devtools.build.lib.rules.cpp.CcCommon;
@@ -72,7 +71,6 @@ import com.google.devtools.build.lib.rules.cpp.CppModuleMap;
 import com.google.devtools.build.lib.rules.cpp.CppRunfilesProvider;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileType;
-import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
@@ -108,11 +106,7 @@ public final class ObjcCommon {
       if (!ruleContext.attributes().has("hdrs", BuildType.LABEL_LIST)) {
         return ImmutableList.of();
       }
-      ImmutableList.Builder<Artifact> headers = ImmutableList.builder();
-      for (Pair<Artifact, Label> header : CcCommon.getHeaders(ruleContext)) {
-        headers.add(header.first);
-      }
-      return headers.build();
+      return ImmutableList.copyOf(CcCommon.getHeaders(ruleContext));
     }
 
     /**