Skip to content

Fix circular dependency between SIL and AST libraries #24443

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 2, 2019
Merged

Fix circular dependency between SIL and AST libraries #24443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions include/swift/AST/Types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4828,10 +4828,11 @@ END_CAN_TYPE_WRAPPER(OpaqueTypeArchetypeType, ArchetypeType)
/// to their underlying types.
class ReplaceOpaqueTypesWithUnderlyingTypes {
public:
SILFunction *context;
ReplaceOpaqueTypesWithUnderlyingTypes(
SILFunction *context)
: context(context) {}
ModuleDecl *contextModule;
ResilienceExpansion contextExpansion;
ReplaceOpaqueTypesWithUnderlyingTypes(ModuleDecl *contextModule,
ResilienceExpansion contextExpansion)
: contextModule(contextModule), contextExpansion(contextExpansion) {}

/// TypeSubstitutionFn
Type operator()(SubstitutableType *maybeOpaqueType) const;
Expand All @@ -4844,7 +4845,8 @@ class ReplaceOpaqueTypesWithUnderlyingTypes {
bool shouldPerformSubstitution(OpaqueTypeDecl *opaque) const;

static bool shouldPerformSubstitution(OpaqueTypeDecl *opaque,
SILFunction *context);
ModuleDecl *contextModule,
ResilienceExpansion contextExpansion);
};

/// An archetype that represents the dynamic type of an opened existential.
Expand Down
44 changes: 36 additions & 8 deletions lib/AST/Type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2474,12 +2474,36 @@ getArchetypeAndRootOpaqueArchetype(Type maybeOpaqueType) {

bool ReplaceOpaqueTypesWithUnderlyingTypes::shouldPerformSubstitution(
OpaqueTypeDecl *opaque) const {
return shouldPerformSubstitution(opaque, context);
return shouldPerformSubstitution(opaque, contextModule, contextExpansion);
}

static Type substOpaqueTypesWithUnderlyingTypes(
Type ty, SILFunction *context) {
ReplaceOpaqueTypesWithUnderlyingTypes replacer(context);
bool ReplaceOpaqueTypesWithUnderlyingTypes::shouldPerformSubstitution(
OpaqueTypeDecl *opaque, ModuleDecl *contextModule,
ResilienceExpansion contextExpansion) {
auto namingDecl = opaque->getNamingDecl();

// Allow replacement of opaque result types of inlineable function regardless
// of resilience and in which context.
if (namingDecl->getAttrs().hasAttribute<InlinableAttr>()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be namingDecl->getResilienceExpansion() == ResilienceExpansion::Minimal to correctly handle functions nested inside inlinable functions, accessors of inlinable properties ,etc

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 am confused.

I thought I have to look at the resilience expansion of the context (the function in which I perform the substitution). This happens on the next lines.

The namingDecl is the decl that has the opaque result type. If the namingDecl function is inlinable and from a different resilient module it should still be okay to replace the result type.

ResilientModule.swift

struct ResilientStruct {
     @inlineable
     func namingDecl() -> some P {
         return Int64(1)
    }
}

ContextModule.swift


func context() {
    let r = ResilientStruct()
    let opaque = r.namingDecl() // Okay to specialize to Int64 here
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the rules I have currently implemented (or believe to have implemented ;-) :

  • if the function with the opaque result type is inline-able allow substitution
    (see example above)
  • if the context function in which we perform substitution has maximal resilience expansion and the context function and the opaque type's function are in the same module allow substitution
  • if the opaque types's module is not resilient allow substitution
  • otherwise disallow substitution

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the confusion. The right way to check if namingDecl is inlineable is "namingDecl->getResilienceExpansion() == ResilienceExpansion::Minimal ". You should never check for the presence of InlineableAttr directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Okay. Now I got it. Makes sense.

#24478

return true;
}
// Allow replacement of opaque result types in the context of maximal
// resilient expansion if the context's and the opaque type's module are the
// same.
auto module = namingDecl->getModuleContext();
if (contextExpansion == ResilienceExpansion::Maximal &&
module == contextModule)
return true;

// Allow general replacement from non resilient modules. Otherwise, disallow.
return !module->isResilient();
}

static Type
substOpaqueTypesWithUnderlyingTypes(Type ty, ModuleDecl *contextModule,
ResilienceExpansion contextExpansion) {
ReplaceOpaqueTypesWithUnderlyingTypes replacer(contextModule,
contextExpansion);
return ty.subst(replacer, replacer, SubstFlags::SubstituteOpaqueArchetypes);
}

Expand Down Expand Up @@ -2512,15 +2536,18 @@ operator()(SubstitutableType *maybeOpaqueType) const {

// If the type still contains opaque types, recur.
if (substTy->hasOpaqueArchetype()) {
return substOpaqueTypesWithUnderlyingTypes(substTy, context);
return substOpaqueTypesWithUnderlyingTypes(substTy, contextModule,
contextExpansion);
}
return substTy;
}

static ProtocolConformanceRef
substOpaqueTypesWithUnderlyingTypes(ProtocolConformanceRef ref, Type origType,
SILFunction *context) {
ReplaceOpaqueTypesWithUnderlyingTypes replacer(context);
ModuleDecl *contextModule,
ResilienceExpansion contextExpansion) {
ReplaceOpaqueTypesWithUnderlyingTypes replacer(contextModule,
contextExpansion);
return ref.subst(origType, replacer, replacer,
SubstFlags::SubstituteOpaqueArchetypes);
}
Expand Down Expand Up @@ -2562,7 +2589,8 @@ operator()(CanType maybeOpaqueType, Type replacementType,

// If the type still contains opaque types, recur.
if (substTy->hasOpaqueArchetype()) {
return substOpaqueTypesWithUnderlyingTypes(substRef, substTy, context);
return substOpaqueTypesWithUnderlyingTypes(substRef, substTy, contextModule,
contextExpansion);
}
return substRef;
}
Expand Down
4 changes: 3 additions & 1 deletion lib/SIL/SILFunctionType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2762,7 +2762,9 @@ static bool areABICompatibleParamsOrReturns(SILType a, SILType b,
// Opaque types are compatible with their substitution.
if (inFunction) {
auto opaqueTypesSubsituted = aa;
ReplaceOpaqueTypesWithUnderlyingTypes replacer(inFunction);
ReplaceOpaqueTypesWithUnderlyingTypes replacer(
inFunction->getModule().getSwiftModule(),
inFunction->getResilienceExpansion());
if (aa.getASTType()->hasOpaqueArchetype())
opaqueTypesSubsituted = aa.subst(inFunction->getModule(), replacer,
replacer, CanGenericSignature(), true);
Expand Down
22 changes: 0 additions & 22 deletions lib/SIL/SILType.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,25 +591,3 @@ bool SILType::isLoweringOf(SILModule &Mod, CanType formalType) {
// Other types are preserved through lowering.
return loweredType.getASTType() == formalType;
}

bool ReplaceOpaqueTypesWithUnderlyingTypes::shouldPerformSubstitution(
OpaqueTypeDecl *opaque, SILFunction *context) {
auto namingDecl = opaque->getNamingDecl();

// Allow replacement of opaque result types of inlineable function regardless
// of resilience and in which context.
if (namingDecl->getAttrs().hasAttribute<InlinableAttr>()) {
return true;
}
// Allow replacement of opaque result types in the context of maximal
// resilient expansion if the context's and the opaque type's module are the
// same.
auto expansion = context->getResilienceExpansion();
auto module = namingDecl->getModuleContext();
if (expansion == ResilienceExpansion::Maximal &&
module == context->getModule().getSwiftModule())
return true;

// Allow general replacement from non resilient modules. Otherwise, disallow.
return !module->isResilient();
}
18 changes: 13 additions & 5 deletions lib/SILOptimizer/Transforms/SpecializeOpaqueArchetypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,15 @@ using namespace swift;

static Type substOpaqueTypesWithUnderlyingTypes(
Type ty, SILFunction *context) {
ReplaceOpaqueTypesWithUnderlyingTypes replacer(context);
ReplaceOpaqueTypesWithUnderlyingTypes replacer(
context->getModule().getSwiftModule(), context->getResilienceExpansion());
return ty.subst(replacer, replacer, SubstFlags::SubstituteOpaqueArchetypes);
}

static SubstitutionMap
substOpaqueTypesWithUnderlyingTypes(SubstitutionMap map, SILFunction *context) {
ReplaceOpaqueTypesWithUnderlyingTypes replacer(context);
ReplaceOpaqueTypesWithUnderlyingTypes replacer(
context->getModule().getSwiftModule(), context->getResilienceExpansion());
return map.subst(replacer, replacer, SubstFlags::SubstituteOpaqueArchetypes);
}

Expand Down Expand Up @@ -292,7 +294,9 @@ class OpaqueSpecializerCloner
return Sty;

// Apply the opaque types substitution.
ReplaceOpaqueTypesWithUnderlyingTypes replacer(&Original);
ReplaceOpaqueTypesWithUnderlyingTypes replacer(
Original.getModule().getSwiftModule(),
Original.getResilienceExpansion());
Sty = Ty.subst(Original.getModule(), replacer, replacer,
CanGenericSignature(), true);
return Sty;
Expand All @@ -307,7 +311,9 @@ class OpaqueSpecializerCloner
ProtocolConformanceRef remapConformance(Type type,
ProtocolConformanceRef conf) {
// Apply the opaque types substitution.
ReplaceOpaqueTypesWithUnderlyingTypes replacer(&Original);
ReplaceOpaqueTypesWithUnderlyingTypes replacer(
Original.getModule().getSwiftModule(),
Original.getResilienceExpansion());
return conf.subst(type, replacer, replacer,
SubstFlags::SubstituteOpaqueArchetypes);
}
Expand Down Expand Up @@ -434,7 +440,9 @@ class OpaqueArchetypeSpecializer : public SILFunctionTransform {
if (auto opaqueTy = type->getAs<OpaqueTypeArchetypeType>()) {
auto opaque = opaqueTy->getDecl();
return ReplaceOpaqueTypesWithUnderlyingTypes::
shouldPerformSubstitution(opaque, context);
shouldPerformSubstitution(opaque,
context->getModule().getSwiftModule(),
context->getResilienceExpansion());
}
return false;
});
Expand Down