Repository navigation
Add project-aware symbol indexing for cross-file formatting - #2722
Conversation
e97fa81 to
ebca58b
Compare
48df8bf to
891e9af
Compare
| } | ||
|
|
||
| /// Provides a conservative module identity from conventional project layout. | ||
| func moduleIdentifier(for fileURL: URL, in root: ProjectRoot) -> String? { |
There was a problem hiding this comment.
Understanding what module code belongs to will be important for rules like redundantPublic. Maybe we add a --module-roots-glob option that lets you specify globs for where module roots are defined in your project?
For example, at Airbnb, our modules all have the form:
ios/features/MyFeatureios/services/MyFeatureServiceios/foundation/MyFeatureFoundationios/core_ui/MyFeatureCoreUI- etc etc etc
We could have a list of globs like ios/features/*, ios/services/* etc for paths that represent module roots.
There was a problem hiding this comment.
I do intend to try to improve the module recognition, but the idea was to start with something conservative. E.g. if the symbol is found anywhere in the project we'll assume for now that the public is not redundant.
| return nil | ||
| } | ||
| return "\(rootPath):\(components[0]):\(components[1])" | ||
| case .xcodeProject: |
There was a problem hiding this comment.
I wonder how hard it would be to ingest the new JSON-based Xcode project file format. Makes sense to not try to ingest the legacy plist format.
|
This is exciting! I don't think we'll be able to use this at Airbnb since we have a custom |
I'm not sure I understand why this is. The index is generated by finding the root of the project and indexing the whole thing, so it shouldn't matter that you are only formatting files that were touched in the diff - unless the way you are doing that prevents swiftformat from seeing the file paths? |
|
I think indexing the entire codebase (60,000 files) on every |
|
I checked out this branch and ran it locally. Take this example SwiftFormat invocation that happens under the hood when I run our DetailsThis command invocation only formats the files that are modified on my branch: Without the project index, the command takes only takes 3.5 seconds (and branches that modify fewer files are even faster). With the project index, the command takes 15 seconds. Adding an extra 10 seconds of overhead to our local lint command (ran in our git pre-commit hook, etc) probably isn't going to fly. Formatting the entire codebase takes 1m 7s with the project index enabled, and 48s with the project index disabled. |
|
@calda is that with caching enabled? or do you disable the cache (or get no benefit from it due to using fresh VM instances on each run or something)? |
|
We have the cache disabled because we were occasionally hitting edge cases (every month or so) where the SwiftFormat command would succeed locally for somebody but fail in CI, and then they would get confused. The performance penalty from disabling the cache was pretty small given our How does the cache interact with the project index? Do we cache the project index between runs? I could see caching the project index between runs making it so that it's fast enough for us to use it in |
|
The project index is stored in the cache, so once it's been computed once it shouldn't need to be recomputed. If you plan to disable the formatting cache but still keep the index cached then we might need to decouple them in some way |
|
I'll benchmark it again with the cache enabled on Monday |
891e9af to
be643e0
Compare
|
With the cache and project index enabled, it still takes 10-11 seconds to run the command from before, compared to <2 seconds with the project index disabled. The cache saves about 5-6 seconds compared to not using the cache, but I think the extra 10 second hit is still probably too much. Of course I do support shipping this, it will be awesome in smaller projects. |
@calda presumably we need to add a |
|
The |
Oh haha, I forgot I already added it 😅 |
be643e0 to
ca790e4
Compare
ca790e4 to
57300be
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2722 +/- ##
===========================================
+ Coverage 95.59% 95.62% +0.02%
===========================================
Files 190 191 +1
Lines 29560 29921 +361
===========================================
+ Hits 28258 28611 +353
- Misses 1302 1310 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Several of SwiftFormat's rules are currently hampered or made unsafe by the limitation that only the contents of the current file can be taken into account.
For rules that do things like symbol renaming, typo correction, self insertion or removal, dead code elimination, etc. this severely limits their usefulness, and often means that they can't be used with 100% reliability due to the possibility of introducing conflicts with another file.
This PR implements the first steps in a feature I've wanted to add for a long time. It creates a project-wide symbol index, similar to the file-level index that several rules already implement. This index is relatively quick to calculate compared with the formatting pass, and the results are cached so that they don't need to be recomputed for files that haven't changed.
This symbol index then allows file-level formatting rules to query the type or visibility of symbols that are declared outside of that file, without increasing the formatting cost or preventing parallel formatting.
Computing the index adds a roughly 5% increase to formatting times on average. The index is only calculated if at least one enabled rule needs it, so it doesn't impact the performance of unit tests, etc.
For now I've used the index to improve the
redundantPublicandredundantSelfrules, but there is potential for much bigger impact in future.