Fix picking the wrong configuration with —path

[marcelofabri]

Fixes #1744

Pending tests.

[SwiftLintBot]

	12 Messages
📖	Linting Aerial with this PR took 0.37s vs 0.34s on master (8% slower)
📖	Linting Alamofire with this PR took 2.47s vs 2.41s on master (2% slower)
📖	Linting Firefox with this PR took 10.27s vs 10.21s on master (0% slower)
📖	Linting Kickstarter with this PR took 15.74s vs 15.02s on master (4% slower)
📖	Linting Moya with this PR took 1.06s vs 1.06s on master (0% slower)
📖	Linting Nimble with this PR took 1.42s vs 1.38s on master (2% slower)
📖	Linting Quick with this PR took 0.45s vs 0.44s on master (2% slower)
📖	Linting Realm with this PR took 2.2s vs 2.11s on master (4% slower)
📖	Linting SourceKitten with this PR took 0.82s vs 0.79s on master (3% slower)
📖	Linting Sourcery with this PR took 3.78s vs 3.59s on master (5% slower)
📖	Linting Swift with this PR took 10.39s vs 9.92s on master (4% slower)
📖	Linting WordPress with this PR took 9.71s vs 9.21s on master (5% slower)

Generated by 🚫 Danger

[codecov-io]

Codecov Report

Merging #1745 into master will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #1745      +/-   ##
==========================================
- Coverage   87.75%   87.73%   -0.02%     
==========================================
  Files         218      218              
  Lines       10704    10707       +3     
==========================================
+ Hits         9393     9394       +1     
- Misses       1311     1313       +2

