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.

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

Property accesses in failed expectations sometimes include their value twice in expanded description #601

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ public func __checkPropertyAccess<T>(
return __checkValue(
condition,
expression: expression,
expressionWithCapturedRuntimeValues: expression.capturingRuntimeValues(lhs, condition),
expressionWithCapturedRuntimeValues: expression.capturingRuntimeValues(lhs),
comments: comments(),
isRequired: isRequired,
sourceLocation: sourceLocation
Expand Down Expand Up @@ -577,7 +577,7 @@ public func __checkPropertyAccess<T, U>(
return __checkValue(
optionalValue,
expression: expression,
expressionWithCapturedRuntimeValues: expression.capturingRuntimeValues(lhs, optionalValue as U??),
expressionWithCapturedRuntimeValues: expression.capturingRuntimeValues(lhs),
comments: comments(),
isRequired: isRequired,
sourceLocation: sourceLocation
Expand Down
2 changes: 1 addition & 1 deletion Sources/Testing/SourceAttribution/Expression+Macro.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ extension __Expression {
///
/// - Warning: This function is used to implement the `@Test`, `@Suite`,
/// `#expect()` and `#require()` macros. Do not call it directly.
public static func __fromPropertyAccess(_ value: Self, _ keyPath: Self) -> Self {
public static func __fromPropertyAccess(_ value: Self, _ keyPath: String) -> Self {
return Self(kind: .propertyAccess(value: value, keyPath: keyPath))
}

Expand Down
18 changes: 9 additions & 9 deletions Sources/Testing/SourceAttribution/Expression.swift
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public struct __Expression: Sendable {
/// - value: The value whose property was accessed.
/// - keyPath: The key path, relative to `value`, that was accessed, not
/// including a leading backslash or period.
indirect case propertyAccess(value: __Expression, keyPath: __Expression)
indirect case propertyAccess(value: __Expression, keyPath: String)

/// The expression negates another expression.
///
Expand Down Expand Up @@ -124,7 +124,7 @@ public struct __Expression: Sendable {
}
return "\(functionName)(\(argumentList))"
case let .propertyAccess(value, keyPath):
return "\(value.sourceCode).\(keyPath.sourceCode)"
return "\(value.sourceCode).\(keyPath)"
case let .negation(expression, isParenthetical):
var sourceCode = expression.sourceCode
if isParenthetical {
Expand Down Expand Up @@ -298,7 +298,9 @@ public struct __Expression: Sendable {

// Convert the variadic generic argument list to an array.
var additionalValuesArray = [Any?]()
repeat additionalValuesArray.append(each additionalValues)
for additionalValue in repeat each additionalValues {
additionalValuesArray.append(additionalValue)
}

switch kind {
case .generic, .stringLiteral:
Expand All @@ -320,7 +322,7 @@ public struct __Expression: Sendable {
case let .propertyAccess(value, keyPath):
result.kind = .propertyAccess(
value: value.capturingRuntimeValues(firstValue),
keyPath: keyPath.capturingRuntimeValues(additionalValuesArray.first ?? nil)
keyPath: keyPath
)
case let .negation(expression, isParenthetical):
result.kind = .negation(
Expand Down Expand Up @@ -421,9 +423,7 @@ public struct __Expression: Sendable {
"\(functionName)(\(argumentList))"
}
case let .propertyAccess(value, keyPath):
var keyPathContext = childContext
keyPathContext.includeParenthesesIfNeeded = false
result = "\(value._expandedDescription(in: childContext)).\(keyPath._expandedDescription(in: keyPathContext))"
result = "\(value._expandedDescription(in: childContext)).\(keyPath)"
case let .negation(expression, isParenthetical):
childContext.includeParenthesesIfNeeded = !isParenthetical
var expandedDescription = expression._expandedDescription(in: childContext)
Expand Down Expand Up @@ -475,8 +475,8 @@ public struct __Expression: Sendable {
} else {
arguments.lazy.map(\.value)
}
case let .propertyAccess(value: value, keyPath: keyPath):
[value, keyPath]
case let .propertyAccess(value, _):
[value]
case let .negation(expression, _):
[expression]
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/TestingMacros/Support/SourceCodeCapturing.swift
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func createExpressionExprForFunctionCall(_ value: (any SyntaxProtocol)?, _ funct
func createExpressionExprForPropertyAccess(_ value: ExprSyntax, _ keyPath: DeclReferenceExprSyntax) -> ExprSyntax {
let arguments = LabeledExprListSyntax {
LabeledExprSyntax(expression: createExpressionExpr(from: value))
LabeledExprSyntax(expression: createExpressionExpr(from: keyPath.baseName))
LabeledExprSyntax(expression: StringLiteralExprSyntax(content: keyPath.baseName.text))
}

return ".__fromPropertyAccess(\(arguments))"
Expand Down
18 changes: 9 additions & 9 deletions Tests/TestingMacrosTests/ConditionMacroTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,13 @@ struct ConditionMacroTests {
##"#expect(a, sourceLocation: someValue)"##:
##"Testing.__checkValue(a, expression: .__fromSyntaxNode("a"), comments: [], isRequired: false, sourceLocation: someValue).__expected()"##,
##"#expect(a.isB)"##:
##"Testing.__checkPropertyAccess(a.self, getting: { $0.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a"), .__fromSyntaxNode("isB")), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
##"Testing.__checkPropertyAccess(a.self, getting: { $0.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a"), "isB"), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
##"#expect(a???.isB)"##:
##"Testing.__checkPropertyAccess(a.self, getting: { $0???.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a"), .__fromSyntaxNode("isB")), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
##"Testing.__checkPropertyAccess(a.self, getting: { $0???.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a"), "isB"), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
##"#expect(a?.b.isB)"##:
##"Testing.__checkPropertyAccess(a?.b.self, getting: { $0?.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a?.b"), .__fromSyntaxNode("isB")), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
##"Testing.__checkPropertyAccess(a?.b.self, getting: { $0?.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a?.b"), "isB"), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
##"#expect(a?.b().isB)"##:
##"Testing.__checkPropertyAccess(a?.b().self, getting: { $0?.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a?.b()"), .__fromSyntaxNode("isB")), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
##"Testing.__checkPropertyAccess(a?.b().self, getting: { $0?.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a?.b()"), "isB"), comments: [], isRequired: false, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
##"#expect(isolation: somewhere) {}"##:
##"Testing.__checkClosureCall(performing: {}, expression: .__fromSyntaxNode("{}"), comments: [], isRequired: false, isolation: somewhere, sourceLocation: Testing.SourceLocation.__here()).__expected()"##,
]
Expand Down Expand Up @@ -163,13 +163,13 @@ struct ConditionMacroTests {
##"#require(a, sourceLocation: someValue)"##:
##"Testing.__checkValue(a, expression: .__fromSyntaxNode("a"), comments: [], isRequired: true, sourceLocation: someValue).__required()"##,
##"#require(a.isB)"##:
##"Testing.__checkPropertyAccess(a.self, getting: { $0.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a"), .__fromSyntaxNode("isB")), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"Testing.__checkPropertyAccess(a.self, getting: { $0.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a"), "isB"), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"#require(a???.isB)"##:
##"Testing.__checkPropertyAccess(a.self, getting: { $0???.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a"), .__fromSyntaxNode("isB")), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"Testing.__checkPropertyAccess(a.self, getting: { $0???.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a"), "isB"), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"#require(a?.b.isB)"##:
##"Testing.__checkPropertyAccess(a?.b.self, getting: { $0?.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a?.b"), .__fromSyntaxNode("isB")), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"Testing.__checkPropertyAccess(a?.b.self, getting: { $0?.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a?.b"), "isB"), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"#require(a?.b().isB)"##:
##"Testing.__checkPropertyAccess(a?.b().self, getting: { $0?.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a?.b()"), .__fromSyntaxNode("isB")), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"Testing.__checkPropertyAccess(a?.b().self, getting: { $0?.isB }, expression: .__fromPropertyAccess(.__fromSyntaxNode("a?.b()"), "isB"), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"#require(isolation: somewhere) {}"##:
##"Testing.__checkClosureCall(performing: {}, expression: .__fromSyntaxNode("{}"), comments: [], isRequired: true, isolation: somewhere, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
]
Expand All @@ -183,7 +183,7 @@ struct ConditionMacroTests {
@Test("Unwrapping #require() macro",
arguments: [
##"#require(Optional<Int>.none)"##:
##"Testing.__checkPropertyAccess(Optional<Int>.self, getting: { $0.none }, expression: .__fromPropertyAccess(.__fromSyntaxNode("Optional<Int>"), .__fromSyntaxNode("none")), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"Testing.__checkPropertyAccess(Optional<Int>.self, getting: { $0.none }, expression: .__fromPropertyAccess(.__fromSyntaxNode("Optional<Int>"), "none"), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"#require(nil ?? 123)"##:
##"Testing.__checkBinaryOperation(nil, { $0 ?? $1() }, 123, expression: .__fromBinaryOperation(.__fromSyntaxNode("nil"), "??", .__fromSyntaxNode("123")), comments: [], isRequired: true, sourceLocation: Testing.SourceLocation.__here()).__required()"##,
##"#require(123 ?? nil)"##:
Expand Down
57 changes: 56 additions & 1 deletion Tests/TestingTests/IssueTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,60 @@ final class IssueTests: XCTestCase {
await fulfillment(of: [expectationChecked], timeout: 0.0)
}

func testPropertyAccessExpressionExpansion() async {
let expectationFailed = expectation(description: "Expectation failed")

var configuration = Configuration()
configuration.eventHandler = { event, _ in
guard case let .issueRecorded(issue) = event.kind,
case let .expectationFailed(expectation) = issue.kind
else {
return
}

let desc = expectation.evaluatedExpression.expandedDescription()
XCTAssertEqual(desc, "!([].isEmpty → true)")
expectationFailed.fulfill()
}

await Test {
#expect(![].isEmpty)
}.run(configuration: configuration)
await fulfillment(of: [expectationFailed], timeout: 0.0)
}

func testChainedOptionalPropertyAccessExpressionExpansion() async {
let expectationFailed = expectation(description: "Expectation failed")

var configuration = Configuration()
configuration.eventHandler = { event, _ in
guard case let .issueRecorded(issue) = event.kind,
case let .expectationFailed(expectation) = issue.kind
else {
return
}

let desc = expectation.evaluatedExpression.expandedDescription()
XCTAssertEqual(desc, "(outer.middle.inner → Inner(value: nil)).value → nil")
expectationFailed.fulfill()
}

await Test {
struct Outer {
struct Middle {
struct Inner {
var value: Int? = nil
}
var inner = Inner()
}
var middle = Middle()
}
let outer = Outer()
_ = try #require(outer.middle.inner.value)
}.run(configuration: configuration)
await fulfillment(of: [expectationFailed], timeout: 0.0)
}

func testExpressionLiterals() async {
func expectIssue(containing content: String, in testFunction: @escaping @Sendable () async throws -> Void) async {
let issueRecorded = expectation(description: "Issue recorded")
Expand Down Expand Up @@ -1206,7 +1260,8 @@ final class IssueTests: XCTestCase {
return
}
let expression = expectation.evaluatedExpression
XCTAssertTrue(expression.expandedDescription().contains("<not evaluated>"))
let expandedDescription = expression.expandedDescription()
XCTAssertTrue(expandedDescription.contains("<not evaluated>"), "expandedDescription: \(expandedDescription)")
}

@Sendable func rhs() -> Bool {
Expand Down