[PITCH] compilerSettings: a top level statement for enabling compiler flags locally in a specific file

Evolution Pitches
21

One major concern I have is that if we allow these flags to change parsing (which is currently the case if allowing experimental/upcoming features), we then lose the ability to parse files without a round trip with the compiler. IMO we should either:

  1. Not allow any settings that can modify the parsing behavior, or
  2. Implement the evaluation of these settings within the parser itself (preferably with eg. macro expansion-like syntax as @woolsweater has suggested)

(2) would suggest that it cannot be wrapped in a #if. Though as @scanon mentioned, that would mean there's no ability to stage in this feature for projects that need to support older compilers.

Related (though irrelevant if we do (2)), but the stdlib seems like the wrong place for this type - we'd be imposing settings from the SDK for the target platform on the current compilation, which feels very wrong (CC @etcwilde).

Lastly, for the settings themselves, how do we avoid these looking like a bunch of random settings as far as a user is concerned? It seems like they could end up being a very confusing set if we eg. allow some experimental/upcoming features, but not all.

7 Likes
22

Then as part of the implementation, it’d be really useful to also provide negative versions of the flags that are being supported by this directive, in case a file wants to opt out of a module-level setting like in my original comment.

3 Likes
23

Do we have any experimental features that affect parsing in a fundamental way? IMO it’s generally better to always parse the new syntax and then reject it in type checking if the new feature is not enabled.

2 Likes
24

Fixed. Thank you!

What I was imagining was adding state to Features.def that let features explicitly opt into this. I would also be fine with it being explicit as well.

The proposal is actively trying to avoid needing to add support for compilerSettings in that manner. ExistentialAny should not impact this since it does not impact actual behavior. If FullTypedThrows cannot be summarized via its impact by modifying the AST, then it would not be supported. I am not completely familiar with the type checking impacts, so I am not sure off the top of my head.

The proposal is actively trying to avoid doing that since we want to make it easy to know where to look for the command line settings. Also many command line settings only make sense to apply to an entire file at a time... so we would be introducing another way features need to be classified... so I would rather at least not do it and at most leave it as a future direction.

We considered this syntax and decided to avoid it since that looks like a macro which this is not. I would be happy to add that to alternatives considered.

This capability is one that is already handled via the @_optimize attribute.

The actual settings will be applied during type checking... so a compiler setting that impacts parsing would not be eligible. I can add that explicitly to the proposal.

True. We may need a little word smithing there.

I don't think this actually would cause any odd circularity. It would just enable Foo.

The intention is that settings that impact parsing cannot be used with compilerSettings since compilerSettings will be applied after parsing.

One thing that I considered doing was making it a completely synthesized type based off of the currently compiling compiler (skipping the SDK issue). Even if we do that though, we need to place it somewhere and I think the Swift module is as good as any other place.

As part of a feature being proposed in swift-evolution, a feature would have to state whether compilerSettings support would be added. If the user is looking at enabling a specific upcoming feature, I think it is reasonable to expect the compunction to come from either a migration guide or swift evolution. There are already many experimental/upcoming features and users already only know about some of them generally when we suggest that they enable them. Why is this any different?

1 Like
25

My understanding is that Swift uses “#” to denote things that happen at compile-time, such as “#if” and “#available”, and this aligns with the historical use of “#” in C to denote compiler directives.

The fact that macros begin with “#” follows as a result, because macros are expanded at compile time.

Thus it seems entirely natural to specify compiler settings by writing “#compilerSettings”, and indeed it would seem inconsistent to create a compile-time feature which goes in source code but doesn’t start with “#”.

26 Likes
26

So, at the top of each file that you want the compiler to behave differently on, write the code chunk. Got it. That was certainly hinted at, I just wasn't sure. Thank you.

++ on the future directions on adding this to SwiftPM SwiftSettings/interacting with Package.swift somehow as I would potentially like a version where the files could be listed and annotated there when using SwiftPM, so all compiler related info could be in one place, but that's a personal organizational preference.

ETA NOTE: I found this page instructive. Thanks for writing it up! swift/SwiftCompilerSources/README.md at main · swiftlang/swift · GitHub

27

How does this work if certain flags only work when building for certain platforms? Is there a way that, if lets say someone hard-codes one of these into their file and it doesn't work on the platform that I'm building for, can I turn it off at the swiftc/build-system level?

Does this mean that it only supports flags that cannot have an impact on the interface? Do these flags get encoded into the swiftmodule/textual swift interface? If so, what does that look like? I see that there is the experimental feature flag. Is it using the same condfail mechanisms to prevent broken textual swift interfaces?

Still trying to digest how I feel about not being able to look at all of the flags applied during a build by looking at the build log, but also having to check out the sources at a specific commit to determine what was actually built.

Finally, it calls out adding the ability to SPM to pass flags to specific files through the package manifest. What does it look like to add flags to specific files from the command line?

2 Likes
28

This looks nifty on a per-file basis but could this be a pathway to an inline enable-disable mechanism? .NET pragma warning disable/restore is familiar to me. [Edit: missed the comment above already pointing this out]

The most immediate example I can come up is that this would allow per-warning suppression with existing flags:

// compiled with normal warnings elsewhere

#enableCompilerSettings(.suppressWarnings)

functionThatEmitsAWarningButICanIgnoreBecauseImTheEngineer()

// other code that emits warnings but I'm the engineer I can ignore them

#disableCompilerSettings(.suppressWarnings)

I also don't see disableUpcomingFeature/disableExperimentalFeature. I can't come up with a good case for the former, but flexibility on the latter could be useful if one has an experimental feature for the module but doesn't want it for the current file.

29

Love the idea, dislike the syntax.

I agree with @Nevin and @woolsweater that:

#compilerSettings(
  .warningAsError(.concurrency),
  .strictConcurrency(.complete),
  .enableUpcomingFeature(.existentialAny),
  .defaultIsolation(MainActor.self),
)

Is a more intuitive syntax - a build time directive.

whereas

compilerSettings [
    .warningAsError(.concurrency),
    .strictConcurrency(.complete),
    .enableUpcomingFeature(.existentialAny),
    .defaultIsolation(MainActor.self),
]

just looks.... wrong. It looks like a strangely declared variable to be used at runtime. I guess the omission of = was chosen to minimize the feel of a variable? However, for me it just makes it look even more wrong.

Furthermore I don't think compilerSettings [ is required, since I guess I prefer Rust syntax:

#![allow(unused_imports)]
#![feature(let_chains)]

Which I think is a pretty accurate approximation of the Rust equivalence of what is being pitched here for Swift - a per file controlling of building settings, above a mixture of feature flags (per file) and linting config. So perhaps we could do simply:

#warningAsError(.concurrency)
#strictConcurrency(.complete)
#enableUpcomingFeature(.existentialAny)
#defaultIsolation(MainActor.self)

For me that (or some alternative of it) is the best syntax, for these reasons:

  • compilerSettings is never declared - if we need to learn how to do per-file build settings it is not harder to learn to use #warningAsError(.concurrency) than the compilerSettings [ variant - so it is clear it is not a variable which we can use at runtime
  • Less indentation
  • # make it clear it happens at build time - not runtime, and...
  • ... the extra verbosity of # per line is "paid for" by omission of leading . and trailing ,
  • Can be expanded to not only be compiler settings - it is more general syntax which can be adopted by linters, just like Rusts #![allow(unused_imports)]
  • This has precedence in Rust community - not a bad thing!

Alternative spelling is grouping the settings a bit:

#compiler(.warningAsError(.concurrency))
#compiler(.strictConcurrency(.complete))
#compiler(.enableUpcomingFeature(.existentialAny))
#compiler(.defaultIsolation(MainActor.self))
#linter(.snakeCase)
15 Likes
30

This is a good point that should be addressed in the pitch: how will these flags be surfaced during the build if the user passes in -v to see the verbose build output? For proper auditability, these per-file flags should definitely be printed out, so it all depends how this pitch will be implemented.

