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

Implement the DynamicLookupProtocol proposal. #13076

Closed
wants to merge 2 commits into from

Conversation

@lattner
Copy link
Collaborator

commented Nov 27, 2017

This is an implementation of the DynamicLookupProtocol that I am proposing for inclusion in Swift. It has no known implementation problems, but I absolutely welcome feedback on the implementation approach.

The approach is straight-forward. It does not bloat any core data structures and is not invasive into the compiler. High points of the patch:

  • Add the new protocol to the standard library.
  • When ConstraintSystem::performMemberLookup is about to fail a value member lookup, check to see if the base type conforms to DynamicMemberLookupProtocol. If so, add the subscript(dynamicMember:) implementations to the candidate set as a new OverloadChoice::DynamicMemberLookup kind.
  • CSApply handles OverloadChoiceKind::DynamicMemberLookup by calling buildSubscript().
  • Add a test case handling many weird cases I could think of.
@lattner

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 27, 2017

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

Build failed
Swift Test Linux Platform
Git Sha - 3d03994

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

Build failed
Swift Test OS X Platform
Git Sha - 3d03994

@lattner

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 27, 2017

I'll be out of town for the next few days, but I'll take a look at the test failures and fix them when I get back.

@@ -1287,12 +1287,14 @@ namespace {
ArrayRef<Identifier> argLabels,
bool hasTrailingClosure,
ConstraintLocatorBuilder locator,
bool isImplicit, AccessSemantics semantics) {
bool isImplicit, AccessSemantics semantics,
Optional<SelectedOverload> selected = None) {

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

is this ever going to be None?

This comment has been minimized.

Copy link
@lattner

lattner Nov 29, 2017

Author Collaborator

Two of the three clients pass none here, so yep.

cs.getConstraintLocator(
locator.withPathElement(
ConstraintLocator::SubscriptMember)));
if (!selected)

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

Maybe the caller should do this?

Type resultTy;
if (!isDynamicMemberLookup) {
auto subscriptTy = simplifyType(selected->openedType);
AnyFunctionType *subscriptFnTy = subscriptTy->castTo<AnyFunctionType>();

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

It will actually always be a FunctionType. GenericFunctionType cannot appear here

This comment has been minimized.

Copy link
@lattner

lattner Dec 10, 2017

Author Collaborator

Fixed, thanks!

auto nameLabel = cs.getASTContext().getIdentifier("dynamicMember");

auto stringType = subscriptDecl->getIndices()->get(0)->getTypeLoc();
(void)cs.TC.typeCheckExpression(nameExpr, dc, stringType,

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

What if this fails?

This comment has been minimized.

Copy link
@lattner

lattner Nov 29, 2017

Author Collaborator

This can't fail, because previous filtering has validated that the index type is ExpressibleByStringLiteral. This is similar to the logic used forming string literals themselves.

@@ -6417,7 +6461,8 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
}

// Unresolved types come up in diagnostics for lvalue and inout types.
if (fromType->hasUnresolvedType() || toType->hasUnresolvedType())
if (fromType->hasUnresolvedType() || toType->hasUnresolvedType() ||
fromType->is<ErrorType>())

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

Is there a test case that triggers this? You should split it out into a separate patch with a commit message explaining why

This comment has been minimized.

Copy link
@lattner

lattner Dec 10, 2017

Author Collaborator

Removed, thanks.

@@ -7925,7 +7970,8 @@ Expr *TypeChecker::callWitness(Expr *base, DeclContext *dc,
}

Expr *
Solution::convertBooleanTypeToBuiltinI1(Expr *expr, ConstraintLocator *locator) const {
Solution::convertBooleanTypeToBuiltinI1(Expr *expr,
ConstraintLocator *locator) const {

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

It would be nicer if this and other formatting changes were in a separate patch

This comment has been minimized.

Copy link
@lattner

lattner Nov 29, 2017

Author Collaborator

Indeed. I split out the other general changes, but one off changes like this I left in. They aren't systematic enough to cause problems for review I think, but I understand what you mean.

// that contains it.
if (!conformance)
conformance =
TC.containsProtocol(ty, DMLP, DC, ConformanceCheckFlags::InExpression);

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

You should check but I think containsProtocol() is a superset of conformsToProtocol() so you only need the former here

This comment has been minimized.

Copy link
@lattner

lattner Dec 10, 2017

Author Collaborator

Looks like it! Thanks

return false;

// If we found something, make sure that it is a valid conformance.
return conformance->isAbstract() || !conformance->getConcrete()->isInvalid();

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

If ty is existential, the conformance will be abstract so you'll always return false here -- did you test the existential case?

This comment has been minimized.

Copy link
@lattner

lattner Dec 10, 2017

Author Collaborator

Yes, the tests include an existential case.

if (containsOrConformsToDLMP(instanceTy, DC, name, TC)) {
// Recursively look up the subscript(dynamicMember:)'s in this type.
auto subscriptName =
DeclName(getASTContext(), DeclBaseName::createSubscript(),

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

A few places in this method use TC.Context and here you're using getASTContext(), might want to just extract a ctx local variable

DeclContext *DC,
TypeChecker &TC) {
// Check the conforming decl.
auto match = cast<SubscriptDecl>(decl);

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

Since you're casting unconditionally it makes more sense to just change the parameter type

This comment has been minimized.

Copy link
@lattner

lattner Dec 10, 2017

Author Collaborator

Yes, good point, fixed.


// If this is the DynamicMemberLookupProtocol, manually check its
// requirement.
if (Proto->isSpecificProtocol(

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

There are many other places where we directly call ModuleDecl::lookupConformance() and they will miss this check, but I'm not sure if it matters

This comment has been minimized.

Copy link
@lattner

lattner Dec 10, 2017

Author Collaborator

I think it is fine. Whatever defines a conformance will get checked.

@slavapestov

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

I'm generally concerned with how long some of the functions like CSApply's buildSubscript() are getting. If you could split it up a bit it would be great


// Return true if the specified type conforms to the DynamicLookupMemberProtocol
// or if it is a protocol that inherits from it.
static bool containsOrConformsToDLMP(Type ty, DeclContext *DC,

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

This function name doesn't read well to me. The important part is not "contains or conforms", it's "dynamic lookup member protocol". I would shorten the former and expand the latter

This comment has been minimized.

Copy link
@lattner

lattner Dec 10, 2017

Author Collaborator

Good point, switched to isDynamicMemberLookupable

// that conforms to DynamicMemberLookupProtocol, then we resolve the reference
// to the subscript(dynamicMember:) member, and pass the member name as a
// string.
if (constraintKind == ConstraintKind::ValueMember && hasInstanceMembers &&

This comment has been minimized.

Copy link
@slavapestov

slavapestov Nov 28, 2017

Member

Maybe split out the body of this if statement into a new function, since performMemberLookup() is already quite long

This comment has been minimized.

Copy link
@lattner

lattner Dec 10, 2017

Author Collaborator

performMemberLookup is long, but the logic is straight-forward. I don't think that splitting it would make it easier to read or maintain, but it could always be done at some point independent of this patch.

@lattner

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 29, 2017

Thank you for the detailed feedback @slavapestov! When I get a chance to come back to this later this week or early next week, I'll pull in changes for your feedback into the next version of the patch. I appreciate it!

@lattner

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2017

I'm closing this, moving forward to PR 13361 which incorporates Slava's feedback and another change. Thanks!

@lattner lattner closed this Dec 10, 2017

@lattner lattner deleted the lattner:DynamicMemberLookup branch Dec 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.