Implement a warning or error if an unrecognized Rule is specified in .swift-format

[natikgadzhi]

Right now, if one made a typo in a rule name in .swift-format and then ran the formatter, it will ignore the typo and, obviously, not apply the rule.

I was working on swiftlang/swift-syntax#2145, and of course, that's precisely what I did. Oh, nice, our imports are already ordered, right?

Proposed solution

Since we know the list of all available rules, we could verify that each string in the Rules block matches an existing rule.

// Config.swift
//
// If the `rules` key is not present at all, default it to the built-in set
// so that the behavior is the same as if the configuration had been
// default-initialized. To get an empty rules dictionary, one can explicitly
// set the `rules` key to `{}`.
self.rules =
  try container.decodeIfPresent([String: Bool].self, forKey: .rules)
  ?? defaults.rules

It should be relatively easy, we could just generate an enum with all rule names, and use that as the key when decoding the rules.

Should I draft this in a PR?

[allevato]

This is a great idea for improvement, but I think it's a little bit more complicated than you've described here.

The configuration decoding logic doesn't have any way to pass information out of it other than "here's a successfully parsed config" or "here's an error because it failed". So if we want a warning, we have to check it in the frontend after decoding it; otherwise, we can make it an error during the actual decoding process.

The only real value to a warning—aside from just being a little more relaxed—would be to retain compatibility of newer config files with older versions of swift-format, but I don't think that's something we really need to do. So just throwing an error would be easier to implement.

I don't know if we need all the ceremony of generating a new enum for this, though. We already generate a Dictionary with all the rule names as keys here, so after decoding self.rules we could just check that all the decoded rule names exist in that dictionary (doesn't matter what the value is), and throw a decoding error if there's an unrecognized name.

Feel free to send a PR!

[natikgadzhi]

The configuration decoding logic doesn't have any way to pass information out of it other than "here's a successfully parsed config" or "here's an error because it failed". So if we want a warning, we have to check it in the frontend after decoding it; otherwise, we can make it an error during the actual decoding process.

I've explored a bit yesterday, and yep — I think one of the questions is at what level should that check be. It could be in Configuration.decode, but that Codable implementation, in theory, doesn't have to be aware of any domain logic. It can just parse JSON into what it states — [String:Bool].

Then, there's ConfigurationLoader that's relatively simple — perhaps that's the right level. But I gravitate to the next one:

We could put that into Frontend. I'm looking to confirm how exactly it loads config, seems like it has at least two ways to do it, but essentially, we could validate the configuration after we load it, and emit a warning in that context, which is nice and easy because Frontend has diagnosticsEngine on it already.

The only real value to a warning—aside from just being a little more relaxed—would be to retain compatibility of newer config files with older versions of swift-format, but I don't think that's something we really need to do. So just throwing an error would be easier to implement.

If we decide to go with an error, than the simplest solution by far is a check in Configuration.decode, like this:

self.rules = try container.decode(...)

// 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 = self.rules.filter { !RuleRegistry.rules.keys.contains($0.key) }
if !invalidRules.isEmpty {
  throw ConfigurationError.invalidConfiguration("Configuration contains an unsupported rule: \(invalidRules.keys.joined(separator: ", "))"
}

But, if we stick with a warning, we could do that check after we decode, and that would make it so any projects that have a broken config today won't blow up their CI when they update.

I don't know if we need all the ceremony of generating a new enum for this, though.

Yep, I think RuleRegistry.rules is sufficient!

Here's what I have now (rough draft, copy and pasting):

// In frontend.swift

  private func configuration(
    at configurationFileURL: URL?,
    orInferredFromSwiftFileAt swiftFileURL: URL?
  ) -> Configuration? {
    // If an explicit configuration file path was given, try to load it and fail if it cannot be
    // loaded. (Do not try to fall back to a path inferred from the source file path.)
    if let configurationFileURL = configurationFileURL {
      do {
        let configuration = try configurationLoader.configuration(at: configurationFileURL)

        // 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) }
        if !invalidRules.isEmpty {
          diagnosticsEngine.emitWarning("Configuration contains an unsupported rule: \(invalidRules.keys.joined(separator: ", "))", location: nil)
          return nil
        }

        return configuration
      } catch {
        diagnosticsEngine.emitError("Unable to read configuration: \(error.localizedDescription)")
        return nil
      }
    }

    // If no explicit configuration file path was given but a `.swift` source file path was given,
    // then try to load the configuration by inferring it based on the source file path.
    if let swiftFileURL = swiftFileURL {
      do {
        if let configuration = try configurationLoader.configuration(forSwiftFileAt: swiftFileURL) {
          return configuration
        }
        // Fall through to the default return at the end of the function.
      } catch {
        diagnosticsEngine.emitError(
          "Unable to read configuration for \(swiftFileURL.path): \(error.localizedDescription)")
        return nil
      }
    }

    // If neither path was given (for example, formatting standard input with no assumed filename)
    // or if there was no configuration found by inferring it from the source file path, return the
    // default configuration.
    return Configuration()
  }

I think if I just make that check run in both branches of loading configuration, we'll be fine.

Expect a PR in a few hours, depending on my 3 y/o daughter's mood. So far, she does not really understand why swift-format needs to emit warnings 👯‍♀️