Skip to content

Commit 12b0fd9

Browse files
authored
Remove the new diagnostic added in #302. (#352)
In #302, we added a compile-time diagnostic (warning) for expressions like: ```swift #expect(try await foo()) ``` Because we figured that the lack of expression expansion for effectful expressions might be confusing. However, we've found that the diagnostic is significantly noisier than we'd like and the cons outweigh the pros. Hence, this PR removes that diagnostic. Resolves rdar://126393932. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
1 parent f1850ef commit 12b0fd9

11 files changed

+86
-224
lines changed

Sources/TestingMacros/Support/ConditionArgumentParsing.swift

Lines changed: 0 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -128,64 +128,6 @@ func removeParentheses(from expr: ExprSyntax) -> ExprSyntax? {
128128
return nil
129129
}
130130

131-
/// A class that walks a syntax tree looking for `try` and `await` expressions.
132-
///
133-
/// - Bug: This class does not use `lexicalContext` to check for the presence of
134-
/// `try` or `await` _outside_ the current macro expansion.
135-
private final class _EffectFinder: SyntaxVisitor {
136-
/// The effectful expressions discovered so far.
137-
var effectfulExprs = [ExprSyntax]()
138-
139-
/// Common implementation for `visit(_: TryExprSyntax)` and
140-
/// `visit(_: AwaitExprSyntax)`.
141-
///
142-
/// - Parameters:
143-
/// - node: The `try` or `await` expression.
144-
/// - expression: The `.expression` property of `node`.
145-
///
146-
/// - Returns: Whether or not to recurse into `node`.
147-
private func _visitEffectful(_ node: some ExprSyntaxProtocol, expression: ExprSyntax) -> SyntaxVisitorContinueKind {
148-
if let parentNode = node.parent, parentNode.is(TryExprSyntax.self) {
149-
// Suppress this expression as its immediate parent is also an effectful
150-
// expression (e.g. it's a `try await` expression overall.) The diagnostic
151-
// reported for the parent expression should include both as needed.
152-
return .visitChildren
153-
} else if expression.is(AsExprSyntax.self) {
154-
// Do not walk into explicit `as T` casts. This provides an escape hatch
155-
// for expressions that should not diagnose.
156-
return .skipChildren
157-
} else if let awaitExpr = expression.as(AwaitExprSyntax.self), awaitExpr.expression.is(AsExprSyntax.self) {
158-
// As above but for `try await _ as T`.
159-
return .skipChildren
160-
}
161-
effectfulExprs.append(ExprSyntax(node))
162-
return .visitChildren
163-
}
164-
165-
override func visit(_ node: TryExprSyntax) -> SyntaxVisitorContinueKind {
166-
_visitEffectful(node, expression: node.expression)
167-
}
168-
169-
override func visit(_ node: AwaitExprSyntax) -> SyntaxVisitorContinueKind {
170-
_visitEffectful(node, expression: node.expression)
171-
}
172-
173-
override func visit(_ node: AsExprSyntax) -> SyntaxVisitorContinueKind {
174-
// Do not walk into explicit `as T` casts. This provides an escape hatch for
175-
// expressions that should not diagnose.
176-
return .skipChildren
177-
}
178-
179-
override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind {
180-
// Do not walk into closures. Although they are not meant to be an escape
181-
// hatch like `as` casts, it is very difficult (often impossible) to reason
182-
// about effectful expressions inside the scope of a closure. If the closure
183-
// is invoked locally, its caller will also need to say `try`/`await` and we
184-
// can still diagnose those outer expressions.
185-
return .skipChildren
186-
}
187-
}
188-
189131
// MARK: -
190132