Impacted Files	Coverage Δ
...ntFramework/Extensions/Configuration+Merging.swift	87.34% <33.33%> (-2.14%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54d6199...df82148. Read the comment docs.

[SDGGiesbrecht]

@marcelofabri, @jpsim

I looked at this in case it would affect #1748. (Apparently we wrote some of the same code without communicating. Haha...)

Unfortunately, I think there is more to the bug than this. The related bug report (#1744) states that the bug was already manifest in 0.21.0. The code affected by this pull request did not even exist in 0.21.0, so I doubt these changes actually get to the root of the problem. (They will not fix it for any situation that does no merging.)

I advise verifying that 0.21.0 really does exhibit the bug before proceeding.

[SDGGiesbrecht]

...before proceeding.

I mean before assuming the bug is gone. There is nothing wrong with merging the changes proposed here.

[marcelofabri]

The code affected by this pull request did not even exist in 0.21.0, so I doubt these changes actually get to the root of the problem. (They will not fix it for any situation that does no merging.)

Actually, I think is not related to the merge of configurations. It's just how we look to parent directories if we don't find one configuration in the current one.

[marcelofabri]

I'm not sure this actually works with --config.

[marcelofabri]

this test should pass, right?

[SDGGiesbrecht]

@marcelofabri,

For starters, Configuration’s Equatable conformance is broken on master, so XCTAssertEqual is unreliable here. (See the comments in #1748 starting with this one). Although that may not be the only issue since there are also failures on macOS.

[marcelofabri]

This happens locally on my Mac. I'm pretty sure it's not a bug of comparison, but a consequence of the bug that this PR fixes. However, this PR currently only fixes it for implicit configs.

[SDGGiesbrecht]

Actually, I this is not related to the merge of configurations. It's just how we look to parent directories if we don't find one configuration in the current one.

...which was all refactored to integrate with merging (searching now needs to continue past the first configuration found). The edits here are all in Configuration+Merging.swift.

[marcelofabri]

..which was all refactored to integrate with merging (searching now needs to continue past the first configuration found). The edits here are all in Configuration+Merging.swift.

They were refactored just to be in different extensions.

[marcelofabri]

No idea why I'm getting different results on Linux and macOS 🤔

[SDGGiesbrecht]

@jpsim, @marcelofabri

I’ve thought more about this. I think this is a broader issue than we initially realized. While the bug addressed here predated configuration merging, merging is now live in master, and configuration merging broke the very design of --path and --config (though it was definitely worth it). Now real thought has to go into what they are supposed to do before trying to fix them for the next release. Each had several different uses that were more or less identical before merging, but are now mutually exclusive. It all comes down to the fact that there are now two distinct paths involved, (a) what scope to lint and (b) where is the root of the configuration tree?

What does --path mean?

A)

User John Doe normally runs swiftlint lint in his project root. But at the moment he just wants to see what is wrong in a particular module and filter out the rest. He runs swiftlint lint --path Sources/MyModule/ expecting to get the same results as normal for that module. (There is a real usage case in #987.)

A possible clearer name might be --subdirectory:
swiftlint lint --subdirectory Sources/MyModule/.

He needs the configurations to inherit down to his project root (the working directory) and no further in order to produce results consistent with his normal broader use of swiftlint lint.

Using this to point outside the working directory would be a precondition failure, as it would—unavoidably—inherit all the way from the system root.

B)

User Jane Doe does not want to have to cd to her project root and back again afterward. She wants to be able to specify the location of the project. She runs swiftlint lint --path Documents/Projects/MyProject/ expecting to get the same results as her CI which runs in the project root. (There is a real usage case in #1744.)

A possible clearer name might be --project:
swiftlint lint --project Documents/Projects/MyProject/.

She needs the configurations to inherit down to the specified directory and no further.

She will think it is a bug if they inherit from her working directory (home), which is outside the project. However, that “bug” was exactly what John Doe above expected, and he will think it is a “bug” if it does not happen.

(Note however that both --project and --subdirectory might be used together to do what John Doe wants, but from somewhere else in the file system.)

C)

User Max Mustermann is integrating SwiftLint into an IDE. He wants to be able to feed SwiftLint arbitrary code even if it is still just in some temporary directory and the user has not saved it yet. His IDE runs swiftlint lint --path tmp/TransientFile.swift expecting no interference from configurations anywhere in the file system (though it might provide one to the command line directly). (There is a real usage case in #1721.)

A possible clearer name might be --isolated-file:
swiftlint lint --isolated-file tmp/TransientFile.swift

Contrary to either John or Jane Doe, Max will consider it a bug if a configuration file gets imported from the same directory as the file he provided.

A, B and C have mutually exclusive behaviour and can no longer share the same option.

What does --config mean?

A)

User Erika Mustermann wants to be able to specify distinct configurations under different conditions. She has two configurations: .swiftlint‐local.yml and .swiftlint‐ci.yml. She specifies which one she wants at the invocation in each script: swiftlint lint --config .swiftlint‐local.yml or swiftlint lint --config .swiftlint‐ci.yml. She is less strict about the Tests directory, so she has a nested configuration there that she expects both invocations to respect. (There is a real usage case in #799.)

This would be covered by --config-defaults as implemented in #1748:
swiftlint lint --config-defaults .swiftlint‐local.yml
swiftlint lint --config-defaults .swiftlint‐ci.yml

She will think it is a bug if her nested file does not merge on top of the file she specifies.

B)

User Jean Dupont reads the current description for --config, “the path to SwiftLint's configuration file” and naturally believes it will redirect SwiftLint away from the .swiftlint.yml files littered throughout the project. He uses --config with just that intention: swiftlint lint --config ~/Documents/MyRules.yml.

This would be covered by the combination of --config-defaults and --ignore-nested-configs as implemented in #1748:
swiftlint lint --config-defaults ~/Documents/MyRules.yml --ignore-nested-configs

He will be very confused if anything is merged and he will think there is a bug. If enough things are overridden, it may even appear to him as though --config did nothing at all.

A and B have mutually exclusive behaviour and can no longer share the same option.

Possibilities for Moving Forward

None of the above usage styles work as expect from master right now. Neither would any of them fully work after merging this pull request. (Though it is not very noticeable yet, since so far no one is using SwiftLint on projects full of nested configurations designed to be merged.)

Moving forward one might:

A)

Do nothing. (Possibly merging this pull request, but nothing more.)

• Brace for mountains of bug reports at the next release.

B)

