🍒[5.10][Distributed] Allow overloads with different types; mangling can handle it #69596
Conversation
|
@swift-ci please test |
|
@swift-ci please test |
|
@swift-ci please test |
lib/AST/Decl.cpp
Outdated
| // Note also, that overloading on throws is already illegal anyway. | ||
| if (sig1.IsDistributed || sig2.IsDistributed) { | ||
| if (sig1.IsAsyncFunction != sig2.IsAsyncFunction) | ||
| return true; |
There was a problem hiding this comment.
I do not believe this is correct; if one of them is distributed, and mismatch on async, we should fall through to check the names below, not determine that they are always conflicting. They might have different argument names entirely.
There was a problem hiding this comment.
Thank you, I’ll check and fix that
There was a problem hiding this comment.
Oh it’s too early, I should have taken a closer look, sorry!
There was a problem hiding this comment.
Thank you both for reviewing! 🙏
There was a problem hiding this comment.
adjustment here #69688 though I must be blind, was not able to get the described overload scenarios fail 🤔
|
@swift-ci please test |
|
@swift-ci please test Windows |
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.
Original PR: #69595
Radar: rdar://117818281