From 5d45493b8f54041ead10ced7f60b2f7feaf1eb11 Mon Sep 17 00:00:00 2001 From: Natik Gadzhi Date: Sun, 3 Sep 2023 18:17:56 -0700 Subject: [PATCH] Output a warning for unknown config rules **Summary**: - Helps developers surface any errors and typos in their `.swift-format` config files by showing a warning when running `swift-format` if the configuration includes invalid rules. - The actual check is in `Frontend.swift`. - The dictionary of rules for verification is in `RuleRegistry`, which is made `public` to be able to import it in `swift-format` target. **Motivation**: - Closes #607 Apply suggestions from code review Co-authored-by: Tony Allevato Cleaning up based on PR review --- .../Core/RuleRegistry+Generated.swift | 4 ++-- .../RuleRegistryGenerator.swift | 4 ++-- Sources/swift-format/Frontend/Frontend.swift | 23 ++++++++++++++++--- .../Utilities/DiagnosticsEngine.swift | 14 +++++++++++ 4 files changed, 38 insertions(+), 7 deletions(-) diff --git a/Sources/SwiftFormat/Core/RuleRegistry+Generated.swift b/Sources/SwiftFormat/Core/RuleRegistry+Generated.swift index cd6c7ca4..7a0de731 100644 --- a/Sources/SwiftFormat/Core/RuleRegistry+Generated.swift +++ b/Sources/SwiftFormat/Core/RuleRegistry+Generated.swift @@ -12,8 +12,8 @@ // This file is automatically generated with generate-pipeline. Do Not Edit! -enum RuleRegistry { - static let rules: [String: Bool] = [ +@_spi(Internal) public enum RuleRegistry { + public static let rules: [String: Bool] = [ "AllPublicDeclarationsHaveDocumentation": false, "AlwaysUseLowerCamelCase": true, "AmbiguousTrailingClosureOverload": true, diff --git a/Sources/generate-pipeline/RuleRegistryGenerator.swift b/Sources/generate-pipeline/RuleRegistryGenerator.swift index 7a564420..8e4c66ff 100644 --- a/Sources/generate-pipeline/RuleRegistryGenerator.swift +++ b/Sources/generate-pipeline/RuleRegistryGenerator.swift @@ -40,8 +40,8 @@ final class RuleRegistryGenerator: FileGenerator { // This file is automatically generated with generate-pipeline. Do Not Edit! - enum RuleRegistry { - static let rules: [String: Bool] = [ + @_spi(Internal) public enum RuleRegistry { + public static let rules: [String: Bool] = [ """ ) diff --git a/Sources/swift-format/Frontend/Frontend.swift b/Sources/swift-format/Frontend/Frontend.swift index de62ff4a..28391a19 100644 --- a/Sources/swift-format/Frontend/Frontend.swift +++ b/Sources/swift-format/Frontend/Frontend.swift @@ -11,7 +11,7 @@ //===----------------------------------------------------------------------===// import Foundation -import SwiftFormat +@_spi(Internal) import SwiftFormat import SwiftSyntax import SwiftParser @@ -161,17 +161,19 @@ class Frontend { } /// Returns the configuration that applies to the given `.swift` source file, when an explicit - /// configuration path is also perhaps provided. + /// configuration path is also perhaps provided. Checks for unrecognized rules within the configuration. /// /// - Parameters: /// - configurationFilePath: The path to a configuration file that will be loaded, or `nil` to /// try to infer it from `swiftFilePath`. /// - swiftFilePath: The path to a `.swift` file, which will be used to infer the path to the /// configuration file if `configurationFilePath` is nil. + /// /// - Returns: If successful, the returned configuration is the one loaded from /// `configurationFilePath` if it was provided, or by searching in paths inferred by /// `swiftFilePath` if one exists, or the default configuration otherwise. If an error occurred /// when reading the configuration, a diagnostic is emitted and `nil` is returned. + /// if neither `configurationFilePath` nor `swiftFilePath` were provided, a default `Configuration()` will be returned. private func configuration( at configurationFileURL: URL?, orInferredFromSwiftFileAt swiftFileURL: URL? @@ -180,7 +182,9 @@ class Frontend { // loaded. (Do not try to fall back to a path inferred from the source file path.) if let configurationFileURL = configurationFileURL { do { - return try configurationLoader.configuration(at: configurationFileURL) + let configuration = try configurationLoader.configuration(at: configurationFileURL) + self.checkForUnrecognizedRules(in: configuration) + return configuration } catch { diagnosticsEngine.emitError("Unable to read configuration: \(error.localizedDescription)") return nil @@ -192,6 +196,7 @@ class Frontend { if let swiftFileURL = swiftFileURL { do { if let configuration = try configurationLoader.configuration(forSwiftFileAt: swiftFileURL) { + self.checkForUnrecognizedRules(in: configuration) return configuration } // Fall through to the default return at the end of the function. @@ -207,4 +212,16 @@ class Frontend { // default configuration. return Configuration() } + + /// Checks if all the rules in the given configuration are supported by the registry. + /// If there are any rules that are not supported, they are emitted as a warning. + private func checkForUnrecognizedRules(in configuration: Configuration) { + // If any rules in the decoded configuration are not supported by the registry, + // emit them into the diagnosticsEngine as warnings. + // That way they will be printed out, but we'll continue execution on the valid rules. + let invalidRules = configuration.rules.filter { !RuleRegistry.rules.keys.contains($0.key) } + for rule in invalidRules { + diagnosticsEngine.emitWarning("Configuration contains an unrecognized rule: \(rule.key)", location: nil) + } + } } diff --git a/Sources/swift-format/Utilities/DiagnosticsEngine.swift b/Sources/swift-format/Utilities/DiagnosticsEngine.swift index c2353b19..a6cd6978 100644 --- a/Sources/swift-format/Utilities/DiagnosticsEngine.swift +++ b/Sources/swift-format/Utilities/DiagnosticsEngine.swift @@ -65,6 +65,20 @@ final class DiagnosticsEngine { message: message)) } + /// Emits a generic warning message. + /// + /// - Parameters: + /// - message: The message associated with the error. + /// - location: The location in the source code associated with the error, or nil if there is no + /// location associated with the error. + func emitWarning(_ message: String, location: SourceLocation? = nil) { + emit( + Diagnostic( + severity: .warning, + location: location.map(Diagnostic.Location.init), + message: message)) + } + /// Emits a finding from the linter and any of its associated notes as diagnostics. /// /// - Parameter finding: The finding that should be emitted.