Amend SE-0216 Dynamic Callable: Reduce Overloads

Yep, that seems true. In the withKeywordArguments method, it's certainly possible to check if no labels are specified (i.e. all labels are the empty string).

1 Like

As an original author of SE-0216, I support this amendment.

  • Disallowing the withArguments and withKeywordArguments methods from both being specified simplifies overload resolution, reducing confusion and complexity.
  • Supporting both methods does not seem to have critical use cases. In any case, use cases that benefit from both methods being defined can still be implemented using just a withKeywordArguments method.

I believe this amendment should become a proposal and undergo review as soon as possible. This would help @dynamicCallable meet the Swift 5 final branching deadline in November.

5 Likes

Since we're talking about a dynamic method, why not look to (e.g.,) Python for an example of how to implement this? Python lets you write

def f(a, b, <other positional args>, 
      *args, 
      x1=default_value1, x2=default_value2, <other keyword args>, 
      **kwargs):

This seems like the best route to me. It would require some compiler magic, but I think the best solution would be to allow any method with a signature similar to the following to be used as dynamicallyCall:

func dynamicallyCall(x: T1, and y: T2, <other positional args, possibly labeled>, 
                     variadic: T...,
                     dict: [K: V])

This function would consume as many arguments as necessary before shoving the rest of the unlabeled arguments into variadic and the rest of the labeled arguments into dict. Naturally, any issues occurring in the manually specified arguments (everything prior to variadic) would cause a compiler error.

I think one difference here is that PythonObject (the abstraction representing any Python object, and not a specific one) is marked @dynamicCallable. This means that we want to implement the most general version of func dynamicallyCall, it'd look like def call(*args, **kwargs) in Python.

For dynamic language interop, we basically always want to implement the most general version of func dynamicallyCall, so there's no opportunity to leverage specific labeled arguments like in func dynamicallyCall(x: T1, y: T2, args: T..., kwargs: [K: V]).

@Douglas_Gregor We just chatted in person about this. Hope the core team can review this amendment proposal!

