Property accesses in failed expectations sometimes include their valu…

…e twice in expanded description
swiftlang · Sep 3, 2024 · 4960964 · 4960964
1 parent f2c8ee1
commit 4960964
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 23 deletions.
diff --git a/Sources/Testing/Expectations/ExpectationChecking+Macro.swift b/Sources/Testing/Expectations/ExpectationChecking+Macro.swift
@@ -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
@@ -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

diff --git a/Sources/Testing/SourceAttribution/Expression+Macro.swift b/Sources/Testing/SourceAttribution/Expression+Macro.swift
@@ -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))
   }
 

diff --git a/Sources/Testing/SourceAttribution/Expression.swift b/Sources/Testing/SourceAttribution/Expression.swift
@@ -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.
     ///
@@ -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 {
@@ -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:
@@ -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(
@@ -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)
@@ -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]
     }

diff --git a/Sources/TestingMacros/Support/SourceCodeCapturing.swift b/Sources/TestingMacros/Support/SourceCodeCapturing.swift
@@ -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))"

diff --git a/Tests/TestingMacrosTests/ConditionMacroTests.swift b/Tests/TestingMacrosTests/ConditionMacroTests.swift
@@ -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()"##,
     ]
@@ -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()"##,
     ]
@@ -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)"##:

diff --git a/Tests/TestingTests/IssueTests.swift b/Tests/TestingTests/IssueTests.swift
@@ -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")
@@ -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 {