Skip to content

SwiftIfConfig improvements to suit usage in the compiler #2819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions Sources/SwiftIfConfig/BuildConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
//
//===----------------------------------------------------------------------===//
import SwiftSyntax

/// Describes the ordering of a sequence of bytes that make up a word of
/// storage for a particular architecture.
Expand Down Expand Up @@ -114,15 +115,15 @@ public protocol BuildConfiguration {
/// information, which will translate into the `version` argument.
///
/// - Parameters:
/// - importPath: A nonempty sequence of identifiers describing the
/// imported module, which was written in source as a dotted sequence,
/// e.g., `UIKit.UIViewController` will be passed in as the import path
/// array `["UIKit", "UIViewController"]`.
/// - importPath: A nonempty sequence of (token, identifier) pairs
/// describing the imported module, which was written in source as a
/// dotted sequence, e.g., `UIKit.UIViewController` will be passed in as
/// the import path array `[(token, "UIKit"), (token, "UIViewController")]`.
/// - version: The version restriction on the imported module. For the
/// normal `canImport(<import-path>)` syntax, this will always be
/// `CanImportVersion.unversioned`.
/// - Returns: Whether the module can be imported.
func canImport(importPath: [String], version: CanImportVersion) throws -> Bool
func canImport(importPath: [(TokenSyntax, String)], version: CanImportVersion) throws -> Bool
Comment on lines -125 to +126
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need both the token and the string? Couldn’t we extract the identifier from the token?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Makes sense. I think that explanation in a doc comment would be useful.


/// Determine whether the given name is the active target OS (e.g., Linux, iOS).
///
Expand Down
200 changes: 170 additions & 30 deletions Sources/SwiftIfConfig/ConfiguredRegions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,90 @@
import SwiftDiagnostics
import SwiftSyntax

/// Describes all of the #if/#elseif/#else clauses within the given syntax node,
/// indicating their active state. This operation will recurse into all
/// clauses to indicate regions of active / inactive / unparsed code.
///
/// For example, given code like the following:
/// #if DEBUG
/// #if A
/// func f()
/// #elseif B
/// func g()
/// #elseif compiler(>= 12.0)
/// please print the number after 41
/// #endif
/// #else
/// #endif
///
/// If the configuration options `DEBUG` and `B` are provided, but `A` is not,
/// and the compiler version is less than 12.0, the results will be contain:
/// - Active region for the `#if DEBUG`.
/// - Inactive region for the `#if A`.
/// - Active region for the `#elseif B`.
/// - Unparsed region for the `#elseif compiler(>= 12.0)`.
/// - Inactive region for the final `#else`.
public struct ConfiguredRegions {
let regions: [Element]

/// The set of diagnostics produced when evaluating the configured regions.
public let diagnostics: [Diagnostic]

/// Determine whether the given syntax node is active within the configured
/// regions.
public func isActive(_ node: some SyntaxProtocol) -> IfConfigRegionState {
var currentState: IfConfigRegionState = .active
for (ifClause, state) in regions {
if node.position < ifClause.position {
return currentState
}

if node.position >= ifClause.regionStart && node.position <= ifClause.endPosition {
currentState = state
}
}

return currentState
}
}

extension ConfiguredRegions: RandomAccessCollection {
public typealias Element = (IfConfigClauseSyntax, IfConfigRegionState)
public var startIndex: Int { regions.startIndex }
public var endIndex: Int { regions.endIndex }

public subscript(index: Int) -> Element {
regions[index]
}
}

extension ConfiguredRegions: CustomDebugStringConvertible {
/// Provides source ranges for each of the configured regions.
public var debugDescription: String {
guard let firstRegion = first else {
return "[]"
}

let root = firstRegion.0.root
let converter = SourceLocationConverter(fileName: "", tree: root)
let regionDescriptions = regions.map { (ifClause, state) in
let startPosition = converter.location(for: ifClause.position)
let endPosition = converter.location(for: ifClause.endPosition)
return "[\(startPosition.line):\(startPosition.column) - \(endPosition.line):\(endPosition.column)] = \(state)"
}

return "[\(regionDescriptions.joined(separator: ", ")))]"
}
}

extension IfConfigClauseSyntax {
/// The effective start of the region after which code is subject to its
/// condition.
fileprivate var regionStart: AbsolutePosition {
condition?.endPosition ?? elements?._syntaxNode.position ?? poundKeyword.endPosition
}
}

extension SyntaxProtocol {
/// Find all of the #if/#elseif/#else clauses within the given syntax node,
/// indicating their active state. This operation will recurse into all
Expand All @@ -39,10 +123,13 @@ extension SyntaxProtocol {
/// - Inactive region for the final `#else`.
public func configuredRegions(
in configuration: some BuildConfiguration
) -> [(IfConfigClauseSyntax, IfConfigRegionState)] {
) -> ConfiguredRegions {
let visitor = ConfiguredRegionVisitor(configuration: configuration)
visitor.walk(self)
return visitor.regions
return ConfiguredRegions(
regions: visitor.regions,
diagnostics: visitor.diagnostics
)
}
}

Expand All @@ -56,58 +143,111 @@ fileprivate class ConfiguredRegionVisitor<Configuration: BuildConfiguration>: Sy
/// Whether we are currently within an active region.
var inActiveRegion = true

/// Whether we are currently within an #if at all.
var inAnyIfConfig = false

// All diagnostics encountered along the way.
var diagnostics: [Diagnostic] = []

init(configuration: Configuration) {
self.configuration = configuration
super.init(viewMode: .sourceAccurate)
}

override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
// If we're in an active region, find the active clause. Otherwise,
// there isn't one.
let activeClause = inActiveRegion ? node.activeClause(in: configuration).clause : nil
// We are in an #if.
let priorInAnyIfConfig = inAnyIfConfig
inAnyIfConfig = true
defer {
inAnyIfConfig = priorInAnyIfConfig
}

// Walk through the clauses to find the active one.
var foundActive = false
var syntaxErrorsAllowed = false
let outerState: IfConfigRegionState = inActiveRegion ? .active : .inactive
for clause in node.clauses {
// If we haven't found the active clause yet, syntax errors are allowed
// depending on this clause.
if !foundActive {
syntaxErrorsAllowed =
clause.condition.map {
IfConfigClauseSyntax.syntaxErrorsAllowed($0).syntaxErrorsAllowed
} ?? false
}
let isActive: Bool
if let condition = clause.condition {
if !foundActive {
// Fold operators so we can evaluate this #if condition.
let (foldedCondition, foldDiagnostics) = IfConfigClauseSyntax.foldOperators(condition)
diagnostics.append(contentsOf: foldDiagnostics)
Comment on lines +174 to +175
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do a little bit better here, because there were 4 copies of code like this.


// In an active region, evaluate the condition to determine whether
// this clause is active. Otherwise, this clause is inactive.
// inactive.
if inActiveRegion {
let (thisIsActive, _, evalDiagnostics) = evaluateIfConfig(
condition: foldedCondition,
configuration: configuration
)
diagnostics.append(contentsOf: evalDiagnostics)

// If this is the active clause, record it and then recurse into the
// elements.
if clause == activeClause {
assert(inActiveRegion)
// Determine if there was an error that prevented us from
// evaluating the condition. If so, we'll allow syntax errors
// from here on out.
let hadError =
foldDiagnostics.contains { diag in
diag.diagMessage.severity == .error
}
|| evalDiagnostics.contains { diag in
diag.diagMessage.severity == .error
}

regions.append((clause, .active))
if hadError {
isActive = false
syntaxErrorsAllowed = true
} else {
isActive = thisIsActive

if let elements = clause.elements {
walk(elements)
// Determine whether syntax errors are allowed.
syntaxErrorsAllowed = foldedCondition.allowsSyntaxErrorsFolded
}
} else {
isActive = false

// Determine whether syntax errors are allowed, even though we
// skipped evaluation of the actual condition.
syntaxErrorsAllowed = foldedCondition.allowsSyntaxErrorsFolded
}
} else {
// We already found an active condition, so this is inactive.
isActive = false
}
} 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
#endif

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

foundActive = true
continue
// Determine and record the current state.
let currentState: IfConfigRegionState
switch (isActive, syntaxErrorsAllowed) {
case (true, _): currentState = .active
case (false, false): currentState = .inactive
case (false, true): currentState = .unparsed
}

// If this is within an active region, or this is an unparsed region,
// record it.
if inActiveRegion || syntaxErrorsAllowed {
regions.append((clause, syntaxErrorsAllowed ? .unparsed : .inactive))
// If there is a state change, record it.
if !priorInAnyIfConfig || currentState != .inactive || currentState != outerState {
regions.append((clause, currentState))
}

// Recurse into inactive (but not unparsed) regions to find any
// unparsed regions below.
if !syntaxErrorsAllowed, let elements = clause.elements {
// If this is a parsed region, recurse into it.
if currentState != .unparsed, let elements = clause.elements {
let priorInActiveRegion = inActiveRegion
inActiveRegion = false
inActiveRegion = isActive
defer {
inActiveRegion = priorInActiveRegion
}
walk(elements)
}

// Note when we found an active clause.
if isActive {
foundActive = true
}
}

return .skipChildren
Expand Down
16 changes: 8 additions & 8 deletions Sources/SwiftIfConfig/IfConfigEvaluation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ func evaluateIfConfig(
}

// Extract the import path.
let importPath: [String]
let importPath: [(TokenSyntax, String)]
do {
importPath = try extractImportPath(firstArg.expression)
} catch {
Expand Down Expand Up @@ -502,7 +502,7 @@ func evaluateIfConfig(
return checkConfiguration(at: call) {
(
active: try configuration.canImport(
importPath: importPath.map { String($0) },
importPath: importPath,
version: version
),
syntaxErrorsAllowed: fn.syntaxErrorsAllowed
Expand Down Expand Up @@ -540,22 +540,22 @@ extension SyntaxProtocol {
}

/// Given an expression with the expected form A.B.C, extract the import path
/// ["A", "B", "C"] from it. Throws an error if the expression doesn't match
/// this form.
private func extractImportPath(_ expression: some ExprSyntaxProtocol) throws -> [String] {
/// ["A", "B", "C"] from it with the token syntax nodes for each name.
/// Throws an error if the expression doesn't match this form.
private func extractImportPath(_ expression: some ExprSyntaxProtocol) throws -> [(TokenSyntax, String)] {
// Member access.
if let memberAccess = expression.as(MemberAccessExprSyntax.self),
let base = memberAccess.base,
let memberName = memberAccess.declName.simpleIdentifier?.name
{
return try extractImportPath(base) + [memberName]
return try extractImportPath(base) + [(memberAccess.declName.baseName, memberName)]
}

// Declaration reference.
if let declRef = expression.as(DeclReferenceExprSyntax.self),
let name = declRef.simpleIdentifier?.name
{
return [name]
return [(declRef.baseName, name)]
}

throw IfConfigDiagnostic.expectedModuleName(syntax: ExprSyntax(expression))
Expand Down Expand Up @@ -794,7 +794,7 @@ private struct CanImportSuppressingBuildConfiguration<Other: BuildConfiguration>
return try other.hasAttribute(name: name)
}

func canImport(importPath: [String], version: CanImportVersion) throws -> Bool {
func canImport(importPath: [(TokenSyntax, String)], version: CanImportVersion) throws -> Bool {
return false
}

Expand Down
Loading