-
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
Parameterized extensions #25263
base: main
Are you sure you want to change the base?
Parameterized extensions #25263
Conversation
08166ac
to
b210b8f
Compare
I've been thinking about this more, and I think extending generic type params could make sense in the future. For the sake of this PR however, I think it's reasonable to disallow for the time being. There's a lot of implementation and design space to think about for things like Just some initial thoughts, but maybe we could add members of those extensions to name lookup as methods, but lower them as global functions that take a self parameter of extension<T> T {
// This is treated as a member at the type checker level.
func abc() {}
}
let x = 3
x.abc() // ok
// T.abc() is lowered as
func abc<T>(_ self: T) {} This is essentially "sugar", but you couldn't have both the member |
Rewriting existing conditional conformance syntax to use parameterized extensions breaks ABI. There's an additional extension<T: Equatable> SomeType: Equatable {} Could we detect this and remove that redundant requirement to make switching between these two syntaxes ABI compatible? Do we even want that to be ABI compatible? |
5c476be
to
f11acf1
Compare
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.
This looks great. I think it's ready for a pitch and proposal.
Can you add some SILGen tests demonstrating the mangling you get for methods of a parametrized extension?
Also, I would test nested types inside parametrized extensions, and make sure it all works with remote mirrors (validation-test/Reflection) and type reconstruction (test/TypeDecoder) too.
@slavapestov So, I've started adding test cases for type reconstruction and it seems the demangler can't reconstruct nested types. I've added the following to test/TypeDecoder/extensions.swift: extension<T> Generic<T?> {
struct Nested3 {}
} which results in the following:
I'm unsure if maybe it's mangling the nested type wrongly, or if maybe it's a demangling type issue? When I just demangle the symbol I get:
which for the most part looks correct, except the |
d71600b
to
546430f
Compare
@slavapestov Hopefully I can explain this clearly, but the issue was that when we spun up a GSB in ASTDemangler we wouldn't ever add generic parameters that came from the extension. There wasn't enough information in the current mangling to deduce adding generic parameters, so I went ahead and mangled the full signature with parameterized extensions skipping any generic parameters from the nominal and adding the rest to GSB in ASTDemangler. |
This is going to require a little bit more work. struct Generic<T> {}
extension<U> Generic<U?> {
struct Nested {
let x: U
}
}
This is due to the fact that I've also been thinking about the mangling. Maybe we could introduce a new mangling for extensions with added generic params that way we don't have to mangle the extended type's generic signature as well. |
@DougGregor worked on the mangling for extension members most recently. Doug, can you offer some guidance here? |
9fc8724
to
95e3075
Compare
95e3075
to
9178e46
Compare
@slavapestov I've been having some difficulty pinpointing exactly where reflection fails for nested types. Is there a specific place or some general area you can recommend me looking into? I've been messing around for a while now in @jckarter I saw this little tidbit in MetadataLookup.cpp that you committed a while ago, could you explain what you mean by match up generic arguments? Also, could it be possible to do this without a new mangling? I ask because it would be great to be able to use this feature in older Swift versions, but I guess any runtime changes needed here kind of void that argument no? I actually played with a new mangling, Sorry for all the questions (and mentions), but I've been trying to get this to work for a while now. I would really appreciate any advice that you can give! |
The mangling of generic parameters currently relies on the extension having the same generic parameters as the extended type. If you have something like:
then inside the extension context,
then |
These would be mangled as depth 2 no? Because inside the extension context we would still want to support |
You could model it that way, I suppose, but that seems like it adds additional complexity. |
4fad6ae
to
ca35b5f
Compare
@davidungar I recently rebased against master and it appears that part of this is failing now due to AST scope lookup. protocol SomeProto {}
struct Generic<T> {}
extension<U: SomeProto> Generic<U> {} unqualified lookup in astscope is failing to resolve |
Chiming in: with the recent ASTScope changes, I think name lookup now uses source location ranges to find names in scope. We ran into the same Our fix was to add ad-hoc ASTScope support for the |
@dan-zheng thanks so much, will look into this! |
Alejandro,
I’m sorry you are hitting this issue. About 5 days ago I landed the second of two ASTScope fixes. They were:
#27743 <#27743> and
#27786 <#27786>
Naturally, I’m hoping that you didn’t pick these up in your rebase.
The merge with the second fix was:
e67921f on the 18th.
Of course Murphy’s law states that yours is something else. Let me know if you already have these PRs in your repo.
BTW, you can always try invoking the compiler with -disable-astscope-lookup as a workaround.
Thanks,
- David
… On Oct 23, 2019, at 7:18 PM, Alejandro ***@***.***> wrote:
@davidungar <https://github.com/davidungar> I recently rebased against master and it appears that part of this is failing now due to AST scope lookup.
extension<T: SomeProto> Generic<T> {}
unqualified lookup in astscope is failing to resolve SomeProto that's hitting an assertion: could not find startingScope. Any ideas on if I need to change something in astscope to fix this? Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#25263>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADQTUEPASS34AYYITYKGULQQEAX5ANCNFSM4HUAFMBA>.
|
Thanks for this, Dan! I’ll try the example.
… On Oct 23, 2019, at 7:26 PM, Dan Zheng ***@***.***> wrote:
@davidungar <https://github.com/davidungar> I recently rebased against master and it appears that part of this is failing now due to AST scope lookup.
protocol SomeProto {}
struct Generic<T> {}
extension<U: SomeProto> Generic<U> {}
unqualified lookup in astscope is failing to resolve SomeProto that's hitting an assertion: could not find startingScope. (I tried with -disable-astscope-lookup and it passes). Any ideas on if I need to change something in astscope to fix this? Thanks!
Chiming in: with the recent ASTScope changes, I think name lookup now uses source location ranges to find names in scope. We ran into the same could not find startingScope error with the @differentiable attribute, which can have a where clause: @differentiable(where T: Differentiable), similar to @_specialize attribute.
Our fix was to add ad-hoc ASTScope support for the @differentiable attribute: #27451 <#27451>. Parameterized extensions may need similar changes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#25263>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADQTUG6VWXBNL6XU3CBV2DQQEBUFANCNFSM4HUAFMBA>.
|
I see--I hadn't been aware of this work. |
(Sorry about my initial misapprehension.) I try to always be half-expecting the next bug report:) |
I hadn't considered the scoping rules for this admittedly too.
struct Generic<T> {}
extension<U> Generic<U?> // Is U visible to T? Yes it should be I hope that I'm understanding this correctly, it's my first time understanding the code around astscoping and thinking about scoping this way. These generic parameters act the same way generic parameters on a nested type act. E.g. struct Generic0<T> {
struct Generic1<U> where T == U? {
// U can see T and T can see U,
}
} |
What helps me think about scopes is to take an example and draw boxes around pieces of it, such that if A is visible from B, then B is in a box inside of A. I.e. the local variables of a function body are in a box corresponding to the curly braces, and that box is outside of a structure that the function is in. Hard to put into words, I hope it's clear. |
ca35b5f
to
0af8bdf
Compare
@@ -3743,7 +3743,7 @@ class swift::DeclDeserializer { | |||
return declOrOffset; | |||
|
|||
auto extension = ExtensionDecl::create(ctx, SourceLoc(), nullptr, { }, | |||
DC, nullptr); | |||
nullptr, DC, nullptr); |
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.
Is the plan to handle serialization and deserialization in a later PR?
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.
I think that can be handled in this PR, I just need to figure out everything else right now and then handle that.
…arameterized extensions
b56fd22
to
287808d
Compare
287808d
to
86c524a
Compare
@Azoy Sorry for the ping, I see this is still open to this day; what is the status on this PR? :) |
Refer to: Generics Manifesto.
I'm not really sure exactly what to discuss here, but right now this is a bare implementation of parameterized extensions. There might be some more that this needs, but this successfully implements all the examples from the manifesto. This also piggybacks on some of Doug's work with extending specialized typealiases, but now you can extend specialized nominal types as well.
You can also extend sugar types now like:
Right now this has basic tests, but I'd really appreciate any test cases that I should add.
cc: @slavapestov @DougGregor