Don't emit "unhandled files" warnings for files that are handled by build commands from package plugins (#3971)

In the long run we will want to make the built-in rules and plugins all go through the same funnel.  This is a more tactical change than that, which defers emitting warnings until after running the plugins.

rdar://74663980

Author: abertelrud

Sources/Build/BuildOperation.swift

@@ -209,6 +209,36 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS
         let prebuildCommandResults = try graph.reachableTargets.reduce(into: [:], { partial, target in
             partial[target] = try buildToolPluginInvocationResults[target].map { try self.runPrebuildCommands(for: $0) }
         })
+
+        // Emit warnings about any unhandled files in authored packages. We do this after applying build tool plugins, once we know what files they handled.
+        for package in graph.rootPackages where package.manifest.toolsVersion >= .v5_3 {
+            for target in package.targets {
+                // Get the set of unhandled files in targets.
+                var unhandledFiles = Set(target.underlyingTarget.others)
+                if unhandledFiles.isEmpty { continue }
+                
+                // Subtract out any that were inputs to any commands generated by plugins.
+                if let result = buildToolPluginInvocationResults[target] {
+                    let handledFiles = result.flatMap{ $0.buildCommands.flatMap{ $0.inputFiles } }
+                    unhandledFiles.subtract(handledFiles)
+                }
+                if unhandledFiles.isEmpty { continue }
+                
+                // Emit a diagnostic if any remain. This is kept the same as the previous message for now, but this could be improved.
+                let diagnosticsEmitter = self.observabilityScope.makeDiagnosticsEmitter {
+                    var metadata = ObservabilityMetadata()
+                    metadata.packageIdentity = package.identity
+                    metadata.packageKind = package.manifest.packageKind
+                    metadata.targetName = target.name
+                    return metadata
+                }
+                var warning = "found \(unhandledFiles.count) file(s) which are unhandled; explicitly declare them as resources or exclude from the target\n"
+                for file in unhandledFiles {
+                    warning += "    " + file.pathString + "\n"
+                }
+                diagnosticsEmitter.emit(warning: warning)
+            }
+        }
 
         // Create the build plan based, on the graph and any information from plugins.
         let plan = try BuildPlan(

Sources/PackageLoading/TargetSourcesBuilder.swift

@@ -162,26 +162,11 @@ public struct TargetSourcesBuilder {
             pathToRule[path] = self.computeRule(for: path)
         }
 
-        // Emit an error if we found files without a matching rule in
-        // tools version >= v5_3. This will be activated once resources
-        // support is complete.
-        var others: [AbsolutePath] = []
-        if toolsVersion >= .v5_3 {
-            let filesWithNoRules = pathToRule.filter { $0.value.rule == .none }
-            if !filesWithNoRules.isEmpty, self.packageKind.emitAuthorWarnings {
-                var warning = "found \(filesWithNoRules.count) file(s) which are unhandled; explicitly declare them as resources or exclude from the target\n"
-                for (file, _) in filesWithNoRules {
-                    warning += "    " + file.pathString + "\n"
-                }
-                self.observabilityScope.emit(warning: warning)
-            }
-            others.append(contentsOf: filesWithNoRules.keys)
-        }
-
         let headers = pathToRule.lazy.filter { $0.value.rule == .header }.map { $0.key }.sorted()
         let compilePaths = pathToRule.lazy.filter { $0.value.rule == .compile }.map { $0.key }
         let sources = Sources(paths: Array(compilePaths), root: targetPath)
         let resources: [Resource] = pathToRule.compactMap { resource(for: $0.key, with: $0.value) }
+        let others = pathToRule.filter { $0.value.rule == .none }.map { $0.key }
 
         diagnoseConflictingResources(in: resources)
         diagnoseCopyConflictsWithLocalizationDirectories(in: resources)

Sources/PackageModel/Target.swift

@@ -434,6 +434,7 @@ public final class ClangTarget: Target {
         type: Kind,
         sources: Sources,
         resources: [Resource] = [],
+        others: [AbsolutePath] = [],
         dependencies: [Target.Dependency] = [],
         buildSettings: BuildSettings.AssignmentTable = .init()
     ) {
@@ -452,6 +453,7 @@ public final class ClangTarget: Target {
             type: type,
             sources: sources,
             resources: resources,
+            others: others,
             dependencies: dependencies,
             buildSettings: buildSettings,
             pluginUsages: []

Tests/CommandsTests/PackageToolTests.swift

@@ -1070,6 +1070,16 @@ final class PackageToolTests: CommandsTestCase {
                     public func Foo() { }
                 """
             }
+            try localFileSystem.writeFileContents(packageDir.appending(components: "Sources", "MyLibrary", "library.foo")) {
+                $0 <<< """
+                    a file with a filename suffix handled by the plugin
+                """
+            }
+            try localFileSystem.writeFileContents(packageDir.appending(components: "Sources", "MyLibrary", "library.bar")) {
+                $0 <<< """
+                    a file with a filename suffix not handled by the plugin
+                """
+            }
             try localFileSystem.writeFileContents(packageDir.appending(components: "Plugins", "MyPlugin", "plugin.swift")) {
                 $0 <<< """
                     import PackagePlugin
@@ -1080,23 +1090,30 @@ final class PackageToolTests: CommandsTestCase {
                             context: PluginContext,
                             target: Target
                         ) throws -> [Command] {
+                            // Check that the package display name is what we expect.
                             guard context.package.displayName == "MyPackage" else {
-                                throw Error.inconsistentDisplayName(context.package.displayName)
+                                throw "expected display name to be ‘MyPackage’ but found ‘\\(context.package.displayName)’"
                             }
-                            return []
+                            // Create and return a build command that uses all the `.foo` files in the target as inputs, so they get counted as having been handled.
+                            let fooFiles = (target as? SourceModuleTarget)?.sourceFiles.compactMap{ $0.path.extension == "foo" ? $0.path : nil } ?? []
+                            return [ .buildCommand(displayName: "A command", executable: "/bin/echo", arguments: ["Hello"], inputFiles: fooFiles) ]
                         }
 
-                        enum Error : Swift.Error {
-                        case inconsistentDisplayName(String)
-                        }
                     }
+                    extension String : Error {}
                 """
             }
 
             // Invoke it, and check the results.
             let result = try SwiftPMProduct.SwiftBuild.executeProcess([], packagePath: packageDir)
             XCTAssertEqual(result.exitStatus, .terminated(code: 0))
             XCTAssert(try result.utf8Output().contains("Build complete!"))
+            
+            // We expect a warning about `library.bar` but not about `library.foo`.
+            let stderrOutput = try result.utf8stderrOutput()
+            XCTAssertMatch(stderrOutput, .contains("found 1 file(s) which are unhandled"))
+            XCTAssertNoMatch(stderrOutput, .contains("Sources/MyLibrary/library.foo"))
+            XCTAssertMatch(stderrOutput, .contains("Sources/MyLibrary/library.bar"))
         }
     }
 

Tests/PackageLoadingTests/TargetSourcesBuilderTests.swift

@@ -855,13 +855,13 @@ class TargetSourcesBuilderTests: XCTestCase {
             fileSystem: fs,
             observabilityScope: observability.topScope
         )
-        _ = try builder.run()
+        let outputs = try builder.run()
+        XCTAssertEqual(outputs.sources.paths, [AbsolutePath("/File.swift")])
+        XCTAssertEqual(outputs.resources, [])
+        XCTAssertEqual(outputs.others, [AbsolutePath("/Foo.xcdatamodel")])
 
-        testDiagnostics(observability.diagnostics) { result in
-            let diagnostic = result.check(diagnostic: "found 1 file(s) which are unhandled; explicitly declare them as resources or exclude from the target\n    /Foo.xcdatamodel\n", severity: .warning)
-            XCTAssertEqual(diagnostic?.metadata?.packageIdentity, builder.packageIdentity)
-            XCTAssertEqual(diagnostic?.metadata?.targetName, target.name)
-        }
+        XCTAssertFalse(observability.hasWarningDiagnostics)
+        XCTAssertFalse(observability.hasErrorDiagnostics)
     }
 
     func testUnhandledResources() throws {
@@ -895,13 +895,13 @@ class TargetSourcesBuilderTests: XCTestCase {
                 fileSystem: fs,
                 observabilityScope: observability.topScope
             )
-            _ = try builder.run()
+            let outputs = try builder.run()
+            XCTAssertEqual(outputs.sources.paths, [AbsolutePath("/File.swift")])
+            XCTAssertEqual(outputs.resources, [])
+            XCTAssertEqual(outputs.others, [AbsolutePath("/foo.bar")])
 
-            testDiagnostics(observability.diagnostics) { result in
-                let diagnostic = result.check(diagnostic: "found 1 file(s) which are unhandled; explicitly declare them as resources or exclude from the target\n    /foo.bar\n", severity: .warning)
-                XCTAssertEqual(diagnostic?.metadata?.packageIdentity, builder.packageIdentity)
-                XCTAssertEqual(diagnostic?.metadata?.targetName, target.name)
-            }
+            XCTAssertFalse(observability.hasWarningDiagnostics)
+            XCTAssertFalse(observability.hasErrorDiagnostics)
         }
 
         // should not emit for "remote" packages