Bump the deployment target to macOS 13 🚀 #957

TTOzzi · 2025-03-07T17:00:28Z

This PR addresses the follow-up work discussed in #950 (comment).

Bump the deployment target to macOS 13
Relax regex restrictions to allow rules containing _ or other special characters
Drop NSRegularExpression and migrate to Swift Regex

TTOzzi · 2025-03-07T17:02:18Z

Package.swift

-    .macOS("12.0"),
-    .iOS("13.0"),
+    .macOS("13.0"),
+    .iOS("16.0"),


I also raised the iOS deployment target to 16 to use Swift Regex.
One thing I'm curious about—does swift-format need to support iOS?

Someone added it a while back. I guess theoretically you could write an editor for iPad and want to format Swift code in it.

Oh, I see! That could be a possible use case.

Sources/_SwiftFormatTestSupport/DiagnosingTestCase.swift

TTOzzi · 2025-03-07T17:22:41Z

It seems that RegexBuilder is not supported in Swift 5.9 on Windows.. 😨

allevato · 2025-03-07T17:06:03Z

Package.swift

-    .macOS("12.0"),
-    .iOS("13.0"),
+    .macOS("13.0"),
+    .iOS("16.0"),


Someone added it a while back. I guess theoretically you could write an editor for iPad and want to format Swift code in it.

Sources/_SwiftFormatTestSupport/DiagnosingTestCase.swift

allevato · 2025-03-07T17:52:15Z

Sources/SwiftFormat/Core/RuleMask.swift

+  /// Regex pattern to match an ignore directive comment.
+  /// - Capture group #1 captures the rule names if `":"` is present.
+  fileprivate func makeRegex() -> Regex<(Substring, Substring?)> {
+    let pattern = #"^\s*\/\/\s*"# + description + #"(?:\s*:\s*(.+))?$"#


If we can't use RegexBuilder here because of platform limitations, it would still be nice to use real regex literals.

I don't remember from the previous PR, but if the description property is only used here, you could lift the regexes up into that instead, something like:

var regex: Regex { switch self { case .node: return /...swift-format-ignore.../ case .file: return /...swift-format-ignore-file.../ }

It would also be great to use named capture groups so that the result tuples elsewhere get labels, to make it more readable.

description is used in OrderedImports to distinguish directives.
Also, after modifying it to use regex literals and running the CI, I found that, as @ahoppen mentioned below, regex literals are not supported on Windows Swift 5.9 (5.10). Given this, it seems best to keep the current implementation as is 🥲

It would also be great to use named capture groups so that the result tuples elsewhere get labels, to make it more readable.

I have utilized the named capture group in the regex to add the label ruleNames.

Every time we try to do some good, Windows is there lurking in the background. 🙁

Thanks for checking!

ahoppen

Nice

ahoppen · 2025-03-07T18:06:48Z

Sources/SwiftFormat/Core/RuleMask.swift

+  /// - Capture group #1 captures the rule names if `":"` is present.
+  fileprivate func makeRegex() -> Regex<(Substring, Substring?)> {
+    let pattern = #"^\s*\/\/\s*"# + description + #"(?:\s*:\s*(.+))?$"#
+    return try! Regex(pattern)


Could you add a comment that we can’t use regex literals here because Windows didn’t have support for regex literals until Swift 5.10 (at least I think it was 5.10)? Just so we don’t go back and wonder why we aren’t using regex literals here.

Sure, I've added it.

- Bump the deployment target to macOS 13 - Relax regex restrictions to allow rules containing _ or other special characters - Drop NSRegularExpression and migrate to Swift Regex

allevato

Thanks!

ahoppen · 2025-03-19T21:19:55Z

This change regressed performance of swift-format by 3% in release mode

To reproduce this regression, run

git clone https://github.com/apple/swift-syntax.git
cd swift-syntax
git checkout -f 509.0.0

.build/release/swift-format lint --measure-instructions --recursive path/to/above/checkout/of/swift-syntax 2> /dev/null

@TTOzzi Could you check if there’s a good way to recover that performance regression?

TTOzzi · 2025-03-19T23:23:15Z

This change regressed performance of swift-format by 3% in release mode

To reproduce this regression, run
git clone https://github.com/apple/swift-syntax.git
cd swift-syntax
git checkout -f 509.0.0

.build/release/swift-format lint --measure-instructions --recursive path/to/above/checkout/of/swift-syntax 2> /dev/null
@TTOzzi Could you check if there’s a good way to recover that performance regression?

Oh, I see! Thank you for letting me know.
I didn't think to check the performance at the time 😓
I'll look into it by this weekend and open a PR 🙇

ahoppen · 2025-03-20T00:07:31Z

Thank you ❤️

TTOzzi requested review from ahoppen, allevato, bnbarham and shawnhyam as code owners March 7, 2025 17:00

TTOzzi commented Mar 7, 2025

View reviewed changes

Sources/_SwiftFormatTestSupport/DiagnosingTestCase.swift Show resolved Hide resolved

TTOzzi marked this pull request as draft March 7, 2025 17:29

allevato reviewed Mar 7, 2025

View reviewed changes

ahoppen reviewed Mar 7, 2025

View reviewed changes

TTOzzi force-pushed the bump-macos-version-13 branch from a51d245 to 9757b67 Compare March 7, 2025 19:07

TTOzzi marked this pull request as ready for review March 7, 2025 19:10

TTOzzi force-pushed the bump-macos-version-13 branch from 9757b67 to 4e51ade Compare March 7, 2025 19:23

Bump the deployment target to macOS 13 🚀

af6f7ab

- Bump the deployment target to macOS 13 - Relax regex restrictions to allow rules containing _ or other special characters - Drop NSRegularExpression and migrate to Swift Regex

TTOzzi force-pushed the bump-macos-version-13 branch from 4e51ade to af6f7ab Compare March 7, 2025 19:24

allevato approved these changes Mar 7, 2025

View reviewed changes

allevato merged commit 67983dd into swiftlang:main Mar 7, 2025
19 checks passed

TTOzzi deleted the bump-macos-version-13 branch March 7, 2025 19:51

allevato mentioned this pull request Mar 20, 2025

Add performance measurements to PR checks? #967

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump the deployment target to macOS 13 🚀 #957

Bump the deployment target to macOS 13 🚀 #957

TTOzzi commented Mar 7, 2025

TTOzzi Mar 7, 2025

allevato Mar 7, 2025

TTOzzi Mar 7, 2025

TTOzzi commented Mar 7, 2025 •

edited

Loading

allevato Mar 7, 2025

allevato Mar 7, 2025

allevato Mar 7, 2025

TTOzzi Mar 7, 2025 •

edited

Loading

TTOzzi Mar 7, 2025

allevato Mar 7, 2025

ahoppen left a comment

ahoppen Mar 7, 2025

TTOzzi Mar 7, 2025

allevato left a comment

ahoppen commented Mar 19, 2025 •

edited

Loading

TTOzzi commented Mar 19, 2025

ahoppen commented Mar 20, 2025

Bump the deployment target to macOS 13 🚀 #957

Bump the deployment target to macOS 13 🚀 #957

Conversation

TTOzzi commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TTOzzi commented Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TTOzzi Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahoppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allevato left a comment

Choose a reason for hiding this comment

ahoppen commented Mar 19, 2025 • edited Loading

TTOzzi commented Mar 19, 2025

ahoppen commented Mar 20, 2025

TTOzzi commented Mar 7, 2025 •

edited

Loading

TTOzzi Mar 7, 2025 •

edited

Loading

ahoppen commented Mar 19, 2025 •

edited

Loading