I see three possibilities for implementation:

  1. Implement this in the separate swift-driver executable: this is unlikely because I don't think the swift-driver touches the Swift files to be compiled now, but it would make exposing the flags easy, as they'd simply be passed to the separate swift-frontend invocation.
  2. Have the swift-frontend add these flags itself: if so, it should definitely always dump out whatever flags it sees in these Swift files if -v was passed in.
  3. Have the swift-driver query the swift-frontend to parse the flags from Swift files to be compiled: this is already currently done for a different purpose by the -print-target-info flag, which passes that info back in JSON format, it could be done here too. This leaves the parsing to the swift-frontend and the CLI flag-handling to the swift-driver, so it keeps those responsibilities separated as they are today.

I personally have little preference to which implementation route is chosen, but all three should definitely dump out what they're doing with the -v flag.

As for the pitch itself, the feature is clearly worthwhile and I can see the advantages of putting the flags in the file itself, ie this won't depend on any build system, but I don't have experience with a programming language putting build flags in the files themselves, so that will be weird for me and many other users. That is why auditability will be very important.

31

Some experimental features are source breaking for SwiftSyntax users. For example, when DoExpressions feature enabled, SwiftParser parses do { .. } as a DoExpressionSyntax instead of DoExpressionStmt, For current users handling SyntaxVisitor.visit(_ node: DoExpressionStmt) looses the way to handle it unless enabling a @_spi because DoExpressionSyntax is invisible without the SPI.

Also some features are fundamentally for Parser only. E.g. TrailingComma

4 Likes
32

Somebody mentioned it in passing earlier, but I think it's important to note that a common use-case would be something like

#if swift(>=6.x)
compilerSettings [
    ...
]
#endif

However, #if blocks generally have to parse in all versions of the compiler, even if they can only be analyzed in one version. So unless this proposed syntax would parse (I think it would complain about a missing semicolon), we should adopt a syntax that is more backward-compatible. I like the previously-proposed option of

#if swift(>=6.x)
#compilerSettings([
    ...
])
#endif

(presuming that the older compiler won't freak out when it finds no such macro)

1 Like
33

This doesn't appear to be true with #if swift or #if compiler. For example, if I compile this with the latest toolchain, it works with -swift-version 5 and diagnoses two errors with -swift-version 6:

#if swift(>=6)

compilerSettings [ ... ]

func

#endif

However, only the func is actually a parser error. The expression compilerSettings [ ... ] parses successfully today, as a subscript call with a single argument value that references the symbol ...:

              (subscript_expr type="<null>"
                (unresolved_decl_ref_expr type="<null>" name="compilerSettings" function_ref=unapplied)
                (argument_list
                  (argument
                    (unresolved_decl_ref_expr type="<null>" name="..." function_ref=unapplied)))))))))))

This also means that the following is totally valid code today:

struct compilerSettings {
  static subscript(_: Int...) -> Int { return 0 }
}

compilerSettings [1, 2, 3, 4]

So this proposal, in its current form, is source breaking.

10 Likes
34

I have to admit, I kind of hate this.

I hate it conceptually because it's the kind of boilerplate we've always tried to avoid. I worry that people (or templates!) will defensively put compilerSettings at the top of every file, whether it needs it or not.

I also hate this particular design because it's incredibly wordy and syntactically complex. In most languages I have experience with, common pragmas at least have the virtue of being short. For example, I haven't written much Perl in the last twenty years, but I still have the pragmas you write at the beginning of a file memorized:

use strict;
use warnings;

Compare to something that uses a bunch of camel-case names and requires you to mix square brackets (in a really weird usage for the language!), parentheses, dots, and commas in a fairly arbitrary order. This is actually a pretty difficult syntax to use correctly without code completion.

I hate that this complexity isn't even buying us the benefit of using existing syntactic structures. If the proposal were for a built-in magic macro, you could at least justify the wordy, punctuation-heavy syntax as leveraging existing designs. But instead, we have something that's just novel enough to need a new syntax without actually taking advantage of the design space that a custom syntax opens up.

