[Diagnostics][Sema] warn inconstructible enum #16987
Conversation
| !(elt->isIndirect() || E->isIndirect())) | ||
| return false; | ||
|
|
||
| auto argTy = elt->getArgumentInterfaceType(); |
There was a problem hiding this comment.
You can simplify this and handle nested uninhabited tuples by using TypeBase::isStructurallyUninhabited
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is a bit unexpected for me, can you explain why a tuple containing an uninhabited type is uninhabited?
enum Foo {
case bar(Never)
}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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
| @@ -3209,6 +3209,8 @@ ERROR(unsupported_recursive_struct,none, | |||
| "contains it", | |||
| (Type)) | |||
|
|
|||
| WARNING(type_not_constructible,none, | |||
There was a problem hiding this comment.
Since we're only diagnosing direct self-recursion in the payload type, could you make this diagnostic more specific?
| return false; | ||
|
|
||
| for (auto elt: elts) { | ||
| if (!elt->hasInterfaceType() || |
There was a problem hiding this comment.
Could you assert that RequireDelayedChecking got tripped if the element has no interface type?
There was a problem hiding this comment.
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.
lib/Sema/TypeCheckCircularity.cpp
Outdated
| } | ||
| return false; | ||
| }; | ||
| auto isInconstructible = [containsType, E]() -> bool { |
There was a problem hiding this comment.
These are only used once so they can be pulled out as static functions.
There was a problem hiding this comment.
For the same reason, isn't it preferable to leave them in the function scope unless they are relatively long or complex? It's more readable and uniform this way.
|
@jckarter Do you want to actually disallow them or just warn? |
|
Can we standardize on terminology?
I find very few results on Google for either "unconstructible type" or "inconstructible type." Could we maybe just stick to not constructible (or, if necessary, non-constructible) throughout? |
|
@xwu I think 'not constructible' is a good idea and it sounds good, besides, IIRC diagnostic guidelines discourage these negations on long words. Thanks! |
|
The standard term for a type that does not have values is "uninhabited". A recursive type that does not have a base case is "non-well-founded". |
|
@slavapestov Thank you. Is it preferable to keep the compiler formal while using the non-formal diagnostic message? |
|
"Is not constructible" sounds a bit opaque to me too. "'T' is impossible to instantiate because it's infinitely recursive" might be more understandable? |
|
I think the part preceding
|
There was a problem hiding this comment.
FYI, @jrose-apple wrote an excellent document about diagnostics phrasing and other nuances https://github.com/apple/swift/blob/master/docs/Diagnostics.md
|
Yeah, one of the first docs I've read. It's great! |
|
Any comments? Does the message sound alright? |
|
Sounds good to me. |
|
@swift-ci Please smoke test |
|
⛵️? |
Resolves SR-6273.
Simple cases, I do not recurse into tuples or other possibly inconstructible types. Should I, or is it an overkill?