Stick to --config and --path:

• Choose one definition from above to lock down the semantics.
• Make the help description more explicit.
• Re‐implement the options so they actually do what they purport to do.
• Inform users who assumed differently that their usage will no longer work.

C)

Split the options up for each usage, making --config and --path obsolete:

• Merge Command Line Options for Interacting with the Configuration Merging Process #1748 and then implement --project, --subdirectory and --isolated-file.
• Either:
  • a) Remove --config and --path to prevent confusion, or:
  • b) Keep them around for pseudo‐compatibility and route them each straight into one of the above clearer options.

In my opinion C)‐a) is the best option, but no matter how you decide to proceed, it will be easier to do after #1748 than before it, as #1748 fixes several existing bugs (like the Linux one @marcelofabri encountered) and does most of the work needed for several of these options.

[lswith]

This is an interesting use case for both config and path. I think you should have a look at a few popular linters to figure out the best use case for these configurations. This problem has been solved before.

[marcelofabri]

as #1748 fixes several existing bugs (like the Linux one @marcelofabri encountered)

I don't think it's the same bug. If you look the log for this PR, the configurations are totally different (not just the order of rules).

[SDGGiesbrecht]

If you look the log for this PR, the configurations are totally different (not just the order of rules).

Sorry. I did not look at this log that close. You’re right then that there is another problem in addition to the one I mentioned.

The left side is the configuration for SwiftLint, the right side is the configuration for the mock project.

Does deletingLastPathComponent actually do anything on Linux? It is ringing a bell as one of the methods that gave me grief because it was not implemented yet. (I wish such methods would crash and say so instead of silently returning self, nil or something useless.) It may have been fixed in Swift 3.1, but it is as good a place to start as any.

[marcelofabri]

Should we continue the discussion in #1693?

[SDGGiesbrecht]

Should we continue the discussion in #1693?

They are only loosely related. This is about fixing --path, no? (I mentioned --config because you did, but in my opinion it is a separate issue.)

I like the code you have written, @marcelofabri. It is a great improvement to predictability. I honestly suspect the only thing still stopping the tests is that deletingLastPathComponent has to been done manually in Swift 3 on Linux.

I was just aware that people are already using --path in other ways, so a lot of existing usage is broken now, not just the specific case you discovered (the examples did not come out of nowhere; see the linked issues). You do not have to cater to such usage, it is just useful to be aware of it as you work instead of discovering afterward. (In my experience, I often I wish I had known such things sooner.)

As for me, I do not—and probably never will—have a need to use --path, so I am satisfied any way you go.

I apologize if this has been frustrating. I truly wish you a nice day, @marcelofabri.

[marcelofabri]

(I mentioned --config because you did, but in my opinion it is a separate issue.)

Thanks for that detailed comment BTW. It made me realize that we might be overcomplicating things (see #1693 (comment)).

I honestly suspect the only thing still stopping the tests is that deletingLastPathComponent has to been done manually in Swift 3 on Linux.

I'm not so sure: we use deletingLastPathComponent in other places and quickly browsing https://github.com/apple/swift-corelibs-foundation gave me the impression that it's implemented. I didn't have the time to investigate deeper yet.

I apologize if this has been frustrating. I truly wish you a nice day, @marcelofabri.

Not at all! I just wanted to keep the discussion here focused on that single issue. I do think we have issues with how configurations work (see all the issues in the previous releases).

[marcelofabri]

If I disable the configuration (memory) cache, the tests pass on Linux 🤔

[marcelofabri]

So, turns out mutability causes issues 😅
Fixed in d4248e9.