Simplify input file creation logic

This alters an error message that used some obsolete verbiage (it said "generated label" when really it could be any non-input-file target).

- Eliminate GeneratedLabelConflict subclass. It's unnecessary to distinguish the case of input file conflicts from other types of name conflicts.

- Clarify comment about nameConflictCheckingPolicy.

- Remove addInputFile() overload that did its own creation of the InputFile. This makes custody of the input file object more explicit and allows it to be passed to helper functions like nameConflict().

- In createInputFile(), reorder clauses for readability and reuse of InputFile object across two cases.

Work toward #19922.

PiperOrigin-RevId: 596059017
Change-Id: Ie7acc7ba7592875c8726271b01c81f4a56c42a2d

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -114,8 +114,13 @@ public class Package {
   /** Sentinel value for package overhead being empty. */
   private static final long PACKAGE_OVERHEAD_UNSET = -1;
 
-  /** Common superclass for all name-conflict exceptions. */
-  public static class NameConflictException extends Exception {
+  /**
+   * An exception used when a target's name conflicts with other targets in the package.
+   *
+   * <p>This could be an exact match, or in the case of output files, a prefix clash (one output
+   * file is a prefix of another).
+   */
+  public static final class NameConflictException extends Exception {
     private NameConflictException(String message) {
       super(message);
     }
@@ -895,8 +900,13 @@ private enum NameConflictCheckingPolicy {
 
     /**
      * Whether to do validation checks for name clashes between targets and between output file
-     * prefixes. This should only be disabled for data that has already been validated, e.g. in
-     * package deserialization.
+     * prefixes.
+     *
+     * <p>This should only be disabled for data that has already been validated, e.g. in package
+     * deserialization.
+     *
+     * <p>Even disabling this does not turn off *all* checking, just some of the more expensive
+     * ones. Do not rely on being able to violate these checks.
      */
     private NameConflictCheckingPolicy nameConflictCheckingPolicy =
         NameConflictCheckingPolicy.UNKNOWN;
@@ -1026,7 +1036,9 @@ public T intern(T sample) {
 
       // Add target for the BUILD file itself.
       // (This may be overridden by an exports_file declaration.)
-      addInputFile(metadata.buildFileLabel, Location.fromFile(filename.asPath().toString()));
+      addInputFile(
+          new InputFile(
+              pkg, metadata.buildFileLabel, Location.fromFile(filename.asPath().toString())));
     }
 
     PackageIdentifier getPackageIdentifier() {
@@ -1376,42 +1388,39 @@ private Iterable<Rule> getRules() {
       return Package.getTargets(targets, Rule.class);
     }
 
-    /** An input file name conflicts with an existing package member. */
-    static class GeneratedLabelConflict extends NameConflictException {
-      private GeneratedLabelConflict(String message) {
-        super(message);
-      }
-    }
-
     /**
-     * Creates an input file target in this package with the specified name.
+     * Creates an input file target in this package with the specified name, if it does not yet
+     * exist.
+     *
+     * <p>This operation is idempotent.
      *
      * @param targetName name of the input file. This must be a valid target name as defined by
      *     {@link com.google.devtools.build.lib.cmdline.LabelValidator#validateTargetName}.
-     * @return the newly-created InputFile, or the old one if it already existed.
-     * @throws GeneratedLabelConflict if the name was already taken by a Rule or an OutputFile
-     *     target.
+     * @return the newly-created {@code InputFile}, or the old one if it already existed.
+     * @throws NameConflictException if the name was already taken by another target that is not an
+     *     input file
      * @throws IllegalArgumentException if the name is not a valid label
      */
-    InputFile createInputFile(String targetName, Location location) throws GeneratedLabelConflict {
+    InputFile createInputFile(String targetName, Location location) throws NameConflictException {
       Target existing = targets.get(targetName);
-      if (existing == null) {
-        try {
-          return addInputFile(createLabel(targetName), location);
-        } catch (LabelSyntaxException e) {
+
+      if (existing instanceof InputFile) {
+        return (InputFile) existing; // idempotent
+      }
+
+      InputFile inputFile;
+      try {
+        inputFile = new InputFile(pkg, createLabel(targetName), location);
+      } catch (LabelSyntaxException e) {
           throw new IllegalArgumentException(
               "FileTarget in package " + pkg.getName() + " has illegal name: " + targetName, e);
-        }
-      } else if (existing instanceof InputFile) {
-        return (InputFile) existing; // idempotent
+      }
+
+      if (existing == null) {
+        addInputFile(inputFile);
+        return inputFile;
       } else {
-        throw new GeneratedLabelConflict(
-            "generated label '//"
-                + pkg.getName()
-                + ":"
-                + targetName
-                + "' conflicts with existing "
-                + existing.getTargetKind());
+        throw nameConflict(inputFile, existing);
       }
     }
 
@@ -1761,14 +1770,14 @@ Package build(boolean discoverAssumedInputFiles) throws NoSuchPackageException {
       return finishBuild();
     }
 
-    private InputFile addInputFile(Label label, Location location) {
-      return addInputFile(new InputFile(pkg, label, location));
-    }
-
-    private InputFile addInputFile(InputFile inputFile) {
+    /**
+     * Adds an input file to this package.
+     *
+     * <p>There must not already be a target with the same name (i.e., this is not idempotent).
+     */
+    private void addInputFile(InputFile inputFile) {
       Target prev = targets.put(inputFile.getLabel().getName(), inputFile);
       Preconditions.checkState(prev == null);
-      return inputFile;
     }
 
     /**

src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java

@@ -598,7 +598,7 @@ public NoneType exportsFiles(
         }
 
         pkgBuilder.setVisibilityAndLicense(inputFile, visibility, license);
-      } catch (Package.Builder.GeneratedLabelConflict e) {
+      } catch (Package.NameConflictException e) {
         throw Starlark.errorf("%s", e.getMessage());
       }
     }

src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java

@@ -1010,7 +1010,7 @@ public void testExportGenruleConflict() throws Exception {
   @Test
   public void testGenruleExportConflict() throws Exception {
     expectEvalError(
-        "generated label '//pkg:a.cc' conflicts with existing generated file",
+        "source file 'a.cc' conflicts with existing generated file from rule 'foo'",
         "genrule(name = 'foo',",
         "    outs = ['a.cc'],",
         "    cmd = '')",