SwiftIfConfig improvements to suit usage in the compiler #2819
Conversation
The compiler's heuristics for inactive vs. unparsed regions differ from the implementation in SwiftIfConfig and have changed fairly recently. Rework the implementation here to match what the compiler does.
Move the "isActive" check for configured regions into this new struct, and also capture all of the diagnostics.
…ons. The compiler's semantics for determining unparsed vs. inactive regions require looking all the way up the `#if` tree to the root, and the bottom-up implementation of `isActive(in:)` was failing to match the semantics of configured regions. Use `ConfiguredRegions` directly, with a note that the performance of `SyntaxProtocol.isActive(in:)` is not good. I'm still considering whether this API should go away entirely.
…anImport() The compiler produces diagnostics for `canImport` checks, which requires more source location information than was previously provided via `BuildConfiguration.canImport`. Add in TokenSyntax nodes for each of the elements in the import path so that the compiler has the syntax node information it needs.
|
@swift-ci please test |
Add a debugDescription so it's easier to see what the regions are, and allow us to test for exact results. Tweak the set of configured regions more precisely so they match what the compiler is expecting. The only difference here is that we create a separate region for an active block within another active block.
|
@swift-ci please test |
|
@swift-ci please test Windows |
| func canImport(importPath: [String], version: CanImportVersion) throws -> Bool | ||
| func canImport(importPath: [(TokenSyntax, String)], version: CanImportVersion) throws -> Bool |
There was a problem hiding this comment.
Why do we need both the token and the string? Couldn’t we extract the identifier from the token?
There was a problem hiding this comment.
The identifier on the token is optional, so we'd be asking clients to either force the optional every time, or add a spurious "guard". I'd rather just duplicate the information.
There was a problem hiding this comment.
Ah, I see. Makes sense. I think that explanation in a doc comment would be useful.
| let (foldedCondition, foldDiagnostics) = IfConfigClauseSyntax.foldOperators(condition) | ||
| diagnostics.append(contentsOf: foldDiagnostics) |
There was a problem hiding this comment.
This is now duplicating logic from IfConfigDeclSyntax.activeClause(in:), right? Do you think there’s any way to avoid the duplication? I’m a little worried that they will get out of sync eventually.
There was a problem hiding this comment.
We can do a little bit better here, because there were 4 copies of code like this.
| } else { | ||
| // This is an #else. It's active if we haven't found an active clause | ||
| // yet and are in an active region. | ||
| isActive = !foundActive && inActiveRegion |
There was a problem hiding this comment.
Do we still need to determine syntaxErrorsAllowed in that case. Eg. assuming that DEBUG is set, do we report print debugDescription as syntaxErrorsAllowed for the following
#if DEBUG
print(debugDescription)
#elseif compiler(>99.0)
print debugDescription
#endifThere was a problem hiding this comment.
The compiler doesn't determine syntaxErrorsAllowed here, and prints an error for print debugDescription. We could choose to deviate from the compiler here (and be more lax) if we want to.
There was a problem hiding this comment.
OK, that’s surprising to me but matching the compiler here for now makes sense for now to make switching between the two implementations easier.
Should we gather these oddities that we find somewhere so we can consider fixing them in the future?
There was a problem hiding this comment.
It turns out that this behavior changed in the compiler very recently (swiftlang/swift#74415). We could consider whether we need to fix the original issue in some other way.
A couple more semantic tweaks and improvements to address the needs of the compiler:
isActiveoperationSyntaxProtocol.isActive(in:)in terms of configured regions, and warn that it's slowTokenSyntaxnodes for the import path inBuildConfiguration.canImportbecause the compiler really needs location information