Proposal for #SwiftSettings.
#2699
Conversation
|
Some compiler settings have a security impact and I don't expect the number of these to go down over time. As we get a new way to change settings, it's spreading the source of truth for what settings are or aren't enabled in any given project. Do we enumerate the ways compiler settings can be modified in an Xcode project or in a SwiftPM project anywhere? |
|
Please leave design feedback in the forthcoming pitch thread on the forums - it will be posted soon - and leave editorial feedback on this PR. |
gottesmm
force-pushed
the
compiler-settings
branch
3 times, most recently
from
27d96b4 to
78f02f6
Compare
|
For folks who landed here and are looking for the pitch to comment on: https://forums.swift.org/t/pitch-compilersettings-a-top-level-statement-for-enabling-compiler-flags-locally-in-a-specific-file/77994 |
gottesmm
force-pushed
the
compiler-settings
branch
from
baa657d to
e812ae3
Compare
gottesmm
force-pushed
the
compiler-settings
branch
from
e812ae3 to
dbab768
Compare
rjmccall
added
the
LSG
gottesmm
force-pushed
the
compiler-settings
branch
10 times, most recently
from
b2c6122 to
0034b65
Compare
gottesmm
force-pushed
the
compiler-settings
branch
from
0034b65 to
3029caa
Compare
gottesmm
force-pushed
the
compiler-settings
branch
from
1340b0a to
0143cbf
Compare
gottesmm
force-pushed
the
compiler-settings
branch
3 times, most recently
from
8be7d33 to
37244ba
Compare
gottesmm
force-pushed
the
compiler-settings
branch
from
37244ba to
cf7975e
Compare
hborla
changed the title
compilerSettings.#SwiftSettings.
| these flags one module at a time instead of one file at a time. When confronted | ||
| with this, users are forced to split the subset of files into a separate module | ||
| creating an unnecessary module split. We foresee that this will occur more often | ||
| in the future if new features like [Controlling Default Actor Isolation](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0466-control-default-actor-isolation.md) |
There was a problem hiding this comment.
I think this specific argument isn't compelling, because we really don't want to introduce more features like default actor isolation in the future. I think you can just remove this sentence and say this applies for default actor isolation and also for the various diagnostic control flags.
|
|
||
| ```swift | ||
| extension SwiftSetting { | ||
| public static func defaultIsolation(_ isolation: Actor.Type?) -> SwiftSetting { SwiftSetting() } |
There was a problem hiding this comment.
This should match the package manifest API in SE-0466, which uses MainActor.Type?
| parsing of other constructs since the name shadowing rules in the compiler will | ||
| ensure that any local macros or types that shadow those names will take | ||
| precedence. If the user wishes to still use `#SwiftSettings`, the user can spell | ||
| the macro as `#Swift.SwiftSettings` as needed. |
There was a problem hiding this comment.
Does this work because it's parsed as a freestanding declaration macro? I'm pretty sure that in expression context, #Swift.SwiftSettings is parsed as a #Swift freestanding expression macro with .SwiftSettings as a postfix member. (doesn't really matter for this proposal, only for my curiosity!)
|
|
||
| Adopters of this proposal should be aware that while this feature does not | ||
| inherently break ABI, enabling options using `#SwiftSettings` that break ABI can | ||
| result in ABI breakage. |
There was a problem hiding this comment.
You can be more explicit here - say that changing default actor isolation on public API is not source compatible and not ABI compatible.
| updated state. We think that this approach will make it significantly easier for | ||
| larger modules to be migrated to strict concurrency. | ||
|
|
||
| #### `strictConcurrency` |
There was a problem hiding this comment.
I recommend omitting this section and the next one; since you've left strictConcurrency and warningsAsErrors as a future direction, it's not a good use of time for reviewers to comment on the details of how it would be accomplished in the review thread for this proposal.
| on the command line are passed as parameters to the static method. The compiler | ||
| upon parsing a `#SwiftSettings` macro updates its internal state to set the | ||
| appropriate settings before compiling the rest of the file. An example of such a | ||
| `#SwiftSettings` invocation is the following: |
There was a problem hiding this comment.
I didn't see a mention in the detailed design about the reason why you chose this spelling, which is to match the Swift Package Manifest API so there's a consistent spelling for settings in source code. I recommend putting that either here or in the detailed design.
|
|
||
| We could also use an "import" syntax to enable features like Python and | ||
| Scala. We decided to not follow this approach since `#` is a manner in swift of | ||
| saying "compile time". |
There was a problem hiding this comment.
Further, we'd be introducing a third way of specifying build settings (command line spelling, package manifest spelling, and an import-like spelling). The approach in this proposal was taken primarily to keep a consistent way of spelling build settings in source code.
|
|
||
| We could also use a typealiased based syntax. But this would only be able to be | ||
| used to specify the current global actor and would not let us add additional | ||
| features such as controlling strict concurrency or warnings as errors. |
There was a problem hiding this comment.
More importantly, a type alias does not work for default actor isolation, because we'd need some type to represent nonisolated. It's also easy to get wrong because misspelling a type alias is still valid Swift code, etc.
| settings that semantically can have file wide implications. If we wanted to | ||
| support push/pop of compiler settings it would involve different trade-offs and | ||
| restrictions on what settings would be able to be used since the restrictions | ||
| would necessarily need to be greater. As an example, we could never allow for |
There was a problem hiding this comment.
There's an unfinished sentence here.
Just an initial draft for feedback