[Distributed] Allow overloads with different types; mangling can handle it #69595
Conversation
|
@swift-ci please smoke test |
lib/Sema/TypeCheckDistributed.cpp
Outdated
| for (size_t i = 0; i < candidateFunc->getParameters()->size(); ++i) { | ||
| auto lhs = firstDecl->getParameters()->get(i); | ||
| auto rhs = candidateFunc->getParameters()->get(i); | ||
| if (!lhs->getInterfaceType()->isEqual(rhs->getInterfaceType())) { |
There was a problem hiding this comment.
This makes me wonder whether instead we should augment re-declaration checker to discount async and throws on distributed func, I think that would give us the behavior we want here instead of trying to equate stuff in ad-hoc way i.e. it would diagnose
distributed func test<T>(a: T) {}
distrbiuted func test<U>(a: U) async {}
With a custom message instead of the default "invalid redeclaration of".
There was a problem hiding this comment.
Yeah maybe that's easier hmmm... I'll see if I can find where that is done.
There was a problem hiding this comment.
Very nice, that simplified things a lot!
|
I managed to remove a lot of "custom" code here, thanks for the review @xedin |
|
@swift-ci please smoke test |
| if (sig1.IsDistributed || sig2.IsDistributed) { | ||
| if (sig1.IsAsyncFunction != sig2.IsAsyncFunction) | ||
| return true; | ||
| } else { |
There was a problem hiding this comment.
Couldn't this be simplified to:
if (sig1.IsAsyncFunction != sig2.IsAsyncFunction)
return sig1.IsDistributed || sig2.IsDistributed;
There was a problem hiding this comment.
Yeah but perhaps a bit harder to understand? I thought "if any of them is distributed we handle differently" was a bit easier 🤔
There was a problem hiding this comment.
For me it’s easier to understand a more concise version - if async is different return true if one of the signatures is distributed and false otherwise.
There was a problem hiding this comment.
Seems this logic is incorrect slightly, Doug reviewed the other PR. Will do a follow up here asap
|
@swift-ci please smoke test and merge |
Description: We too aggressively prevented overloads; The code's intention was to prevent overloads ONLY on the effects of a function (async / throws), since it complicates the relationship with thunks. Overloads ARE allowed and work just fine, so this patch fixes the too naive type system checking.
Risk: Low, loosens up the too restrictive checks made on distributed funcs. The mangling scheme can handle these with no problem.
Reward: Adopters are able to write APIs in the style they're used to, e.g. "
distributed func received(SomeEvent)" and "distributed func received(OtherEvent)".Impact: Small; Only distributed function overloads are affected.
Review by: @xedin @slavapestov
Testing: CI testing; added tests to cover the situation.
Radar: rdar://117818281