Implement `@dynamicCallable`. #20305

dan-zheng · 2018-11-03T02:41:58Z

This PR introduces the @dynamicCallable attribute, which enables nominal types to be "callable" via a simple syntactic sugar.

This is the implementation of SE-0216. Read the proposal for more details.

@dynamicCallable is a follow-up to SE-0195 "Dynamic Member Lookup".

Forum discussion regarding @dynamicCallable:

Proposal review and acceptance rationale:

Implementation notes:

The @dynamicCallable implementation has been revamped according to proposal clarifications.
- The main clarification is about the precise rules for dynamic call ambiguity resolution.
- The rules are based solely on call-site syntax.
  - If a @dynamicCallable type implements the withArguments: method and it is called with no keyword arguments, use the withArguments: method.
  - In all other cases, attempt to use the withKeywordArguments: method.
- The rules are intentionally simple. Advanced return type overloading (example) is not supported.
@dynamicCallable types with multiple dynamicallyCall methods are supported.
- A type may have multiple dynamicallyCall methods if it inherits from a @dynamicCallable superclass or conforms to @dynamicCallable protocols, or if it itself defines overloaded dynamicallyCall methods.
- To support multiple dynamicallyCall methods, the implementation creates a disjunction constraint over all dynamicallyCall methods, and pairs it with a DynamicCallableApplicableFunction constraint.
  - Originally, I tried pairing the disjunction with an ApplicableFunction constraint, but encountered a crash involving a parameter label count mismatch in constraints::matchCallArguments.
    - This crash occurs because dynamic calls are not an exact sugar: the dynamic call expression may contain multiple parameters (e.g. three parameters in x(1, 2, 3)) but the dynamicallyCall method defines only one parameter (e.g. one parameter in x.dynamicallyCall(withArguments: [Int]).
    - AFAIK, there's no precedent for such mismatches (where the parameter count for a CallExpr and the parameter count of the function it calls don't match).
  - To work around the crash, I found it necessary to add a new DynamicallyCallableApplicableFunction constraint kind. simplifyDynamicCallableApplicableFnConstraint has custom logic for checking parameter types and doesn't produce the crash.
Please look at test/attr/attr_dynamic_callable.swift to see what functionality is supported/unsupported.

dan-zheng · 2018-11-03T03:02:54Z

The implementation has been revised to match the proposal.

Dynamic calls are resolved based on call-site syntax.
- Use the withArguments: method if it's defined and there are no keyword arguments.
- Otherwise, use the withKeywordArguments: method.

Additionally, I decided to support multiple dynamicallyCall methods.
This makes @dynamicCallable dynamic call resolution match the behavior of normal protocol requirement resolution and @dynamicMemberLookup resolution.

This enables two scenarios:
- Overloaded dynamicallyCall methods on a single @dynamicCallable type.
- Multiple dynamicallyCall methods from a @dynamicCallable superclass or from @dynamicCallable protocols.
Add DynamicCallableApplicableFunction constraint. This, used with an overload set, is necessary to support multiple dynamicallyCall methods.

Please look at the test file to see what's supported/unsupported.

- Implement dynamically callable types as proposed in SE-0216. - Dynamic calls are resolved based on call-site syntax. - Use the `withArguments:` method if it's defined and there are no keyword arguments. - Otherwise, use the `withKeywordArguments:` method. - Support multiple `dynamicallyCall` methods. - This enables two scenarios: - Overloaded `dynamicallyCall` methods on a single `@dynamicCallable` type. - Multiple `dynamicallyCall` methods from a `@dynamicCallable` superclass or from `@dynamicCallable` protocols. - Add `DynamicCallableApplicableFunction` constraint. This, used with an overload set, is necessary to support multiple `dynamicallyCall` methods.

lattner · 2018-11-03T05:46:26Z

Awesome Dan!

dan-zheng · 2018-11-03T06:20:09Z

lib/Sema/CSSimplify.cpp

@@ -4432,6 +4432,9 @@ ConstraintSystem::simplifyApplicableFnConstraint(
  ConstraintLocatorBuilder outerLocator =
    getConstraintLocator(anchor, parts, locator.getSummaryFlags());

+  // Before stripping optional types, save original type for handling
+  // @dynamicCallable applications. This supports the fringe case where
+  // `Optional` itself is extended with @dynamicCallable functionality.


There is a test exercising this fringe case:

extension Optional : KeywordCallableProtocol {} func testExtensions() { let x: Int? = 3 // Test `Optional` extension. print(x()) print(x(label: 1, 2)) }

If origType2 is not used and optional types are stripped, the test fails:

/Users/dan/swift-build/swift/test/attr/attr_dynamic_callable.swift:249:10: error: unexpected error produced: cannot call value of non-function type 'Int?' print(x()) ^ /Users/dan/swift-build/swift/test/attr/attr_dynamic_callable.swift:250:10: error: unexpected error produced: cannot call value of non-function type 'Int?' print(x(label: 1, 2)) ^

dan-zheng · 2018-11-03T06:20:55Z

@swift-ci Please smoke test

rudkx · 2018-11-07T01:21:13Z

@swift-ci build toolchain

dan-zheng · 2018-11-07T18:39:23Z

I added some implementation notes to the PR description.

@rudkx I wonder if you (or other code owners) have the time to please review this PR?
I'd really like to land it before the Swift 5 cutoff. Thank you for all your feedback/advice so far!

lattner

Please make sure the code fits in 80 columns, but it otherwise looks ok to me.

@rudkx do you mind if Dan merges this? We can continue to improve it in master.

lattner · 2018-11-09T02:02:54Z

lib/Sema/CSApply.cpp

+                                   /*dotLoc=*/SourceLoc(), choice,
+                                   DeclNameLoc(fn->getEndLoc()),
+                                   selected->openedType, locator, ctorLocator,
+                                   /*Implicit=*/true, choice.getFunctionRefKind(),


Please fit in 80 columns.

lattner · 2018-11-09T02:03:17Z

lib/Sema/CSSimplify.cpp

+    auto cand = cast<FuncDecl>(choice.getDecl());
+    return !isValidDynamicCallableMethod(cand, decl, CS.TC, hasKeywordArgs);
+  };
+  candidates.erase(std::remove_if(candidates.begin(), candidates.end(), filter),


Here too :)

Very minor, but:

This line was actually at 80 columns long, actually (i.e. the last character was on column 80).
I wonder if the 80 column convention is:

The last character of a line can be on column 80 (or before). I've been following this.

The last character of a line can be on column 79 (or before). I suppose this makes the line "fit within 80 columns".

Anyhow, I changed this line and other lines that end on column 80 so that they end on column 79.

lattner · 2018-11-09T02:04:00Z

lib/Sema/CSSimplify.cpp

+    lookupDynamicCallableMethods(type, CS, locator, ctx.Id_withArguments,
+                                 /*hasKeywordArgs*/ false);
+  methods.keywordArgumentsMethods =
+    lookupDynamicCallableMethods(type, CS, locator, ctx.Id_withKeywordArguments,


rudkx · 2018-11-09T02:24:28Z

Yes it's fine to merge. I did a first pass over it yesterday and intended on taking a closer look today but haven't had a chance. I think the risk is very small that it will cause issues with code that isn't using the new feature. It looks like the source compatibility suite is kind of back into shape today so it would be good to let a run of that complete before hitting merge.

rudkx · 2018-11-09T02:24:37Z

@swift-ci Please test source compatibility

@lattner

- Fit lines within 80 columns (addresses feedback by @lattner). - Rename `DictionaryLiteral` to `KeyValuePairs`.

dan-zheng · 2018-11-09T02:45:55Z

Thank you @lattner and @rudkx!
@lattner: I addressed in your comments in c4189f3.

EDIT: I may have interrupted CI with my latest commit. Please feel free to run smoke/compatibility tests again.

rudkx · 2018-11-09T03:39:01Z

@swift-ci Please smoke test

rudkx · 2018-11-09T03:39:08Z

@swift-ci Please test source compatibility

dan-zheng · 2018-11-09T06:49:02Z

It seems the source compatibility suite failed due to a CI error: FATAL: command execution failed.
Rerunning. @swift-ci Please test source compatibility

Fix indentation and tweak comments.

dan-zheng · 2018-11-09T13:44:59Z

All smoke/source compatibility tests passed!
I made some final NFC edits in 14e28ac. Retriggering tests now.

dan-zheng · 2018-11-09T14:02:33Z

@swift-ci Please smoke test

dan-zheng · 2018-11-09T14:02:54Z

@swift-ci Please test source compatibility

dan-zheng · 2018-11-09T16:13:41Z

To reviewers: I'll leave merging the PR to you. After you've reviewed and feel confident.

lattner · 2018-11-09T17:50:02Z

Thank you Dan!!! Please update the ChangeLog.md file if you haven't already!

dan-zheng · 2018-11-09T17:59:55Z

Yay! I'm happy to land this PR after first opening it in June. 😛Thanks for your patience!
I'd like to express thanks for everyone who gave me guidance and feedback along the way.

I'll update the changelog soon.
EDIT: Changelog updated in #20468. (I noticed there isn't a changelog entry for @dynamicMemberLookup.)

EDIT 2: I plan to write a blog post documenting my experience implementing @dynamicCallable and why it took so long. I'll write about the constraint system and various implementation challenges. 😃

dan-zheng · 2018-11-09T20:11:31Z

lib/Sema/CSSimplify.cpp

+  // If the right-hand side is not a function type, it must be a valid
+  // @dynamicCallable type. Attempt to get valid `dynamicallyCall` methods.
+  auto methods = getDynamicCallableMethods(desugar2, *this, locator);
+  if (!methods.isValid()) return SolutionKind::Error;


@slavapestov: you gave this feedback on the old implementation PR:

Also you should not emit diagnostics during constraint simplification because we can explore different parts of the solution space as we visit disjunctions, and discard invalid paths.

I made sure not to emit diagnostics here, during simplification. Instead, I return SolutionKind::Error and added diagnostic logic in CSDiag.cpp.

dan-zheng · 2018-11-09T20:16:18Z

FYI: I'll try to push two extra PRs before the Swift 5 cutoff:

Fix @dynamicMemberLookup for protocol/archetype types. #20516: Fix subscript(dynamicMember:) as a requirement for @dynamicMemberLookup protocols.
- Addresses SR-8077.
- I fixed this bug for @dynamicCallable, need to backport the logic to @dynamicMemberLookup.
Gardening for @dynamicMemberLookup. #20498: Unify implementation styles of @dynamicMemberLookup and @dynamicCallable.
- This involves using consistent variable names, doc comment format, etc.

dan-zheng added the swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process label Nov 3, 2018

This was referenced Nov 3, 2018

[WIP] Implement dynamically callable types (@dynamicCallable). #16980

Closed

Clarify SE-2016: user-defined dynamically callable types. swiftlang/swift-evolution#948

Merged

dan-zheng force-pushed the dynamic-callable-types branch 2 times, most recently from 41f9f15 to 2b358e3 Compare November 3, 2018 03:00

dan-zheng force-pushed the dynamic-callable-types branch from 2b358e3 to e651ed8 Compare November 3, 2018 05:03

[NFC] Add/clarify comments.

803dbfb

dan-zheng force-pushed the dynamic-callable-types branch from e2e426a to 803dbfb Compare November 3, 2018 06:15

dan-zheng commented Nov 3, 2018

View reviewed changes

rudkx requested review from xedin and rudkx November 7, 2018 19:09

lattner approved these changes Nov 9, 2018

View reviewed changes

rudkx approved these changes Nov 9, 2018

View reviewed changes

[NFC] Minor edits.

c4189f3

- Fit lines within 80 columns (addresses feedback by @lattner). - Rename `DictionaryLiteral` to `KeyValuePairs`.

dan-zheng force-pushed the dynamic-callable-types branch from e64808e to 06e2b0d Compare November 9, 2018 13:42

[NFC] Minor edits.

14e28ac

Fix indentation and tweak comments.

dan-zheng force-pushed the dynamic-callable-types branch from 06e2b0d to 14e28ac Compare November 9, 2018 13:43

lattner merged commit 2a4e1b8 into swiftlang:master Nov 9, 2018

dan-zheng mentioned this pull request Nov 9, 2018

[ChangeLog] Add entry for SE-0216. #20468

Merged

dan-zheng commented Nov 9, 2018

View reviewed changes

dan-zheng mentioned this pull request Nov 11, 2018

[SR-8077] Compile error when using @dynamicMemberLookup as a protocol requirement #50610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `@dynamicCallable`. #20305

Implement `@dynamicCallable`. #20305

dan-zheng commented Nov 3, 2018 •

edited

Loading

dan-zheng commented Nov 3, 2018

lattner commented Nov 3, 2018

dan-zheng Nov 3, 2018

dan-zheng commented Nov 3, 2018

rudkx commented Nov 7, 2018

dan-zheng commented Nov 7, 2018

lattner left a comment

lattner Nov 9, 2018

lattner Nov 9, 2018

dan-zheng Nov 9, 2018

lattner Nov 9, 2018

rudkx commented Nov 9, 2018

rudkx commented Nov 9, 2018

dan-zheng commented Nov 9, 2018 •

edited

Loading

rudkx commented Nov 9, 2018

rudkx commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

lattner commented Nov 9, 2018

dan-zheng commented Nov 9, 2018 •

edited

Loading

dan-zheng Nov 9, 2018

dan-zheng commented Nov 9, 2018 •

edited

Loading

Implement @dynamicCallable. #20305

Implement @dynamicCallable. #20305

Conversation

dan-zheng commented Nov 3, 2018 • edited Loading

dan-zheng commented Nov 3, 2018

lattner commented Nov 3, 2018

dan-zheng Nov 3, 2018

Choose a reason for hiding this comment

dan-zheng commented Nov 3, 2018

rudkx commented Nov 7, 2018

dan-zheng commented Nov 7, 2018

lattner left a comment

Choose a reason for hiding this comment

lattner Nov 9, 2018

Choose a reason for hiding this comment

lattner Nov 9, 2018

Choose a reason for hiding this comment

dan-zheng Nov 9, 2018

Choose a reason for hiding this comment

lattner Nov 9, 2018

Choose a reason for hiding this comment

rudkx commented Nov 9, 2018

rudkx commented Nov 9, 2018

dan-zheng commented Nov 9, 2018 • edited Loading

rudkx commented Nov 9, 2018

rudkx commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

dan-zheng commented Nov 9, 2018

lattner commented Nov 9, 2018

dan-zheng commented Nov 9, 2018 • edited Loading

dan-zheng Nov 9, 2018

Choose a reason for hiding this comment

dan-zheng commented Nov 9, 2018 • edited Loading

Implement `@dynamicCallable`. #20305

Implement `@dynamicCallable`. #20305

dan-zheng commented Nov 3, 2018 •

edited

Loading

dan-zheng commented Nov 9, 2018 •

edited

Loading

dan-zheng commented Nov 9, 2018 •

edited

Loading

dan-zheng commented Nov 9, 2018 •

edited

Loading