Skip to content

[Diagnostics][Sema] warn inconstructible enum #16987

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

Merged
merged 4 commits into from
Jun 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -3209,6 +3209,8 @@ ERROR(unsupported_recursive_struct,none,
"contains it",
(Type))

WARNING(enum_non_well_founded,none,
"enum containing only recursive cases is impossible to instantiate", ())
ERROR(recursive_enum_not_indirect,none,
"recursive enum %0 is not marked 'indirect'", (Type))
ERROR(unsupported_infinitely_sized_type,none,
Expand Down
50 changes: 49 additions & 1 deletion lib/Sema/TypeCheckCircularity.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,8 @@ class CircularityChecker {
bool diagnoseInfiniteRecursion(CanType parentType, ValueDecl *member,
CanType memberType);

void diagnoseNonWellFoundedEnum(EnumDecl *E);

void addPathElementsTo(Path &path, CanType type);
void addPathElement(Path &path, ValueDecl *member, CanType memberType);

Expand Down Expand Up @@ -272,7 +274,11 @@ bool CircularityChecker::expandStruct(CanType type, StructDecl *S,
bool CircularityChecker::expandEnum(CanType type, EnumDecl *E,
unsigned depth) {
// Indirect enums are representational leaves.
if (E->isIndirect()) return false;
if (E->isIndirect()) {
// Diagnose whether the enum is non-well-founded before bailing
diagnoseNonWellFoundedEnum(E);
return false;
}

startExpandingType(type);

Expand All @@ -298,6 +304,7 @@ bool CircularityChecker::expandEnum(CanType type, EnumDecl *E,
if (addMember(type, elt, eltType, depth))
return true;
}
diagnoseNonWellFoundedEnum(E);

return false;
}
Expand Down Expand Up @@ -601,3 +608,44 @@ bool CircularityChecker::diagnoseInfiniteRecursion(CanType parentType,

return true;
}

/// Show a warning if all cases of the given enum are recursive,
/// making it impossible to be instantiated. Such an enum is 'non-well-founded'.
/// The outcome of this method is irrelevant.
void CircularityChecker::diagnoseNonWellFoundedEnum(EnumDecl *E) {

auto containsType = [](TupleType *tuple, Type E) -> bool {
for (auto type: tuple->getElementTypes()) {
if (type->isEqual(E))
return true;
}
return false;
};
auto isNonWellFounded = [containsType, E]() -> bool {
auto elts = E->getAllElements();
if (elts.empty())
return false;

for (auto elt: elts) {
if (!elt->hasInterfaceType() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you assert that RequireDelayedChecking got tripped if the element has no interface type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might not get tripped because I have a call to the method in the fast path before the loop in expandEnum where it is set.

!(elt->isIndirect() || E->isIndirect()))
return false;

auto argTy = elt->getArgumentInterfaceType();
Copy link
Contributor

@CodaFi CodaFi Jun 4, 2018

Choose a reason for hiding this comment

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

You can simplify this and handle nested uninhabited tuples by using TypeBase::isStructurallyUninhabited

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bit unexpected for me, can you explain why a tuple containing an uninhabited type is uninhabited? I also don't understand how this can help to simplify.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit unexpected for me, can you explain why a tuple containing an uninhabited type is uninhabited?

enum Foo {
  case bar(Never)
}

Copy link
Collaborator Author

@AnthonyLatsis AnthonyLatsis Jun 5, 2018

Choose a reason for hiding this comment

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

If I understand correctly now, uninhabited means 'not constructible'. So you can't construct a tuple that has an empty enum, because you can't instantiate an empty enum. The first thing that comes to mind when I see uninhabited is a type with no explicit members. Maybe implicit as well. Well, actually, if a type has neither explicit nor implicit members, it is inconstructible. But it's still a somewhat confusing way to put it for me.

Do I correctly understand that you want me to firstly check if the argument is uninhabited so that such cases are also counted in favor of inconstructability? The diagnostic would then be misleading the way it is now.

It can be changed then to enum is not constructible because its cases are either recursive or contain a non-constructible type.

Copy link
Collaborator Author

@AnthonyLatsis AnthonyLatsis Jun 5, 2018

Choose a reason for hiding this comment

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

@CodaFi But, I'm don't think it's a good idea to warn about 'uninhabited' cases. Similar to how we don't warn about empty enums. WDYT?

Copy link
Collaborator Author

@AnthonyLatsis AnthonyLatsis Jun 5, 2018

Choose a reason for hiding this comment

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

@CodaFi Using the terminology Slava gave, I don't think we should warn about uninhabited types (The issue faces non-well-founded types, or unconstructibly infinite types – the way @jckarter describes them in the report).

enum Foo1 { case A(Never), case B(Foo1) } // uninhabited

enum Foo2 { case A(Int, Foo2), case B(Foo2) // non-well-founded

if (!argTy)
return false;

if (auto tuple = argTy->getAs<TupleType>()) {
if (!containsType(tuple, E->getSelfInterfaceType()))
return false;
} else if (auto paren = dyn_cast<ParenType>(argTy.getPointer())) {
if (!E->getSelfInterfaceType()->isEqual(paren->getUnderlyingType()))
return false;
}
}
return true;
};

if (isNonWellFounded())
TC.diagnose(E, diag::enum_non_well_founded);
}
25 changes: 25 additions & 0 deletions test/Sema/unsupported_recursive_value_type.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,31 @@ enum RecursiveByGenericSubstitutionEnum<T> {
case A(T)
}

enum InconstructibleEnum1 { // expected-warning {{enum containing only recursive cases is impossible to instantiate}}
indirect case A(InconstructibleEnum1)
}
enum InconstructibleEnum2 { // OK
indirect case A(InconstructibleEnum2)
case B(Bool)
indirect case C(Int, InconstructibleEnum2)
}
enum InconstructibleEnum3 { // expected-warning {{enum containing only recursive cases is impossible to instantiate}}
indirect case B(Int, InconstructibleEnum3)
}
indirect enum InconstructibleEnum4 {
// expected-warning@-1 {{enum containing only recursive cases is impossible to instantiate}}
case A(InconstructibleEnum4)
}
indirect enum InconstructibleEnum5 {
// expected-warning@-1 {{enum containing only recursive cases is impossible to instantiate}}
case B(Int, InconstructibleEnum5)
}
indirect enum InconstructibleEnum6 { // OK
case A(InconstructibleEnum6)
case B(Bool)
case C(Int, InconstructibleEnum6)
}

struct RecursiveByBeingInTupleStruct {
let a: (Int, RecursiveByBeingInTupleStruct) // expected-error{{value type 'RecursiveByBeingInTupleStruct' cannot have a stored property that recursively contains it}}
// expected-note@-1 {{cycle beginning here: (Int, RecursiveByBeingInTupleStruct) -> (.1: RecursiveByBeingInTupleStruct)}}
Expand Down