Skip to content
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.

Sign up for GitHub

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

Warn about effectful expectations. #302

Merged
merged 3 commits into from
Mar 19, 2024
Merged

Warn about effectful expectations. #302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dadbd91
Warn about effectful expectations.
grynspan Mar 19, 2024
7eba9b3
broken down -> expanded
grynspan Mar 19, 2024
71fe519
Remove dead-store isTrivial (from earlier approach)
grynspan Mar 19, 2024
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
69 changes: 69 additions & 0 deletions Sources/TestingMacros/Support/ConditionArgumentParsing.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,64 @@ func removeParentheses(from expr: ExprSyntax) -> ExprSyntax? {
return nil
}

/// A class that walks a syntax tree looking for `try` and `await` expressions.
///
/// - Bug: This class does not use `lexicalContext` to check for the presence of
/// `try` or `await` _outside_ the current macro expansion.
private final class _EffectFinder: SyntaxVisitor {
/// The effectful expressions discovered so far.
var effectfulExprs = [ExprSyntax]()

/// Common implementation for `visit(_: TryExprSyntax)` and
/// `visit(_: AwaitExprSyntax)`.
///
/// - Parameters:
/// - node: The `try` or `await` expression.
/// - expression: The `.expression` property of `node`.
///
/// - Returns: Whether or not to recurse into `node`.
private func _visitEffectful(_ node: some ExprSyntaxProtocol, expression: ExprSyntax) -> SyntaxVisitorContinueKind {
if let parentNode = node.parent, parentNode.is(TryExprSyntax.self) {
// Suppress this expression as its immediate parent is also an effectful
// expression (e.g. it's a `try await` expression overall.) The diagnostic
// reported for the parent expression should include both as needed.
return .visitChildren
} else if expression.is(AsExprSyntax.self) {
// Do not walk into explicit `as T` casts. This provides an escape hatch
// for expressions that should not diagnose.
return .skipChildren
} else if let awaitExpr = expression.as(AwaitExprSyntax.self), awaitExpr.expression.is(AsExprSyntax.self) {
// As above but for `try await _ as T`.
return .skipChildren
}
effectfulExprs.append(ExprSyntax(node))
return .visitChildren
}

override func visit(_ node: TryExprSyntax) -> SyntaxVisitorContinueKind {
_visitEffectful(node, expression: node.expression)
}

override func visit(_ node: AwaitExprSyntax) -> SyntaxVisitorContinueKind {
_visitEffectful(node, expression: node.expression)
}

override func visit(_ node: AsExprSyntax) -> SyntaxVisitorContinueKind {
// Do not walk into explicit `as T` casts. This provides an escape hatch for
// expressions that should not diagnose.
return .skipChildren
}

override func visit(_ node: ClosureExprSyntax) -> SyntaxVisitorContinueKind {
// Do not walk into closures. Although they are not meant to be an escape
// hatch like `as` casts, it is very difficult (often impossible) to reason
// about effectful expressions inside the scope of a closure. If the closure
// is invoked locally, its caller will also need to say `try`/`await` and we
// can still diagnose those outer expressions.
return .skipChildren
}
}

// MARK: -

