Skip to content

Commit f5e3540

Browse files
DougGregorDougGregor
committed
Teach active-clause visitors to handle diagnostics
When the active-clause visitors encounter an error during evaluation of the `#if` conditions, translate the errors into diagnostics and and visit all of the clauses (because we don't know which is active). This behavior is configurable by ActiveSyntax(Any)Visitor subclasses.
1 parent 7ce4981 commit f5e3540

File tree

3 files changed

+143
-64
lines changed

3 files changed

+143
-64
lines changed

‎Sources/SwiftIfConfig/IfConfigVisitor.swift

Lines changed: 65 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
1010
//
1111
//===----------------------------------------------------------------------===//
12+
import SwiftDiagnostics
1213
import SwiftSyntax
1314

1415
/// A syntax visitor that only visits the syntax nodes that are active
@@ -36,29 +37,48 @@ import SwiftSyntax
3637
///
3738
/// All notes visited by this visitor will have the "active" state, i.e.,
3839
/// `node.isActive(in: configuration)` will evaluate to `.active` or will
39-
/// throw.
40-
///
41-
/// TODO: This visitor currently swallows errors uncovered while checking `#if`
42-
/// conditions, which is deeply unfortunate. We need a better answer here.
40+
/// throw. When errors occur, they will be reported via a call to
41+
/// `reportEvaluationError`, which can report the errors (the default is to
42+
/// turn them into diagnostics that go into the `diagnostics` array) and then
43+
/// choose whether to visit all of the `#if` clauses (the default) or skip them.
4344
open class ActiveSyntaxVisitor<Configuration: BuildConfiguration>: SyntaxVisitor {
4445
/// The build configuration, which will be queried for each relevant `#if`.
4546
public let configuration: Configuration
4647

48+
/// The set of diagnostics accumulated during this walk of active syntax.
49+
public var diagnostics: [Diagnostic] = []
50+
4751
public init(viewMode: SyntaxTreeViewMode, configuration: Configuration) {
4852
self.configuration = configuration
4953
super.init(viewMode: viewMode)
5054
}
5155

56+
/// Called when the evaluation of an `#if` condition produces an error.
57+
///
58+
/// By default, this records diagnostics from the error into the `diagnostics`
59+
/// array.
60+
///
61+
/// - Returns: Whether to visit the children of the `#if` or not after the
62+
/// error. By default, this function returns `.visitChildren`.
63+
open func reportEvaluationError(at node: IfConfigDeclSyntax, error: Error) -> SyntaxVisitorContinueKind {
64+
let newDiagnostics = error.asDiagnostics(at: node)
65+
diagnostics.append(contentsOf: newDiagnostics)
66+
return .visitChildren
67+
}
68+
5269
open override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
53-
// If there is an active clause, visit it's children.
54-
// FIXME: try? suppresses errors here. How shall we report them?
55-
if let activeClause = try? node.activeClause(in: configuration),
56-
let elements = activeClause.elements
57-
{
58-
walk(Syntax(elements))
59-
}
70+
do {
71+
// If there is an active clause, visit it's children.
72+
if let activeClause = try node.activeClause(in: configuration),
73+
let elements = activeClause.elements {
74+
walk(Syntax(elements))
75+
}
6076

61-
return .skipChildren
77+
// Skip everything else in the
78+
return .skipChildren
79+
} catch {
80+
return reportEvaluationError(at: node, error: error)
81+
}
6282
}
6383
}
6484

@@ -89,26 +109,49 @@ open class ActiveSyntaxVisitor<Configuration: BuildConfiguration>: SyntaxVisitor
89109
/// `node.isActive(in: configuration)` will evaluate to `.active` or will
90110
/// throw.
91111
///
92-
/// TODO: This visitor currently swallows errors uncovered while checking `#if`
93-
/// conditions, which is deeply unfortunate. We need a better answer here.
112+
/// All notes visited by this visitor will have the "active" state, i.e.,
113+
/// `node.isActive(in: configuration)` will evaluate to `.active` or will
114+
/// throw. When errors occur, they will be reported via a call to
115+
/// `reportEvaluationError`, which can report the errors (the default is to
116+
/// turn them into diagnostics that go into the `diagnostics` array) and then
117+
/// choose whether to visit all of the `#if` clauses (the default) or skip them.
94118
open class ActiveSyntaxAnyVisitor<Configuration: BuildConfiguration>: SyntaxAnyVisitor {
95119
/// The build configuration, which will be queried for each relevant `#if`.
96120
public let configuration: Configuration
97121

122+
/// The set of diagnostics accumulated during this walk of active syntax.
123+
public var diagnostics: [Diagnostic] = []
124+
98125
public init(viewMode: SyntaxTreeViewMode, configuration: Configuration) {
99126
self.configuration = configuration
100127
super.init(viewMode: viewMode)
101128
}
102129

130+
/// Called when the evaluation of an `#if` condition produces an error.
131+
///
132+
/// By default, this records diagnostics from the error into the `diagnostics`
133+
/// array.
134+
///
135+
/// - Returns: Whether to visit the children of the `#if` or not after the
136+
/// error. By default, this function returns `.visitChildren`.
137+
open func reportEvaluationError(at node: IfConfigDeclSyntax, error: Error) -> SyntaxVisitorContinueKind {
138+
let newDiagnostics = error.asDiagnostics(at: node)
139+
diagnostics.append(contentsOf: newDiagnostics)
140+
return .visitChildren
141+
}
142+
103143
open override func visit(_ node: IfConfigDeclSyntax) -> SyntaxVisitorContinueKind {
104-
// If there is an active clause, visit it's children.
105-
// FIXME: try? suppresses errors here. How shall we report them?
106-
if let activeClause = try? node.activeClause(in: configuration),
107-
let elements = activeClause.elements
108-
{
109-
walk(Syntax(elements))
110-
}
144+
do {
145+
// If there is an active clause, visit it's children.
146+
if let activeClause = try node.activeClause(in: configuration),
147+
let elements = activeClause.elements {
148+
walk(Syntax(elements))
149+
}
111150

112-
return .skipChildren
151+
// Skip everything else in the
152+
return .skipChildren
153+
} catch {
154+
return reportEvaluationError(at: node, error: error)
155+
}
113156
}
114157
}

‎Tests/SwiftIfConfigTest/TestingBuildConfiguration.swift

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,27 @@
1212
import SwiftIfConfig
1313
import SwiftSyntax
1414

