Ability To Turn Off Formatting For Subdirectory

[samdeane]

Draft implementation of #870.

This implementation adds a check for a .swift-format-ignore file, in the same places that a .swift-format file could be.

If present, this file potentially contains gitignore-style rules describing which files to ignore.

Currently the only rule supported is *, to ignore everything in the containing directory and any subdirectories. Later, full pattern-matching rules could be implemented, if desired. For now, we simply check that the content of the file is *, to ease any future migration problems.

The presence of .swift-format-ignore is checked first at any given directory level. Even if a .swift-format file also exists at the same level, the ignore file takes precedence.

Motivation

The use case which motivated this change is a scenario where my workspace has a .swift-format file, and I've turned on format-on-save, but I also have submodules containing third-party code.

If I make a small edit to a file in a submodule, it is formatted using the global workspace settings. This generates a large diff unrelated to my edit, as the submodule is hand-formatted using different rules.

I want to be able to disable formatting for specific submodules on my local machine, whilst retaining formatting for the rest of the workspace.

[allevato]

Thanks for looking into this! I think it's a useful feature to have. Some high-level comments:

• I think the name of the file should be .swift-format-ignore, which aligns nicely with the name used in comments that the formatter supports, and also similar tools like .gitignore and .prettierignore.

• Lowering this down into the SwiftFormatter and SwiftLinter APIs feels like the wrong approach, because it means the tool is still doing all the work of iterating over and parsing all the files that are going to be ignored anyway.

For the second point, raising this check into the frontend seems more appropriate. Maybe FileIterator should be responsible for looking for the .swift-format-ignore file in the directory or its parents, and if one is found, it doesn't recurse into the directory further? That may end up being simpler to implement (you don't have to touch the configuration at all).

It means that somebody using the SwiftFormatter/SwiftLinter APIs directly wouldn't get that logic, but I think that's fine; those APIs are meant to be used by clients who want to format something directly and supplying a configuration in memory, and they're not interested in the search logic used by the frontend.

[samdeane]

I think the name of the file should be .swift-format-ignore

Yes, makes sense 👍

[samdeane]

For the second point, raising this check into the frontend seems more appropriate. Maybe FileIterator should be responsible for looking for the .swift-format-ignore

I think I was fixated on the idea that the ignoring mode needed to be configurable from within a .swift-format, as well as by the presence of a .swift-format-ignore file. Having done it though, I struggle to see the utility of that setting.

I'll try your plan instead.

[samdeane]

Ok, I got rid of the setting and moved the check into FileIterator's directory handling.

It works well for input directories, and for subdirectories reached via recursion, but doesn't work for input files (e.g: in the case where individual file paths are specified on the command line directly).

For input files, we need to perform a check for .swift-format-ignore in the same directory as the file. Putting that into the iterator's next() feels wrong as it's not needed in most cases. My inclination is to filter the input urls in init, and perform this test on any that are files.

[allevato]

For input files, we need to perform a check for .swift-format-ignore in the same directory as the file. Putting that into the iterator's next() feels wrong as it's not needed in most cases. My inclination is to filter the input urls in init, and perform this test on any that are files.

That makes the most sense to me.

[samdeane]

Ok, 'tis a little closer now.

One question is what it should do if --configuration <file> is passed in.

The README still says that the filesystem won't be searched; which is true for .swift-format, but no longer true for .swift-format-ignore.

The path of least resistance is to clarify the README, but that depends whether that's the behaviour we want.

Alternatively I'll need to add a flag to FileIterator to indicate whether or not it should check for the ignore file. Which is perfectly doable, of course.

[allevato]

One question is what it should do if --configuration <file> is passed in.

The README still says that the filesystem won't be searched; which is true for .swift-format, but no longer true for .swift-format-ignore.

The implication of that statement is that the filesystem won't be searched for the configuration (i.e., .swift-format file) specifically. We don't need it to apply to the .swift-format-ignore file.

If we want a way to ignore the ignore file, we could add something like a --force option to the command line interface, but I don't think we need it today.

[samdeane]

If we want a way to ignore the ignore file, we could add something like a --force option to the command line interface, but I don't think we need it today.

Yeah, I was thinking the same. I'll reword the docs.

[allevato]

More than just checking for the presence of the file, I think we go the extra step of validating that it only contains * so that users aren't surprised later when we add more functionality and it subtly changes the behavior based on those contents. If it contains anything else, we should throw an error saying it's the only supported form (and turn that into a nice human-readable error message when we exit).

[andre-richter]

If we want to upgrade to gitignore like sytanx in the future, should we make the default value **/*?

[allevato]

AFAIK, * is sufficient because according to the documentation:

• "An asterisk '*' matches anything except a slash."
• "If there is a separator at the beginning or middle (or both) of the pattern, [...]. Otherwise the pattern may also match at any level below the .gitignore level."
• "If there is a separator at the end of the pattern then the pattern will only match directories, otherwise the pattern can match both files and directories."

[samdeane]

Sorry, got distracted, but am looking at this now.

One complication is that FileIterator doesn't throw, and instead relies on the caller to generate diagnostics for non-existent paths.

In order to report invalid ignore files inside FileIterator we'd either need to be able to:

• throw an error from it
• be able to emit diagnostics directly from it
• return the path of the invalid ignore file (or a special pseudo-path), as if they were to be processed, then spot that path in the front end and emit an "invalid ignore file" diagnostic

I've gone for the third option, but it feels like a bit of a hack.

[ahoppen]

Could you add a test case that ensures this diagnostic gets produced?

[samdeane]

Do you have existing code to test the front end output over a directory?
If you do, then sure.
If not, I think adding that code is outside the scope of this change and would be better off as a follow-up issue.

[samdeane]

Actually I've realised that the current design won't work as written, because when FileIterator encounters a directory, it collapses all subdirectories below it, and therefore will not check for the presence of nested .swift-format-ignore files.

FileIterator will need to be rewritten to check for an ignore file when entering a directory, and to pass that instance down to any subdirectories recursively.

[samdeane]

I'm happy to do this, but it raises a couple of design questions:

• If we flow through the directory structure, inheriting the ignore settings from the parent directory, how should we treat sym links to other directories? Should they inherit their settings from their actual parent, or their logical parent in the context of the traversal?
• We we're traversing a file tree and looking for .swift-format-ignore files, I wonder if we should also be looking for .swift-format files, and producing a (URL, Config) tuple for each element. It would be a lot more efficient than search for a nested config whilst processing every file.

[samdeane]

Slightly annoyingly, my original naive implementation was simple and worked fine, since it wasn't the front end doing the work.

I don't think it would be unreasonable to go with that simpler approach as the first iteration, and then refine it subsequently -- perfect being the enemy of good and all that. That said, I'm happy to look at reworking FileIterator instead.

[samdeane]

In other news, the current main branch (ed01028, without my changes) will not build with my version of Swift:

swift-driver version: 1.115.1 Apple Swift version 6.0.3 (swiftlang-6.0.3.1.9 clang-1600.0.30.1) Target: arm64-apple-macosx15.0

/Users/sam/Developer/Projects/swift-format/Sources/SwiftFormat/Rules/UseShorthandTypeNames.swift:50:78: error: type 'TypeSyntax?' has no member 'type'
 48 |     switch node.name.text {
 49 |     case "Array":
 50 |       guard case .type(let typeArgument) = genericArgumentList.firstAndOnly?.argument else {
    |                                                                              `- error: type 'TypeSyntax?' has no member 'type'
 51 |         newNode = nil
 52 |         break

(and a bunch of other errors)

[samdeane]

fwiw git bisect tells me that the problematic commit is c3b0c9f

[allevato]

Doing this in the frontend is still the right approach; the API layer shouldn't care about .swift-format-ignore files just as it doesn't care about .swift-format configuration files. Implementing it in the API layer as an incremental step would be putting us into a worse state than not having it, design-wise.

If we flow through the directory structure, inheriting the ignore settings from the parent directory, how should we treat sym links to other directories? Should they inherit their settings from their actual parent, or their logical parent in the context of the traversal?

I think the answer has to be the actual (physical) parent. Otherwise you could end up with different ignore states for the same file depending on how you got to it, which wouldn't be desirable. This does make the implementation slightly more complicated, but I think it would be workable to have FileIterator maintain a cache of some sort to keep track of the ignore-specs found in each directory it visits. When you visit a directory you look in the cache to see if it or any of its parents have loaded their ignore-specs, and in most cases, this will be fast because you got to that directory from its immediate parent. In the case where you follow a symlink into a different directory tree, you might need to look up all the way toward the root to find the ignore-specs (or know that they don't exist), but that would be rarer.

We we're traversing a file tree and looking for .swift-format-ignore files, I wonder if we should also be looking for .swift-format files, and producing a (URL, Config) tuple for each element. It would be a lot more efficient than search for a nested config whilst processing every file.

This sounds like a nice optimization! We flatten the iterator into an array in the frontend to do parallel processing so we don't want to have the full Configuration stored with each file URL (it would be identical across all files in the majority of cases), but returning a pair with the URL of the file to format and the URL of the relevant configuration would still be an improvement, I think. Then we can still leverage the existing configuration cache to load them on-demand, without the search each time.

The same caveat about which parent to use applies here as above. For symlinks, we'd want the configuration we choose to be the one from the physical parent, not the logical parent, because we don't want files to be formatted differently based on how you got there.

In other news, the current main branch (ed01028, without my changes) will not build with my version of Swift:

Is your swift-syntax checkout out of date? The change you referenced later was made to support an API change merged a few days ago in swift-syntax.

[samdeane]

Is your swift-syntax checkout out of date? The change you referenced later was made to support an API change merged a few days ago in swift-syntax.

Hmm, I don't have SWIFTCI_USE_LOCAL_DEPS set, so it should just be using what the package specifies. Unfortunately that is just the main branch, which I think is the problem.

Some combination of swift package purge-cache and swift package update convinced SwiftPM to pull the latest swift-syntax, which then worked with swift-format's main; unfortunately switching back to an old branch of swift-format then broke for similar reasons...

If it can't be pinned to a version, wouldn't it be better to pin it to an actual commit?

[allevato]

The main branches of swift-format and swift-syntax are always kept in sync to build against each other, so pinning swift-format's main branch to a specific commit wouldn't make sense. If you need to switch back to an older branch, rebasing it on top of main should be enough to get you in a working state again.