Skip to content

[Sema] Fix derived conformances crasher. #21785

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

Closed
wants to merge 1 commit into from
Closed

[Sema] Fix derived conformances crasher. #21785

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
26 changes: 26 additions & 0 deletions lib/Sema/CodeSynthesis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2581,3 +2581,29 @@ swift::createDesignatedInitOverride(TypeChecker &tc,

return ctor;
}

// SWIFT_ENABLE_TENSORFLOW
ConstructorDecl *swift::getMemberwiseInitializer(NominalTypeDecl *nominal) {
ConstructorDecl *memberwiseInitDecl = nullptr;
auto ctorDecls = nominal->lookupDirect(DeclBaseName::createConstructor());
for (auto decl : ctorDecls) {
auto ctorDecl = dyn_cast<ConstructorDecl>(decl);
if (!ctorDecl)
continue;
// Continue if:
// - Constructor is not a memberwise initializer.
// - Constructor is implicit and takes no arguments, and nominal has no
// stored properties.
// - This ad-hoc logic accepts empty struct constructors generated via
// `TypeChecker::defineDefaultConstructor`. Alternatively,
// `defaultDefaultConstructor` could be edited to mark empty
// constructors as memberwise initializers.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems cleaner then to say that the default constructor is a memberwise init in this case. Also, this could be a method on NominalTypeDecl instead of a top-level function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, both of those points make sense. I was hesitant to make those two changes (editing NominalTypeDecl and defineDefaultConstructor) and diverging from master.

What do you recommend? I can make those two changes, mark with SWIFT_ENABLE_TENSORFLOW, and upstream to master later (but without any tests).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the fact that the default init will become a memberwise init rather than have a body can be observed with a -dump-ast or -emit-silgen rest. If this helps you proceed I don’t see any problem upstreamg it now

Copy link
Contributor Author

@dan-zheng dan-zheng Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry, could you please be specific what changes you'd like to see? I'm a bit hazy so that'd be much appreciated!

Are you suggesting that the logic in getMemberwiseInitializer (introduced by this PR) be changed so that default constructors are also considered memberwise initializers? Or that defaultDefaultConstructor should mark all returned constructors as memberwise initializers?

Some observations:

  • The derived conformances logic assumes that getMemberwiseInitializer returns a constructor whose parameter count is equal to the number of stored properties in the nominal type. Marking default constructors (which take zero parameters) as memberwise initializers changes this contract and would complicate caller code.
  • There doesn't exist a flag like isMemberwiseInitializer() for default initializers. We could check other conditions like isSynthesized(), but those aren't necessarily accurate (other initializers could be synthesized).

if (!ctorDecl->isMemberwiseInitializer() &&
!(nominal->getStoredProperties().empty() && ctorDecl->isImplicit() &&
ctorDecl->getParameters()->size() == 0))
continue;
assert(!memberwiseInitDecl && "Memberwise initializer already found");
memberwiseInitDecl = ctorDecl;
}
return memberwiseInitDecl;
}
4 changes: 4 additions & 0 deletions lib/Sema/CodeSynthesis.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,10 @@ ConstructorDecl *createDesignatedInitOverride(TypeChecker &TC,
ConstructorDecl *superclassCtor,
DesignatedInitKind kind);

// SWIFT_ENABLE_TENSORFLOW
// Get memberwise initializer for a nominal type, if it exists.
ConstructorDecl *getMemberwiseInitializer(NominalTypeDecl *nominal);

} // end namespace swift

#endif
16 changes: 0 additions & 16 deletions lib/Sema/DerivedConformanceAdditiveArithmeticVectorNumeric.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,22 +76,6 @@ static ValueDecl *getProtocolRequirement(ProtocolDecl *proto, Identifier name) {
return lookup[0];
}

// Get memberwise initializer for a nominal type.
static ConstructorDecl *getMemberwiseInitializer(NominalTypeDecl *nominal) {
ConstructorDecl *memberwiseInitDecl = nullptr;
for (auto member : nominal->getMembers()) {
// Find memberwise initializer.
if (!memberwiseInitDecl) {
auto initDecl = dyn_cast<ConstructorDecl>(member);
if (!initDecl || !initDecl->isMemberwiseInitializer())
continue;
assert(!memberwiseInitDecl && "Memberwise initializer already found");
memberwiseInitDecl = initDecl;
}
}
return memberwiseInitDecl;
}

// Return the `Scalar` associated type for a ValueDecl if it conforms to
// `VectorNumeric` in the given context.
// If the decl does not conform to `VectorNumeric`, return a null `Type`.
Expand Down
29 changes: 2 additions & 27 deletions lib/Sema/DerivedConformanceDifferentiable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,29 +175,6 @@ getVectorSpaceStructDecl(NominalTypeDecl *nominal,
return vectorSpaceStructDecl;
}

// Get memberwise initializer for a nominal type.
static ConstructorDecl *getMemberwiseInitializer(NominalTypeDecl *nominal) {
ConstructorDecl *memberwiseInitDecl = nullptr;
auto ctorDecls = nominal->lookupDirect(DeclBaseName::createConstructor());
for (auto decl : ctorDecls) {
auto ctorDecl = dyn_cast<ConstructorDecl>(decl);
if (!ctorDecl)
continue;
// Continue if:
// - Constructor is not a memberwise initializer.
// - Constructor is implicit and takes no arguments, and nominal has no
// stored properties. This is ad-hoc and accepts empt struct
// constructors generated via `TypeChecker::defineDefaultConstructor`.
if (!ctorDecl->isMemberwiseInitializer() &&
!(nominal->getStoredProperties().empty() && ctorDecl->isImplicit() &&
ctorDecl->getParameters()->size() == 0))
continue;
assert(!memberwiseInitDecl && "Memberwise initializer already found");
memberwiseInitDecl = ctorDecl;
}
return memberwiseInitDecl;
}

// Synthesize body for a `Differentiable` method requirement.
static void deriveBodyDifferentiable_method(AbstractFunctionDecl *funcDecl,
Identifier methodName,
Expand Down Expand Up @@ -680,14 +657,12 @@ ValueDecl *DerivedConformance::deriveDifferentiable(ValueDecl *requirement) {
}

Type DerivedConformance::deriveDifferentiable(AssociatedTypeDecl *requirement) {
if (requirement->getBaseName() == TC.Context.Id_TangentVector) {
if (requirement->getBaseName() == TC.Context.Id_TangentVector)
return deriveDifferentiable_VectorSpace(
*this, AutoDiffAssociatedVectorSpaceKind::Tangent);
}
if (requirement->getBaseName() == TC.Context.Id_CotangentVector) {
if (requirement->getBaseName() == TC.Context.Id_CotangentVector)
return deriveDifferentiable_VectorSpace(
*this, AutoDiffAssociatedVectorSpaceKind::Cotangent);
}
TC.diagnose(requirement->getLoc(), diag::broken_differentiable_requirement);
return nullptr;
}
4 changes: 4 additions & 0 deletions test/Sema/struct_differentiable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@

struct Empty : Differentiable {}

// Test interaction with `AdditiveArithmetic` derived conformances.
// Previously, this crashed due to duplicate memberwise initializer synthesis.
struct EmptyAdditiveArithmetic : AdditiveArithmetic, Differentiable {}

struct Simple : AdditiveArithmetic, Differentiable {
var w: Float
var b: Float
Expand Down