Add default keypath method handling for #Predicate.

[amritpan]

This accompanies the swift-syntax and compiler implementation for Method and Initializer Keypaths which extends keypath usage to include references to methods and initializers.

Key path.method handling is currently unimplemented for #Predicate and will be added once this feature is accepted and slotted to be made available in a future version of Swift.

[jmschonfeld]

Unfortunately this is actually something we don't want to emit with #Predicate. Since method() is a function call that needs to be mapped to a predicate operator, instead I think this should emit something like

 \(foundationModuleName).Predicate<Object>({ inputA in
    PredicateExpressions.build_filter(
        PredicateExpressions.build_Arg(inputA),
        {
            PredicateExpressions.build_method(
                PredicateExpressions.build_Arg($0)
            )
        }
    )
})

[amritpan]

Ah so my test was originally similar to this as well, however it gave me this error when I ran the test and so I updated it to match what it suggested:

PredicateMacroFunctionCallTests.swift:350: error: -[FoundationMacrosTests.PredicateMacroFunctionCallTests testFilter] : XCTAssertEqual failed: ("FoundationEssentials.Predicate<Object>({ inputA in
    PredicateExpressions.build_filter(
        PredicateExpressions.build_Arg(inputA),
        {
            PredicateExpressions.build_KeyPath(
                root: PredicateExpressions.build_Arg($0),
                keyPath: \.method
            )
        }
    )
})") is not equal to ("FoundationEssentials.Predicate<Object>({ inputA in
    PredicateExpressions.build_filter(
        PredicateExpressions.build_Arg(inputA),
        {
            PredicateExpressions.build_method(
                PredicateExpressions.build_Arg($0)
            )
        }
    )
})")
/Users/amritpankaur/swift-project/swift-foundation/Tests/FoundationMacrosTests/PredicateMacroFunctionCallTests.swift:350: error: -[FoundationMacrosTests.PredicateMacroFunctionCallTests testFilter] : XCTAssertEqual failed: ("FoundationEssentials.Expression<Object>({ inputA in
    PredicateExpressions.build_filter(
        PredicateExpressions.build_Arg(inputA),
        {
            PredicateExpressions.build_KeyPath(
                root: PredicateExpressions.build_Arg($0),
                keyPath: \.method
            )
        }
    )
})") is not equal to ("FoundationEssentials.Expression<Object>({ inputA in
    PredicateExpressions.build_filter(
        PredicateExpressions.build_Arg(inputA),
        {
            PredicateExpressions.build_method(
                PredicateExpressions.build_Arg($0)
            )
        }
    )
})")

[jmschonfeld]

Hmm ok yeah I think the test that you had is right, to me that suggests that there may be something wrong with the implementation. I think we need to figure out what's wrong with the implementation and update that to match the test expectations. Looking at the implementation, it looks like it might be related to the #if FOUNDATION_FRAMEWORK. I think this should be #if !FOUNDATION_FRAMEWORK (note the !) to make sure that your new code is actually compiled. Otherwise I'm surprised this actually compiled given it wouldn't have the right case - it almost seems like it's picking the .property case instead of the .method case - are you able to step into that function and see what's going on?

[amritpan]

swift-test fails early if I update the check to #if !FOUNDATION_FRAMEWORK:

swift-foundation % swift test --filter PredicateMacroFunctionCallTests
Building for debugging...
/Users/amritpankaur/swift-project/swift-foundation/Sources/FoundationMacros/PredicateMacro.swift:278:32: warning: 'init(leadingTrivia:_:calledExpression:_:_:arguments:_:_:trailingClosure:_:additionalTrailingClosures:_:trailingTrivia:)' is deprecated: Use the initializer that does not provide default values for leftParen and rightParen.
 276 |             let closure = ClosureExprSyntax(statements: [CodeBlockItemSyntax(item: CodeBlockItemSyntax.Item(node))])
 277 |             let functionMember = MemberAccessExprSyntax(base: visited, name: "flatMap")
 278 |             let functionCall = FunctionCallExprSyntax(calledExpression: functionMember, arguments: [], trailingClosure: closure)
     |                                `- warning: 'init(leadingTrivia:_:calledExpression:_:_:arguments:_:_:trailingClosure:_:additionalTrailingClosures:_:trailingTrivia:)' is deprecated: Use the initializer that does not provide default values for leftParen and rightParen.
 279 |             return ExprSyntax(functionCall)
 280 |         }

/Users/amritpankaur/swift-project/swift-foundation/Sources/FoundationMacros/PredicateMacro.swift:397:19: error: type 'KeyPathComponentSyntax.Component' has no member 'method'
 395 |                 result = ExprSyntax(SubscriptCallExprSyntax(calledExpression: result, arguments: sub.arguments))
 396 | #if !FOUNDATION_FRAMEWORK
 397 |             case .method(let method):
     |                   `- error: type 'KeyPathComponentSyntax.Component' has no member 'method'
 398 |                 result = ExprSyntax(FunctionCallExprSyntax(calledExpression: method, arguments: method.arguments))
 399 |             default:
[4/6] Compiling FoundationMacros PredicateMacro.swift
error: fatalError

[jmschonfeld]

Sounds like it might not be building against your swift-syntax change but rather swift-syntax's main branch (which is the default unless you manually change the package manifest), your swift-syntax change is the one that introduced the .method case right?

[amritpan]