15+
enum BuildConfigurationError: Error, CustomStringConvertible {
16+
case badAttribute(String)
17+
18+
var description: String {
19+
switch self {
20+
case .badAttribute(let attribute):
21+
return "unacceptable attribute \(attribute)"
22+
}
23+
}
24+
}
25+
1526
struct TestingBuildConfiguration: BuildConfiguration {
1627
var platformName: String = "Linux"
1728
var customConditions: Set<String> = []
1829
var features: Set<String> = []
1930
var attributes: Set<String> = []
2031

32+
/// A set of attribute names that are "bad", causing the build configuration
33+
/// to throw an error if queried.
34+
var badAttributes: Set<String> = []
35+
2136
func isCustomConditionSet(name: String) -> Bool {
2237
customConditions.contains(name)
2338
}
@@ -26,8 +41,12 @@ struct TestingBuildConfiguration: BuildConfiguration {
2641
features.contains(name)
2742
}
2843

29-
func hasAttribute(name: String) -> Bool {
30-
attributes.contains(name)
44+
func hasAttribute(name: String) throws -> Bool {
45+
if badAttributes.contains(name) {
46+
throw BuildConfigurationError.badAttribute(name)
47+
}
48+
49+
return attributes.contains(name)
3150
}
3251

3352
func canImport(

‎Tests/SwiftIfConfigTest/VisitorTests.swift

Lines changed: 57 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,43 @@ class AllActiveVisitor: ActiveSyntaxAnyVisitor<TestingBuildConfiguration> {
3131
}
3232
}
3333

34+
class NameCheckingVisitor: ActiveSyntaxAnyVisitor<TestingBuildConfiguration> {
35+
/// The set of names we are expected to visit. Any syntax nodes with
36+
/// names that aren't here will be rejected, and each of the names listed
37+
/// here must occur exactly once.
38+
var expectedNames: Set<String>
39+
40+
init(configuration: TestingBuildConfiguration, expectedNames: Set<String>) {
41+
self.expectedNames = expectedNames
42+
43+
super.init(viewMode: .sourceAccurate, configuration: configuration)
44+
}
45+
46+
deinit {
47+
if !expectedNames.isEmpty {
48+
XCTFail("No nodes with expected names visited: \(expectedNames)")
49+
}
50+
}
51+
52+
func checkName(name: String, node: Syntax) {
53+
if !expectedNames.contains(name) {
54+
XCTFail("syntax node with unexpected name \(name) found: \(node.debugDescription)")
55+
}
56+
57+
expectedNames.remove(name)
58+
}
59+
60+
open override func visitAny(_ node: Syntax) -> SyntaxVisitorContinueKind {
61+
if let identified = node.asProtocol(NamedDeclSyntax.self) {
62+
checkName(name: identified.name.text, node: node)
63+
} else if let identPattern = node.as(IdentifierPatternSyntax.self) {
64+
// FIXME: Should the above be an IdentifiedDeclSyntax?
65+
checkName(name: identPattern.identifier.text, node: node)
66+
}
67+
68+
return .visitChildren
69+
}
70+
}
3471
public class VisitorTests: XCTestCase {
3572
let linuxBuildConfig = TestingBuildConfiguration(
3673
customConditions: ["DEBUG", "ASSERTS"],
@@ -95,6 +132,12 @@ public class VisitorTests: XCTestCase {
95132
#endif
96133
}
97134
#endif
135+
136+
#if hasAttribute(available)
137+
func withAvail() { }
138+
#else
139+
func notAvail() { }
140+
#endif
98141
"""
99142

100143
func testAnyVisitorVisitsOnlyActive() throws {
@@ -104,56 +147,29 @@ public class VisitorTests: XCTestCase {
104147
}
105148

106149
func testVisitsExpectedNodes() throws {
107-
class NameCheckingVisitor: ActiveSyntaxAnyVisitor<TestingBuildConfiguration> {
108-
/// The set of names we are expected to visit. Any syntax nodes with
109-
/// names that aren't here will be rejected, and each of the names listed
110-
/// here must occur exactly once.
111-
var expectedNames: Set<String>
112-
113-
init(configuration: TestingBuildConfiguration, expectedNames: Set<String>) {
114-
self.expectedNames = expectedNames
115-
116-
super.init(viewMode: .sourceAccurate, configuration: configuration)
117-
}
118-
119-
deinit {
120-
if !expectedNames.isEmpty {
121-
XCTFail("No nodes with expected names visited: \(expectedNames)")
122-
}
123-
}
124-
125-
func checkName(name: String, node: Syntax) {
126-
if !expectedNames.contains(name) {
127-
XCTFail("syntax node with unexpected name \(name) found: \(node.debugDescription)")
128-
}
129-
130-
expectedNames.remove(name)
131-
}
132-
133-
open override func visitAny(_ node: Syntax) -> SyntaxVisitorContinueKind {
134-
if let identified = node.asProtocol(NamedDeclSyntax.self) {
135-
checkName(name: identified.name.text, node: node)
136-
} else if let identPattern = node.as(IdentifierPatternSyntax.self) {
137-
// FIXME: Should the above be an IdentifiedDeclSyntax?
138-
checkName(name: identPattern.identifier.text, node: node)
139-
}
140-
141-
return .visitChildren
142-
}
143-
}
144-
145150
// Check that the right set of names is visited.
146151
NameCheckingVisitor(
147152
configuration: linuxBuildConfig,
148-
expectedNames: ["f", "h", "i", "S", "generationCount", "value"]
153+
expectedNames: ["f", "h", "i", "S", "generationCount", "value", "withAvail"]
149154
).walk(inputSource)
150155

151156
NameCheckingVisitor(
152157
configuration: iosBuildConfig,
153-
expectedNames: ["g", "h", "i", "a", "S", "generationCount", "value", "error"]
158+
expectedNames: ["g", "h", "i", "a", "S", "generationCount", "value", "error", "withAvail"]
154159
).walk(inputSource)
155160
}
156161

162+
func testVisitorWithErrors() throws {
163+
var configuration = linuxBuildConfig
164+
configuration.badAttributes.insert("available")
165+
let visitor = NameCheckingVisitor(
166+
configuration: configuration,
167+
expectedNames: ["f", "h", "i", "S", "generationCount", "value", "withAvail", "notAvail"]
168+
)
169+
visitor.walk(inputSource)
170+
XCTAssertEqual(visitor.diagnostics.count, 3)
171+
}
172+
157173
func testRemoveInactive() {
158174
assertStringsEqualWithDiff(
159175
inputSource.removingInactive(in: linuxBuildConfig).description,
@@ -179,6 +195,7 @@ public class VisitorTests: XCTestCase {
179195
.c
180196
.d()
181197
}
198+
func withAvail() { }
182199
"""
183200
)
184201
}

0 commit comments

Comments
 (0)