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

Implement @dynamicCallable. #20305

Merged
merged 4 commits into from Nov 9, 2018
Merged

Conversation

dan-zheng
Copy link
Collaborator

@dan-zheng dan-zheng commented Nov 3, 2018

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
Copy link
Collaborator Author

dan-zheng commented Nov 3, 2018

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.
@dan-zheng dan-zheng force-pushed the dynamic-callable-types branch from 2b358e3 to e651ed8 Compare Nov 3, 2018
@lattner
Copy link
Collaborator

lattner commented Nov 3, 2018

Awesome Dan!

@dan-zheng dan-zheng force-pushed the dynamic-callable-types branch from e2e426a to 803dbfb Compare Nov 3, 2018
@@ -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.
Copy link
Collaborator Author

@dan-zheng dan-zheng Nov 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

dan-zheng commented Nov 3, 2018

@swift-ci Please smoke test

@rudkx
Copy link
Member

rudkx commented Nov 7, 2018

@swift-ci build toolchain

@dan-zheng
Copy link
Collaborator Author

dan-zheng commented Nov 7, 2018

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!

@rudkx rudkx requested review from xedin and rudkx Nov 7, 2018
lattner
lattner approved these changes Nov 9, 2018
Copy link
Collaborator

@lattner lattner left a comment

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.

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

@lattner lattner Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fit in 80 columns.

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

@lattner lattner Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too :)

Copy link
Collaborator Author

@dan-zheng dan-zheng Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@lattner lattner Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too

@rudkx
Copy link
Member

rudkx commented Nov 9, 2018

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
Copy link
Member

rudkx commented Nov 9, 2018

@swift-ci Please test source compatibility

rudkx
rudkx approved these changes Nov 9, 2018
- Fit lines within 80 columns (addresses feedback by @lattner).
- Rename `DictionaryLiteral` to `KeyValuePairs`.
@dan-zheng
Copy link
Collaborator Author

dan-zheng commented Nov 9, 2018

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
Copy link
Member

rudkx commented Nov 9, 2018

@swift-ci Please smoke test

@rudkx
Copy link
Member

rudkx commented Nov 9, 2018

@swift-ci Please test source compatibility

@dan-zheng
Copy link
Collaborator Author

dan-zheng commented Nov 9, 2018

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

@dan-zheng dan-zheng force-pushed the dynamic-callable-types branch from e64808e to 06e2b0d Compare Nov 9, 2018
Fix indentation and tweak comments.
@dan-zheng dan-zheng force-pushed the dynamic-callable-types branch from 06e2b0d to 14e28ac Compare Nov 9, 2018
@dan-zheng
Copy link
Collaborator Author

dan-zheng commented Nov 9, 2018

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

@dan-zheng
Copy link
Collaborator Author

dan-zheng commented Nov 9, 2018

@swift-ci Please smoke test

@dan-zheng
Copy link
Collaborator Author

dan-zheng commented Nov 9, 2018

@swift-ci Please test source compatibility

@dan-zheng
Copy link
Collaborator Author

dan-zheng commented Nov 9, 2018

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

@lattner lattner merged commit 2a4e1b8 into apple:master Nov 9, 2018
@lattner
Copy link
Collaborator

lattner commented Nov 9, 2018

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

@dan-zheng
Copy link
Collaborator Author

dan-zheng commented Nov 9, 2018

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. 😃

// 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;
Copy link
Collaborator Author

@dan-zheng dan-zheng Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Collaborator Author

dan-zheng commented Nov 9, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants