-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[SE-0253] Introduce callables. #24299
Conversation
include/swift/AST/Decl.h
Outdated
@@ -5812,6 +5812,9 @@ class FuncDecl : public AbstractFunctionDecl { | |||
bool isConsuming() const { | |||
return getSelfAccessKind() == SelfAccessKind::__Consuming; | |||
} | |||
bool isCallable() const { | |||
return getName().str() == "call" && isInstanceMember(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding an IsCallable
bit to FuncDecl
?
I'd recommend not doing that, unless this is a particularly hot predicate. Cached bits like this have to be kept in sync with the ast. Also, instead of serializing it, if the bit needs to be stored, it can always be computed in the module reader.
lib/Sema/CSApply.cpp
Outdated
// FIXME(TF-444): This logic for `mutating` method calls incorrectly rejects | ||
// IUOs. Performing this ad-hoc check using `originalFn` feels hacky; | ||
// rewrite if possible. | ||
if (!cs.getType(originalFn)->hasLValueType() && selfParam->isInOut()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo: find a better way to handle/diagnose mutating func call
methods.
Then, re-enable mutating func call
+ IUO test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constraint system experts: do you have advice on supporting mutating func callFunction
methods more robustly? Perhaps appropriate constraints can be generated in CSSimplify.cpp?
Currently, function expressions in ApplyExpr
are always coerced to rvalues, which is problematic for mutating func callFunction
method calls. The current workaround in CSApply.cpp is ad-hoc and breaks for mutating func callFunction
+ IUO.
Here's a small reproducer, with -debug-constraint-solver
output:
struct Mutating {
var x: Int
mutating func callFunction() {
x += 1
}
}
func testIUO(f: inout Mutating!) {
f() // not okay
f.callFunction() // okay
}
mutating_iuo.swift:8:3: error: cannot use mutating member on immutable value: 'f' is immutable
f() // not okay
^
cc @xedin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constraint structure of ApplicableFunction
should be good enough here, so it's probably just a problem in the solver. Try doing something like this in simplifyApplicableFnConstraint
(where the existing similar call to getFixedTypeRecursive
is):
Type lvType2 = getFixedTypeRecursive(type2, flags, /*wantRValue=*/false);
type2 = lvType2->hasLValueType() ? getFixedTypeRecursive(lvType2->getRValueType(), flags, /*wantRValue=*/true) : lvType2;
Then you can use lvType2 as the base type when building your constraints for the call to callAsFunction
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the advice!
I found the blocker for mutating func callAsFunction
+ IUO. It's the following line in ExprRewriter::finishApply
in CSApply.cpp:
fn = cs.coerceToRValue(fn);
ConstraintSystem::coerceToRValue
calls TypeChecker::coerceToRValue
, which calls cs.setType
on expressions and inserts an implicit load expression. Thus, simply caching auto *originalFn = fn
before the call to cs.coerceToRValue(fn)
is not effective.
If I comment the line fn = cs.coerceToRValue(fn)
, then the mutating func callAsFunction
+ IUO example passes, but other lit tests fail.
Any advice/suggestions? If there's some way to clone fn
or "undo" the effects of cs.coerceToRValue(fn)
, that should do the trick. I tried creating an ASTWalker
to undo the effects of cs.coerceToRValue(fn)
, but I'm not sure how to nicely undo ConstraintSystem::setType
which expects a non-nullptr
Type
argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you look at the function expression and/or recorded overload choices for the call and make the decision about callable vs. dynamicCallable vs. function before doing any processing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you look at the function expression and/or recorded overload choices for the call and make the decision about callable vs. dynamicCallable vs. function before doing any processing?
Yes! I believe that's possible and may circumvent the issue. Experimenting now.
Edit: checking "function vs callAsFunction
vs @dynamicCallable
" may incur a performance impact for resolving all ApplyExpr
s in ExprRewriter::finishApply
. To be benchmarked.
lib/Sema/CSSimplify.cpp
Outdated
auto &ctx = getASTContext(); | ||
// Get all call methods of the nominal type. | ||
// TODO: Consider caching? | ||
SmallVector<FuncDecl *, 4> callMethods; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo: consider caching the func call
methods for types?
Such caching of methods is done for @dynamicCallable
. There's also a cache for @dynamicMemberLookup
types, but it doesn't store methods.
Verifying that current tests pass. |
I love how simple and clean this patch is! Maybe you should mention how simple the implementation is in the proposal? |
@swift-ci please test compiler performance |
Introduce callables: values that can be called like functions. Methods named `func callFunction` act as call-syntax delegate methods. ``` struct Adder { var base: Int func callFunction(_ x: Int) -> Int { return x + base } } var adder = Adder(base: 3) adder(10) // desugars to `adder.callFunction(10)` ```
Updated based on SE-0253 review feedback. |
Use `callFunction` instead of `call` as the call-syntax delegate method name. Add trailing closure tests.
@swift-ci Please smoke test |
include/swift/AST/Decl.h
Outdated
@@ -5958,6 +5958,9 @@ class FuncDecl : public AbstractFunctionDecl { | |||
bool isConsuming() const { | |||
return getSelfAccessKind() == SelfAccessKind::__Consuming; | |||
} | |||
bool isCallFunction() const { | |||
return getName().str() == "callFunction" && isInstanceMember(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo: unify "call function"/"call method" terminology, and use one spelling consistently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any plans to diagnose static or class callFunction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What diagnostic do you have in mind?
Originally, SE-0253 was pitched with a new call
declaration kind (e.g. call(_ x: Int) -> Int)
, which did not support static
or class
. But in the accepted proposal, call-syntax delegate methods are just func
s, so static/class func callFunction(...)
is valid.
I suppose we could add call-site diagnostics though. Example:
struct Oops {
static func callFunction() {
print("Hi Chéyo")
}
}
let oops = Oops()
oops()
diag.swift:7:5: error: cannot call value of non-function type 'Oops'
oops()
~~~~^~
We could add a more informative note (e.g. did you mean to call the static function 'Oops.callFunction'?
).
Diagnostics can always be added incrementally. 🙂
A detail is that the current diagnostics are being ported from the old system (CSDiag.cpp) to the new request-based system, so adding diagnostics to the old system might be throwaway work. Porting the "function call" diagnostics to the request-based system as a first step might be more productive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Precisely. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A detail is that the current diagnostics are being ported from the old system (CSDiag.cpp) to the new request-based system, so adding diagnostics to the old system might be throwaway work.
You can actually add diagnostics to a new diagnostic framework right now, it's live, in fact me and couple of others have ported a lot of diagnostics already, see #26074 for the most recent example of how easy it is to add new diagnostics now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the discussion in the review announcement thread, please rename the key function here callAsFunction
.
Use `callAsFunction` as the call-syntax delegate method name.
Done in 518bda2. Some notes, in order of importance:
|
Thanks. I haven't given the constraint logic a good once-over; do you want to get |
Sure thing. I missed your earlier suggestion regarding |
Would it be acceptable merge this partial implementation as is and start separate PRs later to support |
@@ -106,8 +106,12 @@ struct Mutating { | |||
} | |||
} | |||
func testMutating(_ x: Mutating, _ y: inout Mutating) { | |||
_ = x() // expected-error {{cannot use mutating member on immutable value: 'x' is a 'let' constant}} | |||
_ = x.callAsFunction() // expected-error {{cannot use mutating member on immutable value: 'x' is a 'let' constant}} | |||
// TODO: Improve this error to match the error using a direct `callAsFunction` member reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this diagnostic regressed - the diagnostic is now produced from failed member
constraint instead of custom CSApply
diagnostic hack. cc @xedin regarding ideas for improving/fixing this.
The diagnostic below in testInout
has improved however - it is now in parity with direct callAsFunction
calls.
I believe the main constraint system changes requested by @xedin are done, so this patch is ready for re-review. Some |
Verifying that tests pass. |
Build failed |
Build failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better now, thank you! Still needs a final push though.
There's one failing test (
I'm not exactly sure what is the cause. |
Maybe, I'm not sure either... Maybe |
- Rename `isValidDynamicallyCallMethod` to `isValidDynamicCallableMethod`. - Add todo hint regarding `static func callAsFunction`, if it is to be supported. - Restore `var origType2` before unwrapping optionality. - This fixes `expr/delayed-ident/static_var.swift`.
I believe all comments have been addressed (besides improving diagnostics). |
Build failed |
@swift-ci Please test macOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Add todo comment references to SR-11378, tracking the improvement of |
@swift-ci Please test |
Build failed |
Build failed |
Thank you to all reviewers! Merging now. |
…oduce callables. (#24299). They are duplicates.
Implementation for SE-0253.
Introduce callables: values that can be called like functions.
Methods named
func callFunction
act as call-syntax delegate methods.