I have updated Package.swift and am able to build against the swift-syntax branch with changes, but the incorrect tests continue to pass. The key path tests that I've added are never hitting the .method case in PredicateMacro.asDirectExpression which could be the explanation, but I'm unsure what to do about it. I just pushed changes to show what I have so far.

[jmschonfeld]

I noticed that you added an experimental feature flag to the FoundationMacros target - I suspect that the issues you're having are likely due to this. Does the experimental feature flag enable the parsing of method key paths in swift-syntax? I don't think adding it to the FoundationMacros target is what you want as that will apply it to the source code in the target itself (i.e. the source code in PredicateMacro.swift). Instead you want to apply it to the source code that is expanded in the tests (note - not the test target itself but rather the source code that the tests provide to the swift parser to parse). Unfortunately I don't know how to provide a list of experimental feature flags to the parser - is there an API we need to use to enable that feature flag in the parser invocation in the unit tests?

[amritpan]

For additional context, once I was able to build foundation with the correct swift-syntax branch locally, I got these errors:

/.../swift-project/swift-foundation/Sources/FoundationMacros/PredicateMacro.swift:397:19: error: 'method' is inaccessible due to '@_spi' protection level
 395 |                 result = ExprSyntax(SubscriptCallExprSyntax(calledExpression: result, arguments: sub.arguments))
 396 | #if !FOUNDATION_FRAMEWORK
 397 |             case .method(let method):
     |                   `- error: 'method' is inaccessible due to '@_spi' protection level
 398 |                 result = ExprSyntax(FunctionCallExprSyntax(calledExpression: method, arguments: method.arguments))
 399 |             default:

/.../swift-project/swift-syntax/Sources/SwiftSyntax/generated/syntaxNodes/SyntaxNodesJKLMN.swift:32:10: note: 'method' declared here
  30 |     /// - Note: Requires experimental feature `keypathWithMethodMembers`.
  31 |     @_spi(ExperimentalLanguageFeatures)
  32 |     case method(KeyPathMethodComponentSyntax)
     |          `- note: 'method' declared here
  33 |     case `subscript`(KeyPathSubscriptComponentSyntax)
  34 |     case optional(KeyPathOptionalComponentSyntax)

which I understood to mean that PredicateMacros needed the key path method implementation in swift-syntax (currently barricaded behind the experimental flag in swift-syntax) and so I added @_spi(ExperimentalLanguageFeatures) import SwiftSyntax to PredicateMacro and then added the specific swift-syntax flag (keypathWithMethodMembers) to the FoundationMacros target. Do you think we still need to add this to the test target source code instead?

Unfortunately I don't know how to provide a list of experimental feature flags to the parser - is there an API we need to use to enable that feature flag in the parser invocation in the unit tests?

This is a good question and other than the approach that I tried above, I also am not sure. @ahoppen has been helping me with the syntax-syntax changes- Alex, do you happen to know how we could accomplish this?

[jmschonfeld]

Ah ok I misunderstood that experimental flag - ok that makes sense why you'd need that on the macro target so that you can use the experimental APIs in swift-syntax. How does that feature flag affect source code that's being parsed by swift-syntax? I ran your code locally and indeed it looks like the \.method() syntax is being parsed as a property with the ( and ) being unexpected nodes rather than as a function which is why the macro isn't expanding as we expect so it sounds like either we're not passing the right feature flags to the swift-syntax parser (if that's what the parser capability is gated behind) or there might be a bug in the swift-syntax implementation. Alex might know how we can pass those feature flags down to the parser from our macro tests if that's what's preventing the parser from creating this new component type in the node

[ahoppen]

Parser has initializers that take the list of experimental features to enable, eg. here. But looking at the patch, I think it might be easiest to just cover .method as part of the default case and add support for method key paths in swift-foundation once it’s no longer an experimental feature.

[ahoppen]

Parser has initializers that take the list of experimental features to enable, eg. here. But looking at the patch, I think it might be easiest to just cover .method as part of the default case and add support for method key paths in swift-foundation once it’s no longer an experimental feature.

[jmschonfeld]

I think the default of return nil here for .method works well until it's no longer an experimental feature, but I don't know if this code actually gets us there - since it's behind #if FOUNDATION_FRAMEWORK, it won't actually be included when building the open source swift toolchain (FOUNDATION_FRAMEWORK is only defined when building this code as part of the Foundation.framework library shipped on apple platforms). I'd expect that this change would hit the same build failures you'd experience without this change when landing the new enum case. Instead, what if we remove the FOUNDATION_FRAMEWORK entirely here and just change default to @unknown default to silence the compiler warning about using default in nonresilient modules. That way .method just falls into the default case until we officially support it later (when it's no longer experimental). Would that allow this to build with your changes?

[amritpan]

Thank you for pointing out @unknown default and yes it should work! The issues that has resulted in this patch are not showing up for me locally but they do when swift-syntax runs their CI to merge my keypath method implementation there so hopefully this will suffice.

[jmschonfeld]

@swift-ci please test

[jmschonfeld]

@amritpan thanks for iterating on this! I think this is looking good now - CI shows that the change builds fine (with no additional warnings) with the current swift-syntax so I think this is ready to merge. Want me to go ahead and merge this now, or do you want to run CI on the swift-syntax repo with this change and your swift-syntax change to make sure that this still builds with your changes as well before we merge?

[amritpan]

@jmschonfeld Thank you for your guidance in getting this right - please merge!