And—though this is admittedly peevish—I hate that compilerSettings is an introducer keyword with a capital letter. Didn't we decide not to do that with associatedtype?

I'm also concerned about some of the settings you want to permit people to change. Is every upcoming feature going to work correctly when enabled on a per-file basis? Do we really want people to specify a long list of warning categories to disable in every source file?

I think we should take a very hard look at which compiler settings we actually need to be able to change per file and which ones we simply think would be nice to change if we have a feature that could accommodate it. If we find that, for instance, the only thing we really need is main actor mode, maybe we could invent a one-off syntax specifically for that purpose and call it a day.

import DefaultMainActor   // unprincipled but lightweight

If we do conclude that a flexible feature is the right move, I think we should decide on a design direction—either a really nice bespoke syntax or a syntax that reuses existing language constructs—and then lean into that instead of building something that has the worst of both worlds.

34 Likes
35

I completely agree with this. To my mind, the settings that are crucial to support per-file are:

  • Default actor isolation
  • Strict concurrency level
  • Strict memory safety
  • Warning control

That's it. I think that upcoming and experimental features would be nice to allow, but because this feature cannot support arbitrary experimental and upcoming features, it might not be worth it. Having to understand whether a particular upcoming feature changes parsing or changes type inference in a way that cannot be made explicit in swiftinterface printing will not lead to a great programming model.

13 Likes
36

The capability itself is quite welcome, being able for a file to opt into some feature or mode is useful, however I'm pretty wary of this spelling of it.

I'll start with what feels not quite right with spelling these as "compiler settings":

  • it looks like top level code. Like some function call, but it magically isn't;
    • Do we really need some arbitrary one off syntax for this?
    • I'm very uncomfortable with special syntax for something that totally looks like "normal top level statements" to actually be compile time configuration, and it doesn't really seem like this specific spelling really gets us some specific benefit (?) like IDE tooling etc, they'd still need to be thought that this is "special"
    • @Sajjon makes good points here that I mostly agree with and will expand on below; and @Slava_Pestov also brings up the potential for source break (even if technically unlikely)
  • I don't think we should be focusing on the "compiler" angle of these settings (in the naming)
    • The fact that these are eventually turned into some set of flags/options passed to the compiler frontend feels somewhat insignificant, we're talking about aspects of the language and how to influence it IMHO

"Importing" modes and features?

There exists some similar prior art in other languages that may be worth leaning into a bit:

Scala has a feature called language imports, where one would "import language.{something}" language features. It's been part of Scala for a long time, since 2.12, proposal here: SIP: Modularizing Language Features - Google Docs and features it enables are e.g. (just to give an idea):

  • existentials - enables writing existential types
  • higherKinds - enables writing higher-kinded types
  • ...
  • experimental - contains newer features that have not yet been tested in production
  • etc.

So since import can use . to dig into objects, a typical enabling of some feature might look like:

// scala reference example
import language.experimental.something