@rxwei Would you like to submit an evolution PR to elevate this to a proposal?
That'd be much appreciated. I'm not sure I can complete the @dynamicCallable implementation without this amendment (don't have a solution for representing two orthogonal sets of constraints for two disjunctions).

I don’t think this will be a proposal — it’ll most likely be a revision of the original proposal like revisions of the random number generator proposal.

1 Like

Hi all (and @dan-zheng and @rxwei) specifically:

The core team met to discuss what to do in this situation. Most importantly, it was observed that there is a misunderstanding of the proposal, and it should be completely unnecessary to use disjunction constraints in the type checker.

The proposal is explicit that ambiguity in the two cases is resolved based on the syntax of a given call site - whether keyword arguments are present or not. Return type overloading is not intended to be supported by the proposal, and supporting that would be an extension beyond what was accepted. No one is aware of motivation for making that extension.

I'm not sure if this observation is enough to unblock the implementation work (I suspect it might be), but if not, there core team also agreed that it is perfectly reasonable to incrementally land partial progress towards the entire proposal (this has occurred many times in the past), so long as the incremental progress doesn't accept invalid code and any limitations are clearly described in error messages.

Specifically in this case, it would be fine to land a patch that:

  1. Faithfully implements support for types that implement one or the other of the methods, but not both.
  2. Diagnose with a "not supported yet" error message if someone defines a type that defines both.

There was no desire or interest in having a formal review cycle to change the proposal. Thank you so much for driving this forward, I really appreciate it!

-Chris

10 Likes

Thank you Chris and the core team for taking the time to revisit this!

I agree that resolving dynamic calls based just on call site syntax makes sense, and it does unblock the implementation. I'm sorry for deviating from the proposal and getting caught up in more complicated rules.


I'll revise the implementation to follow this strategy: calls with no keyword arguments must resolve to a withArguments: method and calls with at least one keyword argument must resolve to a withKeywordArguments: method.

This strategy addresses the concerns raised in the amendment:

  1. Dynamic calls with no argument labels can only resolve to a withArguments: method. They cannot resolve to a withKeywordArguments: method.

To call the withKeywordArguments: method with empty keywords, one must do so explicitly. Examples: x.dynamicallyCall(withKeywordArguments: [:]), x.dynamicallyCall(withKeywordArguments: ["": a, "": b]).

  1. Return type overloading between withArguments: and withKeywordArguments: methods is no longer a concern.

One question is whether to support overloaded withArguments: and withKeywordArguments: methods on a @dynamicCallable type. This question is not addressed by the proposal, but I believe overloaded methods should be supported (for parity with @dynamicMemberLookup, which supports overloaded subscript(dynamicMember:) members, and Swift methods in general).

Regarding the implementation: I plan to support overloaded methods by generating an overload set disjunction over all valid methods paired with a new DynamicCallableApplicableFn constraint (details here). It makes sense to work within the constraint system, as opposed to the lookup-based approach of my original implementation. I'm not sure how to support overloaded methods without adding a new DynamicCallableApplicableFn constraint kind. (The purpose of using the new constraint kind instead of ApplicableFn is to avoid this crash in matchCallArguments.)

I anticipate I can implement the new strategy without a half-measure patch, as Chris brought up. (If I do encounter more implementation difficulties, I'll ask for help and try to land an incremental patch.)

As the original proposal author, I never expected that to be supported. This is simply an omission from the proposal. If you are concerned about this, please create a PR to address the ambiguity and we can run it by the core team to see if they want it to be officially run or whether the clarification can be treated as a bug fix. Given that it is a clarifying issue that narrows the scope of the proposal, I suspect they will just take it as a bug fix.

Constraint system disjunctions exacerbate/contribute-to/lead-to exponential behavior in the type checker. I'd prefer to completely eliminate their possibility if practical, and I see no reason to support this for this language feature. Do you see a use case?

-Chris

This doesn't really make sense to me. Python's keyword arguments are designed to be mixed with non-keyword arguments, but this forces anyone implementing Python interop to implement both methods (even if one's just going to forward to the other). What was the issue with "calls with no keyword arguments resolve to a withArguments: method if possible, but fall back to withKeywordArguments: otherwise"?

EDIT: as a purely syntactic rule, not based on types.

4 Likes

Thanks for the clarification. I suppose the proposal only states that:

  • @dynamicCallable may be applied to structs, classes, enums, and protocols.
  • Dynamic calls are resolved solely based on call site syntax. This handles the case where a @dynamicCallable type specifies both the withArguments: and withKeywordArguments: methods.

However, the proposal doesn't state whether to support, or how to handle the case where multiple dynamicallyCall methods can be found for a @dynamicCallable type.

Multiple dynamicallyCall methods can be found for a @dynamicCallable type if:

  1. The type itself defines overloaded dynamicallyCall methods.
  • From your response, I believe your stance on this is you never expected this to be supported.
  • It should be easy to disallow overloaded dynamicallyCall methods on individual types because there's no need to check inheritance/conformance to other @dynamicCallable types.
  1. The type inherits from a @dynamicCallable superclass or conforms to @dynamicCallable protocols. Somewhere in the protocol/class hierarchy, there exist multiple dynamicallyCall methods.
  • This case is trickier to handle.
  • One approach is to disallow multiple dynamicallyCall methods: emit an error if multiple dynamicallyCall methods can be found via lookup for a @dynamicCallable type. But this brings into question the purpose/usefulness of @dynamicCallable protocols. If a @dynamicCallable protocol defines a dynamicallyCall method as a protocol requirement, then any implementations of the method (defined in a protocol extension or defined on conforming types) will trigger the error (because then more than one dynamicallyCall method can be found).
    • I am not sure how exactly to scope out this approach. One answer is disallow @dynamicCallable on protocols and non-final classes. If a type wants to support dynamic calls, it must implement the dynamicallyCall methods itself - that functionality cannot be inherited. This would require an amendment to the original proposal. It also breaks precedent with @dynamicMemberLookup.
  • The other approach is to support multiple dynamicallyCall methods. The implementation can call CS.addOverloadSet to generate a disjunction over all overload choices. This mimics resolution logic for other overloaded declarations.

I would like to discuss these points (how to handle the case where multiple dynamicallyCall methods can be found for a @dynamicCallable type) and to update the original proposal when we reach consensus. What are your thoughts, and everyone's thoughts?

I don't see a strong use case for overloaded dynamicallyCall methods on a single type (case 1 above).
However, with the current proposal, I do think users may expect case 2 to work. They may declare a @dynamicCallable protocol and expect dynamic call resolution to behave like normal protocol requirement resolution. Here's an example from the proposal thread: it involves @dynamicMemberLookup but the idea applies to @dynamicCallable.

I suppose there are two extremes:

  • To provide minimal support for the motivating use case of language interop. To support dynamic callable behavior for a PythonObject struct, there's no need to support overloaded dynamicallyCall methods or @dynamicCallable protocols/non-final classes. We could disallow those cases.
  • To support full generality. @dynamicCallable can be applied on protocols and non-final classes. Dynamic calls should resolve just like direct calls to the dynamicallyCall method in all cases.

What are your thoughts, and everyone's thoughts?

I agree. It would make sense for calls with no keyword arguments to fall back to a withKeywordArguments: method.

The key implementation challenge is I'm not sure how to represent the two orthogonal sets of constraints for withArguments: and withKeywordArguments: methods. I'll explain in more detail below, perhaps you or someone has ideas for working around them?


Consider this example:

@dynamicCallable
struct Callable {
  func dynamicallyCall(withArguments: [Int]) -> Int { return 1 }
  func dynamicallyCall(withKeywordArguments: [String: Int]) -> Float { return 1.0 }
}

let x = Callable()

// Suppose we want calls with no keyword arguments to
// fall back to the `withKeywordArguments:` method.

// Then this should resolve to the `withKeywordArguments` method:
// x.dynamicallyCall(withKeywordArguments: [:])
let _: Float = x()
// This too:
// x.dynamicallyCall(withKeywordArguments: ["": 1, "": 2])
let _: Float = x(1, 2)

The current implementation works like this:

  • In visitApplyExpr in CSGen.cpp, an ApplicableFunction constraint is generated for dynamic calls like x(1, 2) .
  • In simplifyApplicableFnConstraint in CSSimplify.cpp, check whether x is a function type, metatype, or @dynamicCallable type.
  • Since x is a @dynamicCallable nominal type, look up and cache all dynamicallyCall methods defined by the type (or inherited from other @dynamicCallable protocol/class types).
  • Attempt to resolve the correct dynamicallyCall method.
    • Since the dynamic call x(1, 2) has no argument labels, withArguments: methods are preferred. I attempt to resolve the withArguments: method by generating constraints like $T5 conforms to ExpressibleByArrayLiteral and $T1 arg conv $T5.ArrayLiteralElement. $T5 is the argument type of the withArguments: method and $T1 is the type of the literal argument 1 in x(1, 2).
    • However, as a fallback, I also need to generate constraints for the withKeywordArguments: method like $T5 conforms to ExpressibleByDictionaryLiteral and $T1 arg conv $T5.Value.

How can I represent the two orthogonal sets of constraints for withArguments: and withKeywordArguments: methods? I'm not sure how to reconcile the two: generating both sets of constraints at once is sure to fail. Some kind of "union constraint" seems necessary so I can generate union(withArguments constraints) \/ union(withKeywordArguments constraints).

Do you have any ideas? Perhaps I'm thinking in the wrong mindset. Let me know if you need more information.

I'm not much of an expert on the constraint system, but in my head in your step 3 you would statically see if there are any non-keyword implementations, and if there aren't then you use the set of keyword implementations.

To provide some more context: my strategy in CSSimplify.cpp is to generate one big constraint that encapsulates everything I want to check. Then, I turn things to the constraint solver: if the constraint succeeds, I can resolve the correct method and rewrite the AST, and if not, a diagnostic is emitted.

My original strategy was to check constraints incrementally using methods like CS.matchTypes, similar to your suggestion. However, I'm not sure how to "backtrack" calls to CS.matchTypes to implement the withKeywordArguments: fallback logic. Everywhere else in CSSimplify.cpp, calls to CS.matchTypes that fail are immediately followed by return SolutionKind::Error. In other words, if I check the withArguments: constraints and they fail, I don't know how to reset the state of the constraint system so I can check the withKeywordArguments: constraints.

I see two ways out:

  • Find some way to generate one big constraints that encapsulates everything: union(withArguments constraints) \/ union(withKeywordArguments constraints). The withArguments: constraints need to be preferred.
  • Find a backtrack-able way to incrementally check constraints. Then, I can check the withArguments: constraints. If they fail, I can backtrack and check the withKeywordArguments: constraints.

This is the part I don't understand. If there are withArguments: constraints to check, then you don't check withKeywordArguments: at all. That is, at the time you make the constraints (in simplifyApplicableFnConstraint) you already know which one to be using.

Ah, I think misinterpreted you, sorry. Your original question was:

I believe you're talking about @dynamicCallable types that only define a withKeywordArguments: method:

// Case 1.
@dynamicCallable
struct Callable {
  func dynamicallyCall(withKeywordArguments: [String : Int]) {}
}
let x = Callable()
x() // desugar to `x.dynamicallyCall(withKeywordArguments: [:])`
x(1, 2) // desugar to `x.dynamicallyCall(withKeywordArguments: ["": 1, "": 2])`

This is indeed easily supportable, using the approach you mentioned. I thought you were asking about fallback for more complex cases (return type overloading):

// Case 2.
@dynamicCallable
struct Callable {
  func dynamicallyCall(withArguments: [Int]) -> Int { return 1 }
  func dynamicallyCall(withKeywordArguments: [String : Int]) -> Float { return 1 }
}
let x = Callable()
let f: Float = x(1, 2)
// desugar to `x.dynamicallyCall(withKeywordArguments: ["": 1, "": 2])`,
// even though there are no argument labels and `withArguments:` is defined.

It's certainly possible to support case 1 without supporting case 2. Do you think this is a good idea?

I haven't quite made up my mind.
On one hand, supporting case 1 is good for convenience.
On the other hand, it complicates the resolution rule from "use withKeywordArguments: only if keyword arguments are present" to "use withKeywordArguments: only if keyword arguments are present, unless withArguments: is not defined".

EDIT: I agree with supporting case 1 without supporting case 2. This aligns with my implementation strategy. I'll submit a patch PR to the proposal to make this explicitly clear.

The remaining unanswered question is whether to support cases where multiple dynamicallyCall methods can be found for a @dynamicCallable type (via multiple definitions on that type or inheritance from other @dynamicCallable types).

Whew! Yes, I was suggesting supporting case 1 without supporting case 2. I think the common case is going to be only implementing one of the two dynamicallyCall signatures, and that it's worth making that easy. The rule would then be "use withArguments: if it's defined and there are no keyword arguments; use withKeywordArguments: in all other scenarios". Slightly more complicated, but not much, IMO.

3 Likes

To give closure to this topic: after six months, the @dynamicCallable implementation has finally been completed and merged! The final implementation closely follows the proposal.

Dynamic call resolution is based on call-site syntax, as discussed in this thread:

  • If a @dynamicCallable type defines the withArguments: method and is called with no keyword arguments, use the withArguments: method.
  • In all other cases, attempt to use the withKeywordArguments: method.
  • Detailed rules here. The test file test/attr/attr_dynamic_callable.swift shows what's supported/unsupported.

I'd like to thank everyone who gave me implementation guidance and feedback along the way. Thanks also to everyone who participated in forum discussions! If you have further feedback, please share. :smile:

19 Likes

Sorry to bump again. Already, some @dynamicCallable bugs have been found.

Sharing here because they're starter-ish bugs! They can likely be solved with small changes to expression rewriting in CSApply.cpp, or maybe constraint changes in CSSimplify.cpp.

If anyone's interested in @dynamicCallable or Sema, please take a look. I'm busy the next few days and won't be able to fix these in time for Swift 5, unfortunately.

  • [*] Fixed. SR-8077: subscript(dynamicMember:) fails as a @dynamicMemberLookup protocol requirement.

    • This bug doesn't exist for @dynamicCallable. I fixed it in the @dynamicCallable implementation, then backported the logic for @dynamicMemberLookup.
  • SR-9230: Crash with protocol-typed argument for dynamicallyCall method.

    • I did some initial investigation on the bug and left some comments.
    • Reproducing example.
      // Reproducer.
      protocol Proto {}
      
      @dynamicCallable
      struct Struct : Proto {
        func dynamicallyCall(withArguments args: [Proto] = []) {}
        func dynamicallyCall(withKeywordArguments args: DictionaryLiteral<String, Proto> = [:]) -> Struct {
          return Struct()
        }
      }
      
      func crasher() {
        let a = Struct()
        // Dynamic call crashes.
        a(a(a, key: a))
      
        // Direct call doesn't crash.
        // a.dynamicallyCall(withArguments: [a.dynamicallyCall(withKeywordArguments: ["": a, "key": a])])
      
        // Simpler dynamic calls don't crash.
        // a(a, key: a)
        // a(a)
      }
      
  • SR-9239: Crash with default argument in initializer for @dynamicCallable struct.

    • Reproducing example.
      // Reproducer.
      @dynamicCallable
      struct A {
        init(_ x: Int = 0) {}
        func dynamicallyCall(withArguments args: [Int]) {}
      } 
      
      A()()
      
1 Like