Skip to content
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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Conversation

Azoy
Copy link
Contributor

@Azoy Azoy commented Jun 5, 2019

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.

extension<T> Array where Element == T? {}
extension<T> Array<T?> {} // equivalent to Array where Element == T?
extension Array<String> {} // equivalent to Array where Element == String

You can also extend sugar types now like:

extension<T> [T?] {} // equivalent to Array where Element == T?
extension [String] {} // equivalent to Array where Element == String

Right now this has basic tests, but I'd really appreciate any test cases that I should add.

cc: @slavapestov @DougGregor

@Azoy Azoy force-pushed the parameterized-extensions branch from 08166ac to b210b8f Compare June 6, 2019 02:44
@Azoy Azoy marked this pull request as ready for review June 6, 2019 23:43
@slavapestov slavapestov self-assigned this Jun 7, 2019
@Azoy
Copy link
Contributor Author

Azoy commented Jun 9, 2019

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 extension<T> T and extension<T: ProtoA> T: ProtoB.

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 T.

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 T.abc() and a global abc<T>(), unless we mangled them differently somehow. I'm sure there's more to design and develop there, but just some random thoughts :)

@Azoy
Copy link
Contributor Author

Azoy commented Jun 11, 2019

Rewriting existing conditional conformance syntax to use parameterized extensions breaks ABI. There's an additional A == A1 redundant requirement for something like:

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?

@Azoy Azoy force-pushed the parameterized-extensions branch from 5c476be to f11acf1 Compare June 12, 2019 01:50
Copy link
Contributor

@slavapestov slavapestov left a 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.

@Azoy
Copy link
Contributor Author

Azoy commented Jun 13, 2019

@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:

Failed to reconstruct type for $s10extensions7GenericVAAqd__SgRszlE7Nested3VyAD_GD
Original type:
(struct_type decl=extensions.(file).Generic extension.Nested3@swift/test/TypeDecoder/extensions.swift:50:10
  (parent=bound_generic_struct_type decl=extensions.(file).Generic@swift/test/TypeDecoder/extensions.swift:17:8
    (bound_generic_enum_type decl=Swift.(file).Optional
      (generic_type_param_type depth=1 index=0))))

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:

(extension in extensions):extensions.Generic<Swift.Optional<A1>><A where A == Swift.Optional<A1>>.Nested3

which for the most part looks correct, except the <Swift.Optional<A1>> part I'm a little suspect on because it should just be a plain <A1> no? Any help is greatly appreciated!

@Azoy Azoy force-pushed the parameterized-extensions branch 2 times, most recently from d71600b to 546430f Compare June 14, 2019 23:47
@Azoy
Copy link
Contributor Author

Azoy commented Jun 16, 2019

@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.

@Azoy
Copy link
Contributor Author

Azoy commented Jun 17, 2019

This is going to require a little bit more work.

struct Generic<T> {}

extension<U> Generic<U?> {
  struct Nested {
    let x: U
  }
}

TypeBase::getTypeOfMember treats x as having the following substitution map:

T -> T
U -> Optional<U>
// rather than
T -> Optional<U>
U -> U

This is due to the fact that U doesn't appear as a generic param in Generic<Optional<U>>.Nested. We could treat this nested type as Generic<Optional<U>>.Nested<U> to achieve the desired result, but I don't know if that's the correct fix.

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.

@slavapestov
Copy link
Contributor

@DougGregor worked on the mangling for extension members most recently. Doug, can you offer some guidance here?

include/swift/AST/Decl.h Outdated Show resolved Hide resolved
@Azoy
Copy link
Contributor Author

Azoy commented Sep 13, 2019

@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 MetadataLookup and Demangle in the runtime, but I still can't seem to figure it out.

@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, e, but was unsure what to do about ObjC remangling because unconstrained extensions already used that mangling. Right now this just mangles the complete generic signature of the extension.

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!

@jckarter
Copy link
Contributor

The mangling of generic parameters currently relies on the extension having the same generic parameters as the extended type. If you have something like:

struct Foo<T> { struct Bar<U> { } }

extension Foo.Bar { }

then inside the extension context, T is mangled as depth=0 index=0 and U is mangled as depth=1 index=0. With a parameterized extension, you'd want to mangle generic parameters by following the signature declared on the extension, so if you had:

extension <A, B> Foo<B>.Bar<A> { ... }

then A would be depth=0 index=0 and B would be depth=0 index=1. I don't think we can accommodate this without new manglings and new runtime code.

@Azoy
Copy link
Contributor Author

Azoy commented Sep 14, 2019

then A would be depth=0 index=0 and B would be depth=0 index=1. I don't think we can accommodate this without new manglings and new runtime code.

These would be mangled as depth 2 no? Because inside the extension context we would still want to support T and U. E.g. <T><U><A, B> where T == A, U == B

@jckarter
Copy link
Contributor

You could model it that way, I suppose, but that seems like it adds additional complexity.

@Azoy
Copy link
Contributor Author

Azoy commented Oct 24, 2019

@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!

@dan-zheng
Copy link
Contributor

dan-zheng commented Oct 24, 2019

@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. Parameterized extensions may need similar ASTScope support.

@Azoy
Copy link
Contributor Author

Azoy commented Oct 24, 2019

@dan-zheng thanks so much, will look into this!

@davidungar
Copy link
Contributor

davidungar commented Oct 24, 2019 via email

@davidungar
Copy link
Contributor

davidungar commented Oct 24, 2019 via email

@davidungar
Copy link
Contributor

I see--I hadn't been aware of this work.
You are trying to get ASTScopes working with a new feature!
I haven't thought about it much. How do you think the scoping ought to work?
Can there be multiple parameters? Is one visible to the previous one?
Can old-style generic parameters after that main parameter see the new parameters?
Happy to discuss it more, probably most productive for me to have a good, concrete description of what the scoping rules should be for this feature.

@davidungar
Copy link
Contributor

(Sorry about my initial misapprehension.) I try to always be half-expecting the next bug report:)

@Azoy
Copy link
Contributor Author

Azoy commented Oct 24, 2019

I hadn't considered the scoping rules for this admittedly too.

  1. Can there be multiple parameters? Yes
  2. Is one visible to the previous one? If you mean extension<T, U, V> where T can see U and V, then yes I believe they should be visible to eachother?
  3. Can old-style generic parameters after that main parameter see the new parameters?
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, 
  }
}

@davidungar
Copy link
Contributor

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.
Interestingly a few Swift features are "unboxable". For example a guard statement with a let creates a variable that is in-scope after the guard body, but not inside of it. So a new, "artificial" scope is required for after-a-guard in that case. There is even at least one case where the box topology required for the source-coordinate search cannot be made consistent with the topology required for Swift lookup rules.

@@ -3743,7 +3743,7 @@ class swift::DeclDeserializer {
return declOrOffset;

auto extension = ExtensionDecl::create(ctx, SourceLoc(), nullptr, { },
DC, nullptr);
nullptr, DC, nullptr);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@Azoy Azoy marked this pull request as draft August 11, 2020 02:45
@Azoy Azoy changed the title [WIP] Parameterized extensions Parameterized extensions Aug 11, 2020
@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@Azoy Azoy changed the base branch from master to main October 1, 2020 06:59
@Azoy Azoy force-pushed the parameterized-extensions branch 2 times, most recently from b56fd22 to 287808d Compare October 6, 2020 19:24
@MrSkwiggs
Copy link

@Azoy Sorry for the ping, I see this is still open to this day; what is the status on this PR? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants