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

Opaque types with resilience #22072

Merged
merged 46 commits into from Apr 18, 2019

Conversation

Projects
None yet
5 participants
@jckarter
Copy link
Member

commented Jan 23, 2019

A continuation of #21137. This branch adds "proper" resilience support. Opaque types can now enter the SIL type system, and will be able to be represented at runtime with opaque accessors for their associated metadata.

}
}

auto interfaceSignature = GenericSignature::get(interfaceGenericParams,

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

It might be cleaner if you use a GSB instead; then you can add a requirement with a class or any kind of existential in it and it will flatten it into the right thing. In particular, there's no guarantee that the order in which you're adding stuff above is the right order to get a canonical generic signature

}
auto opaqueTy = OpaqueTypeArchetypeType::get(opaqueDecl, subs);
auto metatype = MetatypeType::get(opaqueTy);
opaqueDecl->setInterfaceType(metatype);

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

The code for setting the type here is the same as in deserialization. Take a look at how NominalTypeDecl and AssociatedTypeDecl have computeType() methods; OpaqueTypeDecl should also have one.

lib/Sema/TypeCheckProtocolInference.cpp Outdated
@@ -1966,8 +1966,9 @@ auto AssociatedTypeInference::solve(ConformanceChecker &checker)
if (replacement->hasDependentMember())
return None;

if (replacement->hasArchetype())
if (replacement->hasArchetype()) {

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

Useless edit here

This comment has been minimized.

Copy link
@jckarter

jckarter Jan 24, 2019

Author Member

I thought LLVM style was always to brace if blocks. It's more maintainable regardless.

getGenericSignature(interfaceSigID),
getType(interfaceTypeID)->castTo<GenericTypeParamType>());
auto genericEnv = getGenericEnvironment(genericEnvID);
opaqueDecl->setGenericEnvironment(genericEnv);

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

For later: there's a mechanism to lazily deserialize generic environments. We do this already for generic functions and types; you might want to look into doing it here


auto replacementParam = replacement->getAs<GenericTypeParamType>();
if (!replacementParam)
llvm_unreachable("opaque types cannot currently be parameterized by non-identity type parameter mappings");

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

Nitpick: lines longer than 80 characters

auto opaqueInterfaceTy = Decl->getUnderlyingInterfaceType();
auto layout = signature->getLayoutConstraint(opaqueInterfaceTy);
auto superclass = signature->getSuperclassBound(opaqueInterfaceTy);
SmallVector<ProtocolDecl*, 4> protos;

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

Can you just do protos = signature->getConformsTo(opaqueInterfaceTy)?


// Create a generic environment and bind the opaque archetype to the
// opaque interface type from the decl's signature.
auto env = signature->createGenericEnvironment();

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

Possibly for later: can you lazily create the generic environment?

lib/AST/ASTContext.cpp Outdated
nullptr);
auto env = signature->createGenericEnvironment();
env->addMapping(signature->getGenericParams()[0], result);
result->Environment = env;

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

This is actually a regression. Can you lazily create the generic environment?

Show resolved Hide resolved lib/AST/Decl.cpp Outdated
ReplaceOpaqueTypesWithUnderlyingTypes::operator()(CanType maybeOpaqueType,
Type replacementType,
ProtocolDecl *protocol) const {
auto abstractRef = ProtocolConformanceRef(protocol);

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

This is almost identical to the above

if (!anyChanged)
return *this;

// FIXME: This re-looks-up conformances instead of transforming them in

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

Perhaps subst() should handle opaque types directly and transform() should not attempt the substitution here?

return *this;
}

// Substitute the new root into the root of the interface type.

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

It feels like this should only need to happen in subst() too.

lib/SILGen/SILGenBuilder.cpp Outdated
@@ -156,6 +155,7 @@ InitExistentialValueInst *SILGenBuilder::createInitExistentialValue(

InitExistentialMetatypeInst *SILGenBuilder::createInitExistentialMetatype(
SILLocation loc, SILValue metatype, SILType existentialType,
CanType formalConcreteInstanceType,

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

Do you really need this? The instance type of a metatype is unlowered, so it should match the formal instance type.

This comment has been minimized.

Copy link
@jckarter

jckarter Jan 24, 2019

Author Member

It was necessary to support the hack of substituting out opaque types everywhere in SILGen, but I can back it out now.

return RValue(SGF, E, opaque);
}

// If the opaque type is loadable, emit the subexpression and bitcast it.

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

I wonder if opaque types should be more like existentials, with special instructions to project an address or wrap a loadable value...

This comment has been minimized.

Copy link
@jckarter

jckarter Jan 24, 2019

Author Member

We could add special instructions, but I don't know that it's worth it. The SIL passes already know how to deal with specializing bitcasts, and if we added specific instructions for underlying-to-opaque and the opposite conversion, they'd have to all be taught about those new instructions which are 99% semantically the same.

auto opaque1 = cast<OpaqueTypeArchetypeType>(desugar1);
auto opaque2 = cast<OpaqueTypeArchetypeType>(desugar2);

assert(!type2->is<LValueType>() && "Unexpected lvalue type!");

This comment has been minimized.

Copy link
@slavapestov

slavapestov Jan 24, 2019

Member

type1 and type2 will never be LValueType at this point, so you can remove the cargo cult

@jckarter jckarter force-pushed the jckarter:opaque-type-runtime branch 3 times, most recently Feb 1, 2019

@jckarter jckarter force-pushed the jckarter:opaque-type-runtime branch 2 times, most recently Feb 11, 2019

@jckarter jckarter force-pushed the jckarter:opaque-type-runtime branch 5 times, most recently to d749086 Feb 20, 2019

}

void addUnderlyingTypeAndConformances() {
auto underlyingType = Type(O->getUnderlyingInterfaceType())

This comment has been minimized.

Copy link
@slavapestov

slavapestov Mar 13, 2019

Member

Should this be a utility method on OpaqueTypeDecl?

@jckarter jckarter force-pushed the jckarter:opaque-type-runtime branch to 88a05b7 Mar 15, 2019

jckarter added some commits Mar 9, 2019

Parsable interface and type reconstruction support for opaque types.
When printing a swiftinterface, represent opaque result types using an attribute that refers to
the mangled name of the defining decl for the opaque type. To turn this back into a reference
to the right decl's implicit OpaqueTypeDecl, use type reconstruction. Since type reconstruction
doesn't normally concern itself with non-type decls, set up a lookup table in SourceFiles and
ModuleFiles to let us handle the mapping from mangled name to opaque type decl in type
reconstruction.

(Since we're invoking type reconstruction during type checking, when the module hasn't yet been
fully validated, we need to plumb a LazyResolver into the ASTBuilder in an unsightly way. Maybe
there's a better way to do this... Longer term, at least, this surface design gives space for
doing things more the right way--a more request-ified decl validator ought to be able to naturally
lazily service this request without the LazyResolver reference, and if type reconstruction in
the future learns how to reconstruct non-type decls, then the lookup tables can go away.)

@jckarter jckarter force-pushed the jckarter:opaque-type-runtime branch to dbd3a48 Apr 17, 2019

@jckarter

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@swift-ci Please test

@jckarter

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@swift-ci Please test source compatibility

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Build failed
Swift Test OS X Platform
Git Sha - 577f14fe900015a7b2878080af7bfd98bbc5344a

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Build failed
Swift Test Linux Platform
Git Sha - 577f14fe900015a7b2878080af7bfd98bbc5344a

@jckarter

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@swift-ci Please test Linux

@jckarter jckarter merged commit a8c2b50 into apple:master Apr 18, 2019

6 checks passed

Swift Source Compatibility Suite on macOS Platform (Debug)
Details
Swift Source Compatibility Suite on macOS Platform (Release)
Details
Swift Test Linux Platform No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform No test results found.
Details
Swift Test OS X Platform (smoke test)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.