191133
/// Parse a condition argument from a binary operation expression.
@@ -554,17 +496,6 @@ private func _parseCondition(from expr: ExprSyntax, for macro: some Freestanding
554496
///
555497
/// - Returns: An instance of ``Condition`` describing `expr`.
556498
func parseCondition(from expr: ExprSyntax, for macro: some FreestandingMacroExpansionSyntax, in context: some MacroExpansionContext) -> Condition {
557-
// Handle `await` first. If present in the expression or a subexpression,
558-
// diagnose and don't expand further.
559-
let effectFinder = _EffectFinder(viewMode: .sourceAccurate)
560-
effectFinder.walk(expr)
561-
guard effectFinder.effectfulExprs.isEmpty else {
562-
for effectfulExpr in effectFinder.effectfulExprs {
563-
context.diagnose(.effectfulExpressionNotParsed(effectfulExpr, in: macro))
564-
}
565-
return Condition(expression: expr)
566-
}
567-
568499
let result = _parseCondition(from: expr, for: macro, in: context)
569500
if result.arguments.count == 1, let onlyArgument = result.arguments.first {
570501
_diagnoseTrivialBooleanValue(from: onlyArgument.expression, for: macro, in: context)

Sources/TestingMacros/Support/DiagnosticMessage.swift

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -56,31 +56,6 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
5656
)
5757
}
5858

59-
/// Create a diagnostic message stating that an effectful (`try` or `await`)
60-
/// expression cannot be parsed and should be broken apart.
61-
///
62-
/// - Parameters:
63-
/// - expr: The expression being diagnosed.
64-
/// - macro: The macro expression.
65-
///
66-
/// - Returns: A diagnostic message.
67-
static func effectfulExpressionNotParsed(_ expr: ExprSyntax, in macro: some FreestandingMacroExpansionSyntax) -> Self {
68-
let effectful = if let tryExpr = expr.as(TryExprSyntax.self) {
69-
if tryExpr.expression.is(AwaitExprSyntax.self) {
70-
"throwing/asynchronous"
71-
} else {
72-
"throwing"
73-
}
74-
} else {
75-
"asynchronous"
76-
}
77-
return Self(
78-
syntax: Syntax(expr),
79-
message: "Expression '\(expr.trimmed)' will not be expanded on failure; move the \(effectful) part out of the call to \(_macroName(macro))",
80-
severity: .warning
81-
)
82-
}
83-
8459
/// Get the human-readable name of the given freestanding macro.
8560
///
8661
/// - Parameters:

Tests/TestingMacrosTests/ConditionMacroTests.swift

Lines changed: 0 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -331,49 +331,6 @@ struct ConditionMacroTests {
331331
#expect(diagnostics.isEmpty)
332332
}
333333

334-
@Test("#expect(try/await) produces a diagnostic",
335-
arguments: [
336-
"#expect(try foo())": ["Expression 'try foo()' will not be expanded on failure; move the throwing part out of the call to '#expect(_:_:)'"],
337-
"#expect(await foo())": ["Expression 'await foo()' will not be expanded on failure; move the asynchronous part out of the call to '#expect(_:_:)'"],
338-
"#expect(try await foo())": ["Expression 'try await foo()' will not be expanded on failure; move the throwing/asynchronous part out of the call to '#expect(_:_:)'"],
339-
"#expect(try await foo(try bar(await quux())))": [
340-
"Expression 'try await foo(try bar(await quux()))' will not be expanded on failure; move the throwing/asynchronous part out of the call to '#expect(_:_:)'",
341-
"Expression 'try bar(await quux())' will not be expanded on failure; move the throwing part out of the call to '#expect(_:_:)'",
342-
"Expression 'await quux()' will not be expanded on failure; move the asynchronous part out of the call to '#expect(_:_:)'",
343-
],
344-
345-
// Diagnoses because the diagnostic for `await` is suppressed due to the
346-
// `as T` cast, but the parentheses limit the effect of the suppression.
347-
"#expect(try (await foo() as T))": ["Expression 'try (await foo() as T)' will not be expanded on failure; move the throwing part out of the call to '#expect(_:_:)'"],
348-
]
349-
)
350-
func effectfulExpectationDiagnoses(input: String, diagnosticMessages: [String]) throws {
351-
let (_, diagnostics) = try parse(input)
352-
#expect(diagnostics.count == diagnosticMessages.count)
353-
for message in diagnosticMessages {
354-
#expect(diagnostics.contains { $0.diagMessage.message == message }, "Missing \(message): \(diagnostics.map(\.diagMessage.message))")
355-
}
356-
}
357-
358-
@Test("#expect(try/await as Bool) suppresses its diagnostic",
359-
arguments: [
360-
"#expect(try foo() as Bool)",
361-
"#expect(await foo() as Bool)",
362-
"#expect(try await foo(try await bar()) as Bool)",
363-
"#expect(try foo() as T?)",
364-
"#expect(await foo() as? T)",
365-
"#expect(try await foo(try await bar()) as! T)",
366-
"#expect((try foo()) as T)",
367-
"#expect((await foo()) as T)",
368-
"#expect((try await foo(try await bar())) as T)",
369-
"#expect(try (await foo()) as T)",
370-
]
371-
)
372-
func effectfulExpectationDiagnosticSuppressWithExplicitBool(input: String) throws {
373-
let (_, diagnostics) = try parse(input)
374-
#expect(diagnostics.isEmpty)
375-
}
376-
377334
@Test("Expectation inside an exit test diagnoses",
378335
arguments: [
379336
"#expectExitTest(exitsWith: .failure) { #expect(1 > 2) }",

Tests/TestingTests/EventRecorderTests.swift

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,11 +150,12 @@ struct EventRecorderTests {
150150
One(.anyGraphemeCluster)
151151
" \(isSuite ? "Suite" : "Test") \(testName) started."
152152
}
153-
let match = try buffer
154-
.split(whereSeparator: \.isNewline)
155-
.compactMap(testFailureRegex.wholeMatch(in:))
156-
.first
157-
#expect(match != nil)
153+
#expect(
154+
try buffer
155+
.split(whereSeparator: \.isNewline)
156+
.compactMap(testFailureRegex.wholeMatch(in:))
157+
.first != nil
158+
)
158159
}
159160

160161
@available(_regexAPI, *)

Tests/TestingTests/IssueTests.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ final class IssueTests: XCTestCase {
5252
}
5353

5454
await Test { () throws in
55-
#expect(try { throw MyError() }() as Bool)
55+
#expect(try { throw MyError() }())
5656
}.run(configuration: configuration)
5757

5858
await Test { () throws in
@@ -283,8 +283,8 @@ final class IssueTests: XCTestCase {
283283
}
284284

285285
await Test { () throws in
286-
#expect(try TypeWithMemberFunctions.n(0) as Bool)
287-
#expect(TypeWithMemberFunctions.f(try { () throws in 0 }()) as Bool)
286+
#expect(try TypeWithMemberFunctions.n(0))
287+
#expect(TypeWithMemberFunctions.f(try { () throws in 0 }()))
288288
}.run(configuration: configuration)
289289

290290
await fulfillment(of: [expectationFailed], timeout: 0.0)

Tests/TestingTests/MiscellaneousTests.swift

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -194,26 +194,24 @@ struct TestsWithAsyncArguments {
194194
struct MiscellaneousTests {
195195
@Test("Free function's name")
196196
func unnamedFreeFunctionTest() async throws {
197-
let tests = await Test.all
198-
let testFunction = try #require(tests.first(where: { $0.name.contains("freeSyncFunction") }))
197+
let testFunction = try #require(await Test.all.first(where: { $0.name.contains("freeSyncFunction") }))
199198
#expect(testFunction.name == "freeSyncFunction()")
200199
}
201200

202201
@Test("Test suite type's name")
203202
func unnamedMemberFunctionTest() async throws {
204-
let testType = try #require(await test(for: SendableTests.self) as Test?)
203+
let testType = try #require(await test(for: SendableTests.self))
205204
#expect(testType.name == "SendableTests")
206205
}
207206

208207
@Test("Free function has custom display name")
209208
func namedFreeFunctionTest() async throws {
210-
let tests = await Test.all
211-
#expect(tests.first { $0.displayName == "Named Free Sync Function" && !$0.isSuite && $0.containingTypeInfo == nil } != nil)
209+
#expect(await Test.all.first { $0.displayName == "Named Free Sync Function" && !$0.isSuite && $0.containingTypeInfo == nil } != nil)
212210
}
213211

214212
@Test("Member function has custom display name")
215213
func namedMemberFunctionTest() async throws {
216-
let testType = try #require(await test(for: NamedSendableTests.self) as Test?)
214+
let testType = try #require(await test(for: NamedSendableTests.self))
217215
#expect(testType.displayName == "Named Sendable test type")
218216
}
219217

@@ -321,18 +319,18 @@ struct MiscellaneousTests {
321319
@Test("Test.parameters property")
322320
func parametersProperty() async throws {
323321
do {
324-
let theTest = try #require(await test(for: SendableTests.self) as Test?)
322+
let theTest = try #require(await test(for: SendableTests.self))
325323
#expect(theTest.parameters == nil)
326324
}
327325

328326
do {
329-
let test = try #require(await testFunction(named: "succeeds()", in: SendableTests.self) as Test?)
327+
let test = try #require(await testFunction(named: "succeeds()", in: SendableTests.self))
330328
let parameters = try #require(test.parameters)
331329
#expect(parameters.isEmpty)
332330
} catch {}
333331

334332
do {
335-
let test = try #require(await testFunction(named: "parameterized(i:)", in: NonSendableTests.self) as Test?)
333+
let test = try #require(await testFunction(named: "parameterized(i:)", in: NonSendableTests.self))
336334
let parameters = try #require(test.parameters)
337335
#expect(parameters.count == 1)
338336
let firstParameter = try #require(parameters.first)
@@ -345,7 +343,7 @@ struct MiscellaneousTests {
345343
} catch {}
346344

347345
do {
348-
let test = try #require(await testFunction(named: "parameterized2(i:j:)", in: NonSendableTests.self) as Test?)
346+
let test = try #require(await testFunction(named: "parameterized2(i:j:)", in: NonSendableTests.self))
349347
let parameters = try #require(test.parameters)
350348
#expect(parameters.count == 2)
351349
let firstParameter = try #require(parameters.first)

0 commit comments

Comments
 (0)