One problem that might (or might not) appear with this is that it would be weird to pass arguments to these options. While for something like language.warningsAsErrors and language.warningsAsErrors.concurrency this might be ok since we can do namespaces (if we extended import to survive multiple ., I don't think it can today AFAIR). But the more I think about it the less I am concerned, these options should not become overly complex and just treating them as on/off flags may be better anyway -- and this way we stop ourselves from inventing very complex modes...

Another alternative to think about, if we want to make sure this is distinct from "just normal imports", we already have such way to classify imports: import struct is a thing, so we could do

import feature defaultIsolation.mainActor

or similar, to signify what this is doing. This would be my preferred way precisely because it is limited to be simple and we're extending an existing notion in the language.

The reason to consider import for this is because:

  • import is well understood as affecting how a file is interpreted, and to me (and prior art mentioned above) it really feels natural to build on this
  • I think it is useful to keep the power of this feature limited to be honest... The proposal states .defaultIsolation(MainActor.self), and I'm wondering if that would actually really work... we'd have to typecheck for the passed thing to actually be a global actor etc... Do we actually ever want to allow passing random actors there? That's not been decided at all so I'm not sure we need the power to do so.

Or #feature() to enable things?

If we really think we need to be able to pass values to these options then I'd be inclined to agree with @Sajjon's idea of spelling it like a macro.

I know it is not a macro in the sense of "expanding code", however it is IMHO by far better to have a macro #feature(.defaultIsolation(.something)) that gets a bit of special treatment that the compiler knows about it enabling the passed feature, rather than invent a completely new thing.

Specifically because:

  • #something is understood to be "at compile time"
  • this can be expressed using plain, present-day, Swift then; just normal enums which IDEs will just understand and think "oh yeah it's some macro, fine, whatever".
    • The expansion could be skipped by the compiler since we know this one evaluates to nothing; IDEs which have no idea about it could attempt to expand, but it would just expand to empty (or some comment)
  • we would still apply the limitations where it can appear, just as the current proposal would have to, so that's no different.

Actual rollout thoughts: versioning, deprecating etc

I also worry about the actual use of this in reality then requiring the usual series of #if swift(>=...) ... #endif around each of those... The macro idea actually works out since we'll just get the suggestions based on the value availability by default...

I would also like the proposal to discuss if we'd ever deprecate an option or mode. Deprecating a value in the settings enum can be painful and may force libs into annoying dances like:

#if swift(>=8.0) 
/*.something is default already*/ 
#else 
#feature(.something)`
#endif

So I am thinking if we should in general aim to not deprecate these features even if they become default...? Or I guess use some other warning type other than deprecation, so I could silence those specific kind of warning.

The specific example is a "8.0 supporting library which also supports 7.0 and 6.0" and all those language releases have the setting, but e.g. 8 decided to deprecate it. It would be good to not have to go around my library and in every file make this #if-not dance to avoid warnings...

I was even thinking if we should allow

import feature doesNotExistYet
#feature(.doesNotExistYet)

and cause just a warning... but I think that may be overdoing it... and I guess in those situations we'll have to do the #if dance around the feature or import... That's probably ok.

Either way, I think this will be valuable overall, but I would like to flesh out how we present this. The longer I thought about this the more I was leaning to a #feature() but the import are also a nice way to express it.

13 Likes
37

I don’t think this makes sense. Presumably the doesNotExistYet language feature has some important impact on the code you write, so you’d also have to guard the code you write that makes use of the feature with #if guards.

One way the import feature syntax could be expanded is by allowing #if canImport(feature defaultIsolation.mainActor) to check if the compiler supports the feature. This could be used both around the import and around code that uses the feature.

2 Likes
38

Yeah, it's probably fine to rely on the existing canImport functionality and allow it to be used with importing language features/settings this way :slight_smile:

39

IMO, if we make a short list of compiler settings that should be modifiable per-file, we should leave out strict memory safety until there's demand for it. Strict memory safety is already built to be overlookable enough: there is no diagnostic that you can't silence, and the diagnostics are warnings by default, and the list already includes warning control. Although the proposal hasn't been accepted yet, in its latest form, we affirmed that you could use unsafe even when strict memory safety wasn't enabled, so there isn't much of an adoption phase where that could be necessary either (you can do the unsafe annotation changes locally and commit the code changes without the setting change until everything is ready).

40

I do wonder if default actor isolation is conceptually different enough from the other three items on this list that it might benefit from a different approach to enablement. Strict concurrency level, strict memory safety, and warning control are (if I ignore a lot of nuance) generally speaking all ways of controlling diagnostics, whereas default actor isolation also has a significant runtime behavior component.

If I consider default actor isolation on its own without considering how the other three items should work, one idea which comes to mind is that it could be handled similarly to default literal types. For example, I can write this code today:

private typealias IntegerLiteralType = Int16
let x = 1
print(type(of: x)) // Int16

And something like the following seems at least somewhat aligned from a syntax perspective:

private typealias DefaultActorIsolationType = MainActor

And aligns nicely with the proposed SwiftPM usage of MainActor.self in Add a `defaultIsolation` static method on `SwiftSetting`. by hborla · Pull Request #8301 · swiftlang/swift-package-manager · GitHub

3 Likes