Support file-level ignore directive for specific rules

[TTOzzi]

Resolve #927

I have modified the function that disables specific rules in node-level directive so that it can also be reused for file-level directive, ensuring both follow the same logic.

Previously, if a file-level directive appeared in the topmost comment, we would always skip visiting nodes. However, with this change, we now visit child nodes when only specific rules are disabled. As a result, this caused an unintended side effect in cases where the OrderedImports rule was enabled. Specifically, a directive included in the leadingTrivia of a sorted import statement was being reordered along with the imports.
To resolve this, I modified the OrderedImports rule to treat the swift-format-ignore-file directive as part of the file header.

Additionally, to avoid simple string comparisons like:

if line.description.contains("swift-format-ignore-file") {

I have created an IgnoreDirective enum to handle ignore directives more cleanly.

For easier review, I have split the commits.
Once the review is complete, I will squash and force push them before merging. 🙂

[ahoppen]

@TTOzzi The Windows CI failures should be addressed by #951. Could you rebase your changes on top of main so that we can pick up that fix for CI?

[TTOzzi]

@TTOzzi The Windows CI failures should be addressed by #951. Could you rebase your changes on top of main so that we can pick up that fix for CI?

Oh, thank you for identifying the cause of the failure 😃
I’ve rebased it.

[ahoppen]

Thank you and thanks for remembering to not re-order the ignore directive in ordered imports. I’ve got a couple of thoughts inline.

[ahoppen]

Does this also end up adding a newline after // swift-format-ignore-file if no re-ordering is necessary? Eg. for

// swift-format-ignore-file
import LibA
import LibB

To be clear, I’m fine with that, just want to check and record the behavior in a test case.

---

One thing I did notice is that I think this flips comments order around if you have eg.

// We need to ignore this file because it is generated
// swift-format-ignore-line

import Foundation

Maybe swift-format-ignore-line just needs to flush the commentBuffer into the headerBuffer similar to what a blank line does.

[TTOzzi]

Does this also end up adding a newline after // swift-format-ignore-file if no re-ordering is necessary?

Yes, that's right. In the current implementation, if the ignore directive does not disable OrderedImports,

// swift-format-ignore-file: DoNotUseSemicolons
import LibA
import LibB

is formatted as

// swift-format-ignore-file: DoNotUseSemicolons

import LibA
import LibB

because the // swift-format-ignore-file: DoNotUseSemicolons comment is treated as part of the file header by the OrderedImports rule and is separated accordingly.
However, if the ignore directive includes OrderedImports, such as // swift-format-ignore-file or // swift-format-ignore-file: OrderedImports, the original code is preserved.

Additionally, I have fixed the issue where the comment order could be flipped when// swift-format-ignore-line appeared after the first line, and I have added a test case for it.

Thanks for the feedback!

[ahoppen]

Since you are working on this already, I’ve got a few thoughts:

• The regex restriction after the : seems a little restrictive to me. Eg. I imagine it would be fine to try and debug why a future rule that contains a _ cannot be ignored using a directive. Maybe we can just make the regex a prefix match. @allevato Do you remember why the regex is this specific?
• This re-compiles the regex every time regex is accessed. Compiling regular expressions is fairly expensive, so I’d prefer to cache that.
• Could we use Swift’s Regex instead of NSRegularExpression?
• Or, alternatively, would it be easier to write a simple line matcher and avoid going through regex matching at all?

All except for avoiding to re-compile the regex don’t need to be in this PR.

[allevato]

@allevato Do you remember why the regex is this specific?

Not off the top of my head. It may have just been to capture precisely what we expected rule names to look like at the time. I think it would be fine to be a little less prescriptive in the regex itself, if it simplifies things.

Could we use Swift’s Regex instead of NSRegularExpression?

We'd have to raise our minimum deployment version, which is macOS 12.0 right now, since Regex requires 13.0. If that aligns with other binaries in the Swift toolchain, I'd love to drop NSRegularExpression (holistically, in a separate PR).

[TTOzzi]

Yes, I have updated the implementation to cache the regex instead of recompiling it each time.

And as for other suggestions, would it be a good idea to create a separate issue for these?

• Relaxing the regex restrictions
  (If we can raise the minimum deployment version, migrating from NSRegularExpression to Swift’s Regex)
• Alternatively, parsing ignore directives using string matching instead of regex

[ahoppen]

Issues (or PRs 😉) for those items would be good, yes. Also, I am happy to raise the deployment target to macOS 13. SourceKit-LSP, for example, already requires macOS 13.0.

[TTOzzi]

Got it! I'll work on a follow-up PR for those items 😁

[ahoppen]

Thanks. Looks good to me 👍🏽