[Swift Export] Export context parameters as tuple parameter#5690
[Swift Export] Export context parameters as tuple parameter#5690rickclephas wants to merge 8 commits intoJetBrains:masterfrom
Conversation
|
/dry-run |
|
THIS IS A DRY RUN Quality gate is triggered at https://buildserver.labs.intellij.net/build/875010588 — use this link to get full insight. Quality gate was triggered with the following revisions:
|
|
Quality gate finished successfully (recovered via auxiliary quality gate). |
There was a problem hiding this comment.
I don't think this is the good place for this logic.
Since SIR tries to model swift as closely as possible for our task, SirPrinter shall only be concerned with pretty-printing and avoiding accidental syntactical errors. Choosing the shape and position for context parameters goes a bit beyond that.
I believe the better approach would be to let Sir-nodes store a single context parameter, while its name and type would be determined during modelling – somewhere around TypeTranslationUtils, for example.
There was a problem hiding this comment.
What is the reasoning behind INVARIANT here, compared to forwarding variance passed to the function?
| class SirTupleType( | ||
| val types: List<Pair<String?, SirType>>, | ||
| override val attributes: List<SirAttribute> = emptyList(), | ||
| ) : SirWrappedType { | ||
| init { | ||
| val supportsNames = types.size > 1 // 0 = Void, 1 = technically not a tuple | ||
| require(supportsNames || types.all { it.first == null }) { | ||
| "Named tuple types are not supported for tuples with ${types.size} types" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It is important to support equals/hashCode on types since certain our utilities rely on their value-like semantics.
There was a problem hiding this comment.
Should we maybe instead ban creating 0/1 tuples altogether? Having to e.g. maintain overload resolution while keeping in mind a 0-tuple might be posing as Void will be nightmarish I imagine.
There was a problem hiding this comment.
I guess we could ban the 0 tuple, we don't really need it anyway. The 1 "tuple" actually make the rest of the code a little more straightforward as we don't have to make exceptions for single vs multiple types.
There was a problem hiding this comment.
1 tuple is still a hard case for the rest of our code. Consider the following example:
context(arg: Int)
fun foo() = TODO()
fun bar(arg: Int) = TODO()These shall produce the following declarations swift:
fun foo(_ context: Int) { /*....*/ }
fun bar(arg: Int) { /*....*/ }
As you can see, the only thing that prevents foo and bar from having the exact same signature in swift is the fact that we don't support exporting function arguments as unnamet yet. If we do – this would be an error in swift. We have mechanisms to detect such cases, but right now for them to be able to handle this we have to update them with the knowledge that SirTupleType(listOf(T)) == T. Same goes for override / protocol witness resolution.
On the other hand, the current code in SirBridgeProviderImpl is only avoiding explicit handling of zero-one-many cases by hiding the fact behind a clever hack with tuple-destructuring syntax (let (arg, ...) = context). Point is, these different cases are already there from a certain perspective.
There was a problem hiding this comment.
Good point, will update to also forbid single element "tuple".
There was a problem hiding this comment.
AFAIK Kotlin Native passes context parameters as regular ones, right after the regular ones. Considering that, we might just do the same in an (I admit, rather moonshot) attempt to open path for compiler optimizations.
There was a problem hiding this comment.
Putting them at the end kinda ruins the DX, no? Also since it's a tuple parameter in Swift and separate parameters in Kotlin, would the compiler be smart enough to optimize this?
There was a problem hiding this comment.
I meant precisely in kotlin bridges – no user-facing code involved :)
would the compiler be smart enough to optimize this
I know that the question was about swift, but still: from what I've seen, the compier is indeed smart enough to optimize kotlin bridges (at least in simpler cases) – and I hope one day we can do even more, specifically for things marked @ExportedKotlinBridge :)
There was a problem hiding this comment.
I personally would expect them to be earlier in the parameters list rather then later. First position feels right to me.
Code Owners
|
^KT-82568 Fixed
rickclephas
force-pushed
the
feature/KT-84133-swift-export-context-parameters
branch
from
cc00ff4 to
0b6afaf
Compare
|
/dry-run |
|
THIS IS A DRY RUN Quality gate is triggered at https://buildserver.labs.intellij.net/build/880809005 — use this link to get full insight. Quality gate was triggered with the following revisions:
|
4 participants
^KT-82568 Fixed
Note: context parameters on functional types will be implemented as part of KT-84263.