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

Implement SE-0195, which introduces "Dynamic Member Lookup" Types #14546

Merged
merged 14 commits into from Feb 17, 2018

Conversation

Projects
None yet
6 participants
@lattner
Collaborator

lattner commented Feb 12, 2018

This is a dusted off and updated version of PR13361,
which switches from DynamicMemberLookupProtocol to @dynamicMemberLookup as
was requested by the final review decision. This also rebases it,
updates it for other changes in the compiler, improves diagnostics a bit, and adds new tests

lattner added some commits Feb 12, 2018

Implement the recently accepted SE-0195 proposal, which introduces "D…
…ynamic

Member Lookup" Types.  This is a dusted off and updated version of PR13361,
which switches from DynamicMemberLookupProtocol to @dynamicMemberLookup as
was requested by the final review decision.  This also rebases it and
updates it for other changes in the compiler.
@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 12, 2018

@swift-ci please test

@lattner lattner requested a review from rudkx Feb 12, 2018

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 12, 2018

@rudkx I'd appreciate a review whenever you have a chance, thanks!

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 12, 2018

@swift-ci please test

@swift-ci

This comment has been minimized.

Contributor

swift-ci commented Feb 12, 2018

Build failed
Swift Test OS X Platform
Git Sha - 12f3f22

@swift-ci

This comment has been minimized.

Contributor

swift-ci commented Feb 12, 2018

Build failed
Swift Test Linux Platform
Git Sha - 12f3f22

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 12, 2018

@swift-ci please test

2 similar comments
@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 12, 2018

@swift-ci please test

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 12, 2018

@swift-ci please test

@rudkx

I mostly focused on the constraint system-related changes and some potential pitfalls, but if you'd like I can take a closer look this week.

/*trailingClosure*/false,
cs.getConstraintLocator(expr),
/*isImplicit*/false,
AccessSemantics::Ordinary, selected);

This comment has been minimized.

@rudkx

rudkx Feb 12, 2018

Member

You're going to run into issues with IUOs here. In resolveOverload() you end up calling into buildDisjunctionForImplicitlyUnwrappedOptional with a locator for Members, but here buildSubscript is going to try to look up the disjunction choice for a SubscriptMember.

It doesn't look like you have tests with IUO subscript results, so it's worth adding one.

This comment has been minimized.

@lattner

lattner Feb 12, 2018

Collaborator

Yep, looks like a problem with IUO results, great catch, I'll fix. Thanks!

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

Thanks for pointing this out. IUO problem fixed and testcase added!

This comment has been minimized.

@rudkx

rudkx Feb 15, 2018

Member

Thanks!