/// Parse a condition argument from a binary operation expression.
Expand Down Expand Up @@ -496,6 +554,17 @@ private func _parseCondition(from expr: ExprSyntax, for macro: some Freestanding
///
/// - Returns: An instance of ``Condition`` describing `expr`.
func parseCondition(from expr: ExprSyntax, for macro: some FreestandingMacroExpansionSyntax, in context: some MacroExpansionContext) -> Condition {
// Handle `await` first. If present in the expression or a subexpression,
// diagnose and don't expand further.
let effectFinder = _EffectFinder(viewMode: .sourceAccurate)
effectFinder.walk(expr)
guard effectFinder.effectfulExprs.isEmpty else {
for effectfulExpr in effectFinder.effectfulExprs {
context.diagnose(.effectfulExpressionNotParsed(effectfulExpr, in: macro))
}
return Condition(expression: expr)
}

let result = _parseCondition(from: expr, for: macro, in: context)
if result.arguments.count == 1, let onlyArgument = result.arguments.first {
_diagnoseTrivialBooleanValue(from: onlyArgument.expression, for: macro, in: context)
Expand Down
95 changes: 75 additions & 20 deletions Sources/TestingMacros/Support/DiagnosticMessage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
///
/// - Returns: A diagnostic message.
static func condition(_ condition: ExprSyntax, isAlways value: Bool, in macro: some FreestandingMacroExpansionSyntax) -> Self {
Self(
let action = value ? "pass" : "fail"
return Self(
syntax: Syntax(condition),
message: "#\(macro.macroName.textWithoutBackticks)(_:_:) will always \(value ? "pass" : "fail") here; use Bool(\(condition)) to silence this warning",
message: "\(_macroName(macro)) will always \(action) here; use 'Bool(\(condition))' to silence this warning",
severity: value ? .note : .warning
)
}
Expand All @@ -50,11 +51,59 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func asExclamationMarkIsEvaluatedEarly(_ expr: AsExprSyntax, in macro: some FreestandingMacroExpansionSyntax) -> Self {
return Self(
syntax: Syntax(expr.asKeyword),
message: "The expression \(expr.trimmed) will be evaluated before #\(macro.macroName.textWithoutBackticks)(_:_:) is invoked; use as? instead of as! to silence this warning",
message: "Expression '\(expr.trimmed)' will be evaluated before \(_macroName(macro)) is invoked; use 'as?' instead of 'as!' to silence this warning",
severity: .warning
)
}

/// Create a diagnostic message stating that an effectful (`try` or `await`)
/// expression cannot be parsed and should be broken apart.
///
/// - Parameters:
/// - expr: The expression being diagnosed.
/// - macro: The macro expression.
///
/// - Returns: A diagnostic message.
static func effectfulExpressionNotParsed(_ expr: ExprSyntax, in macro: some FreestandingMacroExpansionSyntax) -> Self {
let effectful = if let tryExpr = expr.as(TryExprSyntax.self) {
if tryExpr.expression.is(AwaitExprSyntax.self) {
"throwing/asynchronous"
grynspan marked this conversation as resolved.
Show resolved Hide resolved
} else {
"throwing"
}
} else {
"asynchronous"
}
return Self(
syntax: Syntax(expr),
message: "Expression '\(expr.trimmed)' will not be expanded on failure; move the \(effectful) part out of the call to \(_macroName(macro))",
severity: .warning
)
}

/// Get the human-readable name of the given freestanding macro.
///
/// - Parameters:
/// - macro: The freestanding macro node to name.
///
/// - Returns: The name of the macro as understood by a developer, such as
/// `"'#expect(_:_:)'"`. Include single quotes.
private static func _macroName(_ macro: some FreestandingMacroExpansionSyntax) -> String {
"'#\(macro.macroName.textWithoutBackticks)(_:_:)'"
}

/// Get the human-readable name of the given attached macro.
///
/// - Parameters:
/// - attribute: The attached macro node to name.
///
/// - Returns: The name of the macro as understood by a developer, such as
/// `"'@Test'"`. Include single quotes.
private static func _macroName(_ attribute: AttributeSyntax) -> String {
// SEE: https://github.com/apple/swift/blob/main/docs/Diagnostics.md?plain=1#L44
"'\(attribute.attributeNameText)'"
}

/// Get a string corresponding to the specified syntax node (for instance,
/// `"function"` for a function declaration.)
///
Expand Down Expand Up @@ -117,7 +166,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
precondition(!attributes.isEmpty)
return Self(
syntax: Syntax(attributes.last!),
message: "The @\(attributes.last!.attributeNameText) attribute cannot be applied to \(_kindString(for: decl, includeA: true)) more than once.",
message: "Attribute \(_macroName(attributes.last!)) cannot be applied to \(_kindString(for: decl, includeA: true)) more than once",
severity: .error
)
}
Expand All @@ -134,7 +183,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func genericDeclarationNotSupported(_ decl: some SyntaxProtocol, whenUsing attribute: AttributeSyntax, becauseOf genericClause: some SyntaxProtocol) -> Self {
Self(
syntax: Syntax(genericClause),
message: "The @\(attribute.attributeNameText) attribute cannot be applied to a generic \(_kindString(for: decl)).",
message: "Attribute \(_macroName(attribute)) cannot be applied to a generic \(_kindString(for: decl))",
severity: .error
)
}
Expand All @@ -155,7 +204,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func availabilityAttributeNotSupported(_ availabilityAttribute: AttributeSyntax, on decl: some SyntaxProtocol, whenUsing attribute: AttributeSyntax) -> Self {
Self(
syntax: Syntax(availabilityAttribute),
message: "The @\(attribute.attributeNameText) attribute cannot be applied to this \(_kindString(for: decl)) because it has been marked \(availabilityAttribute.trimmed).",
message: "Attribute \(_macroName(attribute)) cannot be applied to this \(_kindString(for: decl)) because it has been marked '\(availabilityAttribute.trimmed)'",
severity: .error
)
}
Expand All @@ -171,7 +220,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func attributeNotSupported(_ attribute: AttributeSyntax, on decl: some SyntaxProtocol) -> Self {
Self(
syntax: Syntax(decl),
message: "The @\(attribute.attributeNameText) attribute cannot be applied to \(_kindString(for: decl, includeA: true)).",
message: "Attribute \(_macroName(attribute)) cannot be applied to \(_kindString(for: decl, includeA: true))",
severity: .error
)
}
Expand All @@ -187,8 +236,14 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func attributeHasNoEffect(_ attribute: AttributeSyntax, on decl: ExtensionDeclSyntax) -> Self {
Self(
syntax: Syntax(decl),
message: "The @\(attribute.attributeNameText) attribute has no effect when applied to an extension and should be removed.",
severity: .error
message: "Attribute \(_macroName(attribute)) has no effect when applied to an extension",
severity: .error,
fixIts: [
FixIt(
message: MacroExpansionFixItMessage("Remove attribute \(_macroName(attribute))"),
changes: [.replace(oldNode: Syntax(attribute), newNode: Syntax("" as ExprSyntax))]
),
]
)
}

Expand All @@ -206,19 +261,19 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
case 0:
return Self(
syntax: Syntax(functionDecl),
message: "The @\(attribute.attributeNameText) attribute cannot specify arguments when used with \(functionDecl.completeName) because it does not take any.",
message: "Attribute \(_macroName(attribute)) cannot specify arguments when used with '\(functionDecl.completeName)' because it does not take any",
severity: .error
)
case 1:
return Self(
syntax: Syntax(functionDecl),
message: "The @\(attribute.attributeNameText) attribute must specify an argument when used with \(functionDecl.completeName).",
message: "Attribute \(_macroName(attribute)) must specify an argument when used with '\(functionDecl.completeName)'",
severity: .error
)
default:
return Self(
syntax: Syntax(functionDecl),
message: "The @\(attribute.attributeNameText) attribute must specify \(expectedArgumentCount) arguments when used with \(functionDecl.completeName).",
message: "Attribute \(_macroName(attribute)) must specify \(expectedArgumentCount) arguments when used with '\(functionDecl.completeName)'",
severity: .error
)
}
Expand All @@ -236,7 +291,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func xcTestCaseNotSupported(_ decl: some SyntaxProtocol, whenUsing attribute: AttributeSyntax) -> Self {
Self(
syntax: Syntax(decl),
message: "The @\(attribute.attributeNameText) attribute cannot be applied to a subclass of XCTestCase.",
message: "Attribute \(_macroName(attribute)) cannot be applied to a subclass of 'XCTestCase'",
severity: .error
)
}
Expand All @@ -252,7 +307,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func nonFinalClassNotSupported(_ decl: ClassDeclSyntax, whenUsing attribute: AttributeSyntax) -> Self {
Self(
syntax: Syntax(decl),
message: "The @\(attribute.attributeNameText) attribute cannot be applied to non-final class \(decl.name.textWithoutBackticks).",
message: "Attribute \(_macroName(attribute)) cannot be applied to non-final class '\(decl.name.textWithoutBackticks)'",
severity: .error
)
}
Expand All @@ -269,7 +324,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func specifierNotSupported(_ specifier: TokenSyntax, on parameter: FunctionParameterSyntax, whenUsing attribute: AttributeSyntax) -> Self {
Self(
syntax: Syntax(parameter),
message: "The @\(attribute.attributeNameText) attribute cannot be applied to a function with a parameter marked '\(specifier.textWithoutBackticks)'.",
message: "Attribute \(_macroName(attribute)) cannot be applied to a function with a parameter marked '\(specifier.textWithoutBackticks)'",
severity: .error
)
}
Expand All @@ -286,7 +341,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func returnTypeNotSupported(_ returnType: TypeSyntax, on decl: some SyntaxProtocol, whenUsing attribute: AttributeSyntax) -> Self {
Self(
syntax: Syntax(returnType),
message: "The result of this \(_kindString(for: decl)) will be discarded during testing.",
message: "The result of this \(_kindString(for: decl)) will be discarded during testing",
severity: .warning
)
}
Expand All @@ -302,7 +357,7 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func tagExprNotSupported(_ tagExpr: some SyntaxProtocol, in attribute: AttributeSyntax) -> Self {
Self(
syntax: Syntax(tagExpr),
message: "The tag \(tagExpr.trimmed) cannot be used with the @\(attribute.attributeNameText) attribute. Pass a member of Tag or a string literal instead.",
message: "Tag '\(tagExpr.trimmed)' cannot be used with attribute \(_macroName(attribute)); pass a member of 'Tag' or a string literal instead",
severity: .error
)
}
Expand All @@ -317,15 +372,15 @@ struct DiagnosticMessage: SwiftDiagnostics.DiagnosticMessage {
static func optionalBoolExprIsAmbiguous(_ boolExpr: ExprSyntax) -> Self {
Self(
syntax: Syntax(boolExpr),
message: "Requirement '\(boolExpr.trimmed)' is ambiguous.",
message: "Requirement '\(boolExpr.trimmed)' is ambiguous",
severity: .warning,
fixIts: [
FixIt(
message: MacroExpansionFixItMessage("To unwrap an optional value, add 'as Bool?'."),
message: MacroExpansionFixItMessage("To unwrap an optional value, add 'as Bool?'"),
changes: [.replace(oldNode: Syntax(boolExpr), newNode: Syntax("\(boolExpr) as Bool?" as ExprSyntax))]
),
FixIt(
message: MacroExpansionFixItMessage("To check if a value is true, add '?? false'."),
message: MacroExpansionFixItMessage("To check if a value is true, add '?? false'"),
changes: [.replace(oldNode: Syntax(boolExpr), newNode: Syntax("\(boolExpr) ?? false" as ExprSyntax))]
),
]
Expand Down
43 changes: 43 additions & 0 deletions Tests/TestingMacrosTests/ConditionMacroTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,49 @@ struct ConditionMacroTests {
#expect(diagnostics.isEmpty)
}

@Test("#expect(try/await) produces a diagnostic",
arguments: [
"#expect(try foo())": ["Expression 'try foo()' will not be expanded on failure; move the throwing part out of the call to '#expect(_:_:)'"],
"#expect(await foo())": ["Expression 'await foo()' will not be expanded on failure; move the asynchronous part out of the call to '#expect(_:_:)'"],
"#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(_:_:)'"],
"#expect(try await foo(try bar(await quux())))": [
"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(_:_:)'",
"Expression 'try bar(await quux())' will not be expanded on failure; move the throwing part out of the call to '#expect(_:_:)'",
"Expression 'await quux()' will not be expanded on failure; move the asynchronous part out of the call to '#expect(_:_:)'",
],

// Diagnoses because the diagnostic for `await` is suppressed due to the
// `as T` cast, but the parentheses limit the effect of the suppression.
"#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(_:_:)'"],
]
)
func effectfulExpectationDiagnoses(input: String, diagnosticMessages: [String]) throws {
let (_, diagnostics) = try parse(input)
#expect(diagnostics.count == diagnosticMessages.count)
for message in diagnosticMessages {
#expect(diagnostics.contains { $0.diagMessage.message == message }, "Missing \(message): \(diagnostics.map(\.diagMessage.message))")
}
}

@Test("#expect(try/await as Bool) suppresses its diagnostic",
arguments: [
"#expect(try foo() as Bool)",
"#expect(await foo() as Bool)",
"#expect(try await foo(try await bar()) as Bool)",
"#expect(try foo() as T?)",
"#expect(await foo() as? T)",
"#expect(try await foo(try await bar()) as! T)",
"#expect((try foo()) as T)",
"#expect((await foo()) as T)",
"#expect((try await foo(try await bar())) as T)",
"#expect(try (await foo()) as T)",
]
)
func effectfulExpectationDiagnosticSuppressWithExplicitBool(input: String) throws {
let (_, diagnostics) = try parse(input)
#expect(diagnostics.isEmpty)
}

@Test("Macro expansion is performed within a test function")
func macroExpansionInTestFunction() throws {
let input = ##"""
Expand Down
Loading