/// particularly fast in the face of deep class hierarchies or lots of protocol
/// conformances, but this is fine because it doesn't get invoked in the normal
/// name lookup path (only when lookup is about to fail).
static bool isDynamicMemberLookupable(Type ty) {

This comment has been minimized.

@rudkx

rudkx Feb 12, 2018

Member

Can I suggest conformsToDynamicMemberLookup?

This comment has been minimized.

@benrimmington

benrimmington Feb 12, 2018

Contributor

@rudkx But can you conform to an attribute?

I suggest hasDynamicMemberLookup.

This comment has been minimized.

@lattner

lattner Feb 12, 2018

Collaborator

Thanks, renamed to hasDynamicMemberLookupAttribute

@@ -46,6 +46,8 @@ enum class OverloadChoiceKind : int {
BaseType,
/// \brief The overload choice selects a key path subscripting operation.
KeyPathApplication,
/// \brief The member is looked up through DynamicMemberLookupProtocol.
DynamicMemberLookup,

This comment has been minimized.

@rudkx

rudkx Feb 12, 2018

Member

Given that this is a separate OverloadChoiceKind, it doesn't seem like it's going to interact at all well with AnyObject lookup or key paths.

For example it seems like:

  var o: AnyObject = ...
  _ = o[dynamicMemberLookup: "property"]

will work, if the subscript is marked @objc.

What happens if someone attempts AnyObject lookup on the property name, though?

  _ = o.property

Since this feature is about non-ObjC interop, perhaps @dynamicMemberLookup attribute checking should only allow subscripts that are not marked @objc?

And what happens with key paths?

@dynamicMemberLookup
class C {
  subscript(dynamicMember member: String) -> Int { return 7 }
}

_ = \C.testLookup
_ = \C.[dynamicMember: "hi"]

This comment has been minimized.

@lattner

lattner Feb 12, 2018

Collaborator

Thank you for the review feedback!

This feature doesn't interact with AnyObject - AnyObject doesn't have the attribute on it, and it doesn't make sense for it to. Also, this proposal doesn't "take away" direct use of the subscript - there are perfectly reasonable reasons to use it directly as well as through DML.

This comment has been minimized.

@rudkx

rudkx Feb 12, 2018

Member

This feature doesn't interact with AnyObject - AnyObject doesn't have the attribute on it, and it doesn't make sense for it to.

I think some users might look at this from a different direction, especially if they know nothing about implementation details but just view AnyObject lookup as magic that can dynamically find things in classes.

@dynamicMemberLookup
@objc
class C : NSObject {
  @objc var i: Int
  @objc subscript(dynamicMember member: String) -> Int { ... magic happens ... }
}

var c = C()

c.i      // => c.i
c[dynamicMember: "x"]  // works as expected
c.x     // => c[dynamicMember: "x"]

var o: AnyObject = c
o.i      // => c.i
o[dynamicMember: "x"] // works as expected
o.x    // fails - shouldn't AnyObject-lookup have found my subscript and called it?

I'm not suggesting we make this work, but am suggesting that it could save confusion for users if we just outright ban @objc on these subscripts.

This comment has been minimized.

@lattner

lattner Feb 12, 2018

Collaborator

That restriction wasn't part of the proposal, and I don't see what adding it would achieve: this has nothing to with Objective-C, the same potential for confusion exists if someone did this:

class X {
subscript(dynamicMember: ... )
}

@dynamicMemberLookup
class Y : X {
}

Because dynamic lookups would be supported if the static type of a reference is Y but not X.

Also, is a subscript like that even @objc compatible?

This comment has been minimized.

@rudkx

rudkx Feb 12, 2018

Member

How about the other issue I mentioned, key paths?

I think users might quite reasonably expect to be able to form key paths with this dynamic lookup behavior, but there is no support here for it.

I strongly suspect we'll crash if we build a key path expression with property components for properties that don't actually exist, so it would be great to have a test or two for that.

This comment has been minimized.

@rudkx

rudkx Feb 12, 2018

Member

this has nothing to with Objective-C, the same potential for confusion exists if someone did this

That's a good point. But your example makes me wonder why we would support finding these subscripts in classes that are not either marked @dynamicmemberlookup or inheriting from a class that is.

I don't see a test case like:

@dynamicmemberlookup class Base {}
class Derived : Base { subscript(dynamicMember: ...) {} }
_ = Derived().dynamicallyLookedUp

and I don't see that mentioned in the SE proposal. Is that supposed to work?

EDIT

Also, is a subscript like that even @objc compatible?

We aren't diagnosing it, so it appears the compiler thinks so. Is there a reason to think it wouldn't be?

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

I just added a testcase for your example:
@dynamicMemberLookup class Base {}

... and the answer is "no" that is not valid. We reject it with this error message:
error: @dynamicMemberLookup attribute requires 'Base' to have a 'subscript(dynamicMember:)' member with a string index

/// name lookup path (only when lookup is about to fail).
static bool isDynamicMemberLookupable(Type ty) {
auto nominal = ty->getAnyNominal();
if (!nominal) return false; // Dynamic lookups don't exist on tuples, etc.

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

This approach also excludes existential types and archetypes that either have a class bound (where one of the classes has @dynamicMemberLookup) or a @dynamicMemberLookup protocol. The proposal isn't clear on whether these should be allowed, but I think it makes the most sense to support them.

/// @dynamicMemberLookup attribute on it. This implementation is not
/// particularly fast in the face of deep class hierarchies or lots of protocol
/// conformances, but this is fine because it doesn't get invoked in the normal
/// name lookup path (only when lookup is about to fail).

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

Remember that the constraint solver might explore a significant number of dead-end paths, so this might be called fairly often in practice. We cache all other member lookups within the constraint system to guard against such cases; I think it makes sense to cache this as well.

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

Ok, I'm happy to add that cache. Do you recommend adding this as a bit to TypeBase, or somewhere else?

This comment has been minimized.

@DougGregor

DougGregor Feb 15, 2018

Member

I think it'd be fine to make it a DenseMap in the ConstraintSystem itself. My concern is about particular constraint system going exponential (or just having a large solution space) and this to kick in along all of the failing paths.

This comment has been minimized.

@lattner

lattner Feb 16, 2018

Collaborator

SGTM, cache added. thanks!

TC.Context.getProtocol(KnownProtocolKind::ExpressibleByStringLiteral);

return indices->size() == 1 &&
!indices->get(0)->isVariadic() &&

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

Odd formatting here.

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

oooh, indeed, not sure what happened there. Fixed, thanks for noticing that!

loc, /*implicit*/true);
auto nameLabel = ctx.getIdentifier("dynamicMember");

auto stringType = subscriptDecl->getIndices()->get(0)->getTypeLoc();

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

If the subscript were generic (or in a generic type), stringType could end up being a contextual type (i.e., an archetype) within the subscript itself... which isn't what we want. We really need the interface type to be "opened" so the generic type parameters become type variables, and then simplify stringType to get the final result. Here's the kind of wacky case I have in mind:

subscript<T: ExpressibleByStringLiteral> (dynamicMember name: T) -> MemberLookup<T> { ... }

where the context of the member lookup can affect the type used for the incoming string literal.

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

Got it, thanks again for your help with this case, I really appreciate it.

// access. Emit a more specific diagnostic.
if (sub->getIndex()->isImplicit() &&
sub->getArgumentLabels().size() == 1 &&
sub->getArgumentLabels().front().str() == "dynamicMember")

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

Maybe add dynamicMember to KnownIdentifiers.def, so you don't have to hardcode the string repeatedly?

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

I can do that if you'd prefer, but is it better in practice? We'd still be hardcoding the string repeatedly, just in a different way/place.

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

I'm happy to do it, and not arguing, just seek to understand why it is better.

This comment has been minimized.

@DougGregor

DougGregor Feb 15, 2018

Member

Mostly I think it's an anti-pattern to have the same string literal repeated in many places. Even a #define would be better, but since you're always working with identifiers, KnownIdentifiers.def seems the best place.

This comment has been minimized.

@lattner

lattner Feb 16, 2018

Collaborator

k, done.

ERROR(invalid_retroactive_conformance_dmlp,none,
"retroactive conformance of %0 to 'DynamicMemberLookupProtocol' is not "
"allowed, only primary type declarations may conform", (Type))
#endif

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

Development cruft.

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

Fixed, thanks!

@@ -2551,6 +2551,13 @@ class EncodedDiagnosticMessage {
const StringRef Message;
};

/// Given a subscript defined as "subscript(dynamicMember:)->T", return true if
/// it is an acceptable implementation of the DynamicMemberLookupProtocol

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

dynamicMemberLookup attribute, now

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

👍


func testMutableExistential(a : PyVal, b : MyType) -> PyVal {
a.x.y = b
b.x.y = b

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

This is hitting the narrow case where an existential is represented as a nominal type (which is why the test succeeds now); I'd expect that, e.g., AnyObject & PyVal would fail because that's represented as a protocol composition type.

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

Fixed, thank you again for your testcase.

}
}
class DerivedClass : BaseClass {
}

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

Fun case to try: a derived class overrides subscript(dynamicMember:) to make it settable.

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

Works 👍, testcase added.

var global = 42

@dynamicMemberLookup
struct Gettable {

This comment has been minimized.

@DougGregor

DougGregor Feb 12, 2018

Member

It's important to test subscript(dynamicMember:) within generic types and probably as a generic subscript.

This comment has been minimized.

@lattner

lattner Feb 15, 2018

Collaborator

👍 Thank you for the test cases, added.

@DougGregor

This comment has been minimized.

Member

DougGregor commented Feb 12, 2018

It's getting there! My primary concerns with the implementation right now are:

  • Support for subscript(dynamicMember:) within generics and that is generic, because the handling of index types looks like it'll be incorrect.
  • Clarification on existential types (and type parameters, I guess), which I think should be supported but aren't because we're only considering nominal types.
@NachoSoto

This comment has been minimized.

Contributor

NachoSoto commented Feb 12, 2018

From the peanut gallery: given the potential for this to introduce regressions, would it make sense to make sure that the source-compatibility suite is up-and-running correctly? I counted 20 targets that are currently ignored. For example, because SR-6658 wasn't fixed on time, there was a regression on Swift 4.1 that could have been caught otherwise, and I'm worried that as time goes on, there could be more of these.

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 12, 2018

@DougGregor Thanks for the feedback. Can you give more detailed examples of the cases you're concerned about? It isn't immediately obvious to me what cases you're concerned about, but I'm happy to make sure they work (or get fixed) if I can get the (sketch of) a testcase.

@NachoSoto This isn't particularly likely to introduce regressions since it is an additive feature. It is much more likely that there will be some new thing that should be supported by it that isn't already.

@DougGregor

This comment has been minimized.

Member

DougGregor commented Feb 13, 2018

@NachoSoto, we aren't going to hold contributions hostage because of unrelated source-compatibility failures. I'm happy to engage on a discussion of SR-6658 and SR-6797 in an appropriate forum, but on a pull request for a separate feature that was approved by the Swift Evolution process, it's noise.

@DougGregor

This comment has been minimized.

Member

DougGregor commented Feb 13, 2018

@lattner , so test case sketches:

Adding a setter for the dynamicMember subscript in a derived class:

class DerivedClassWithSetter : BaseClass {
  override subscript(dynamicMember member: String) -> Int {
    get { return super[dynamicMember: member] }
    set { }
   }
}

Generics:

@dynamicMemberLookup
 struct SettableGeneric1<T> {
   subscript(dynamicMember member: StaticString) -> T? {
     get {return nil}
     set {}
   }
 }

@dynamicMemberLookup
 struct SettableGeneric2<T> {
   subscript<U: ExpressibleByStringLiteral>(dynamicMember member: U) -> T? {
     get {return nil}
     set {}
   }
 }

More existentials:

protocol SubPyVal : PyVal { }

func testMutableExistential(a : AnyObject & PyVal, b : SubPyVal)  {
 a.x.y = b
 b.x.y = b
}

More generics:

func testGeneric<T: PyVal, U: SubPyVal>(a: T, b: U) {
 a.x.y = b
 b.x.y = b
}

lattner added some commits Feb 14, 2018

Fix another problem reported by Paul Hudson, were we dropped substitu…
…tions

on a ConcreteDeclRefExpr because we were using the wrong locator.
Fix problem passing the wrong locator when dealing with IUO results,
thanks to Mark Lacey for pointing out this case.
fix a problem handling protocol compositions and protocol refinements,
add a testcase that already works for derived classes overriding the
subscript.

Thanks to Doug Gregor for both testcases!
Fix a problem where we didn't default the index type of a dynamicMemb…
…erLookup

to String because there was no LiteralConformsTo constraint.  Also fix a problem
where we'd dig the incorrect type out of a decl, applying an archetype to the
resolved string type.  Add testcases to cover some generics cases.

Many thanks to Doug Gregor for the testcases as well as helping me out with one
of the tricky parts of this patch!
@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 15, 2018

Awesome, thank you for the detailed feedback Doug! I added all the test cases and I think I addressed all of your comments above.

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 15, 2018

The two open issues is "where to cache the bit" and "do you really want this in knownIdentifiers.def"? I also owe @rudkx and answer about key paths.

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 16, 2018

I tried caching the bit in RecursiveTypeProperties which seems natural. The problem with that is that types are formed before name lookup is done. Instead I will actually listen to Doug for once ;-) and do what he says: try a map in ConstraintSystem.

Add a cache for the 'isDynamicMemberLookup' query to ConstraintSystem…
… so a type

doesn't get recomputed as the system does backtracking.

I think that all these points have been addressed, let me know if not, thank you again for the detailed review!

Implement keypaths for @dynamicMemberLookup results by desugaring them
to the same form as if the user wrote the subscript out manually.
@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 16, 2018

@swift-ci please test

3 similar comments
@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 16, 2018

@swift-ci please test

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 16, 2018

@swift-ci please test

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 16, 2018

@swift-ci please test

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 16, 2018

Keypath support is now implemented.

@rudkx and @DougGregor, I think I have addressed all of the concerns you've raised upthread. Thank you for the detailed feedback, I really appreciate your effort on this. I just kicked off a test suite check, please take another look whenever you have time and I'd like to merge it if possible. Thanks!!

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 16, 2018

@rudkx and @DougGregor, do you have any concerns with me hitting 'squash and merge'? I'm happy to continue improving this in subsequent PR's if anything new pops up. Thanks!

return false;
}
static bool hasDynamicMemberLookupAttribute(Type ty,
llvm::DenseMap<Type, bool> &IsDynamicMemberLookupCache) {

This comment has been minimized.

@DougGregor

DougGregor Feb 17, 2018

Member

I suggest making the key type of the cache a CanType to ensure that it's canonical, but I don't think it will matter much in practice.

@DougGregor

This comment has been minimized.

Member

DougGregor commented Feb 17, 2018

LGTM!

@lattner lattner merged commit a0fa5d1 into apple:master Feb 17, 2018

4 checks passed

Swift Test Linux Platform 10776 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 53985 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details
@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 17, 2018

Thank you!

@rudkx

Looks good, thanks!

@lattner

This comment has been minimized.

Collaborator

lattner commented Feb 17, 2018

Thanks @DougGregor, I'm improving the caching thing in PR 14697.

@lattner lattner deleted the lattner:dynamicMemberLookup branch Feb 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment