[Sema] Disallow stored properties to have uninhabited types #19992

Azoy · 2018-10-23T00:14:21Z

Resolves: SR-8811

cc: @jrose-apple @CodaFi

Reword comment Add tuple test case

harlanhaskins · 2018-10-23T00:22:59Z

@swift-ci please test

jrose-apple

Thanks for picking this up!

jrose-apple · 2018-10-23T00:30:21Z

lib/Sema/TypeCheckDecl.cpp

@@ -2461,6 +2461,15 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
      }
    }

+    // Reject variable if it is a stored property with a structurally
+    // uninhabited type
+    if (VD->isInstanceMember() && VD->hasStorage() && 


I don't think this is limited to members; any stored type properties or globals or even locals would have the same problem.

jrose-apple · 2018-10-23T00:30:25Z

include/swift/AST/DiagnosticsSema.def

@@ -1367,6 +1367,10 @@ ERROR(pattern_binds_no_variables,none,
      "variables",
      (unsigned))

+ERROR(stored_property_no_uninhabited_type,none,
+      "stored property %0 cannot have uninhabited type %1",


I don't love this diagnostic because I don't think "uninhabited" is a great user-facing term, but I can't think of anything really better.

Yea, I was looking for ways to reword this, but I couldn't come up with anything.

The compiler seems to define uninhabited types as either enums with no cases, or tuples containing uninhabited types. Could we just call it a "caseless enum" or "enum with no cases"? I suppose that might require a separate phrasing for tuples:

ERROR(stored_property_no_uninhabited_enum,none, "stored property %0 cannot have caseless enum %1", ...) ERROR(stored_property_no_uninhabited_tuple,none, "stored property %0 cannot have tuple type %1 containing caseless enum", ...)

How does this sound?

// Non-structurally "%select{%select{variable|constant}0|property}1 %2 cannot have " "enum type %3 with no cases" // structurally "%select{%select{variable|constant}0|property}1 %2 cannot have " "tuple type %3 containing enum with no cases"

swift-ci · 2018-10-23T01:20:02Z

Build failed
Swift Test Linux Platform
Git Sha - 7991aec

swift-ci · 2018-10-23T01:21:40Z

Build failed
Swift Test OS X Platform
Git Sha - 7991aec

Azoy · 2018-10-23T01:45:44Z

@jrose-apple how should we handle IRGen/generic_enums.swift and Reflection/TypeLowering.swift? Can we just add a simple case to the ir test? I'm not sure the intention of that test is to test the emptiness of an enum. With the reflection test, can we just remove the empty enum?

CodaFi · 2018-10-23T03:02:34Z

The condition IRGen/generic_enums.swift is testing for should now be structurally impossible to write down (used to be that you could lower metadata for a generic member that had no cases and get a crash). You can stick a case in that Swift file and be good.

@slavapestov Can speak about the empty member in Reflection/TypeLowering.swift.

jrose-apple · 2018-10-23T16:29:29Z

I think we can still contrive an uninhabited local by using generics:

struct Wrapper<T> {
  var value: T
}

struct HasEmptyMember {
  var empty: Wrapper<Never>
}

(Slava is out right now.)

fix some tests

Azoy · 2018-10-23T23:08:37Z

@jrose-apple how does this look?

jrose-apple · 2018-10-23T23:25:31Z

lib/Sema/TypeCheckDecl.cpp

+
+      if (VD->getInterfaceType()->is<TupleType>()) {
+        uninhabitedTypeDiag = diag::pattern_no_uninhabited_tuple_type;
+      }


Can you add an assertion that the type is an enum otherwise? That way we'll know if we ever add any new ways for something to be structurally uninhabited.

Heh. It could be a BoundGenericEnumType too…

Add generic enum test case

jrose-apple · 2018-10-24T01:02:59Z

Looks pretty good. I still don't love the diagnostic text, but I can't really think of something nicer, and this is probably pretty rare anyway. Any other onlookers have any remaining thoughts?

@swift-ci Please smoke test

jrose-apple · 2018-10-24T01:03:13Z

@swift-ci Please test source compatibility

jrose-apple · 2018-10-24T16:26:19Z

@swift-ci Please test

swift-ci · 2018-10-24T16:28:25Z

Build failed
Swift Test Linux Platform
Git Sha - 7991aec

swift-ci · 2018-10-24T16:28:25Z

Build failed
Swift Test OS X Platform
Git Sha - 7991aec

Azoy · 2018-10-26T16:28:41Z

@jrose-apple is this ready for merge?

jrose-apple · 2018-10-26T17:30:50Z

I wanted to see if @CodaFi or @brentdax had any remaining thoughts, but I guess they didn't. Or they can add them post-merge.

jrose-apple · 2018-10-26T17:31:13Z

Thanks for doing this!

jckarter · 2018-10-26T17:55:13Z

Maybe this makes sense as a warning, but it does not make sense IMO to make it an error to define properties with uninhabited types. The original motivating issue in https://bugs.swift.org/browse/SR-8811 indicates an issue with the unreachable code analysis, and while this makes it impossible to get into that situation, I think we should still look into the underlying issue, and reduce this to a warning.

jrose-apple · 2018-10-26T17:55:57Z

@jckarter convinced me too in a side conversation. Mind doing a follow-up PR to downgrade it to a warning, @Azoy? (And rephrase the diagnostic a little.)

jckarter · 2018-10-26T17:57:15Z

As a warning, I think it'd also make sense to follow the behavior of the Void warning, where we warn if a variable infers its type as some uninhabited type, but allow you to make it explicit.

Azoy · 2018-10-26T18:14:54Z

@jrose-apple will do!

Azoy · 2018-10-26T22:43:29Z

@jckarter which warning are you referring to? I looked through the diagnostics to find something for Void but couldn't find anything that seemed related.

jckarter · 2018-10-26T22:52:17Z

This warning:

$ cat ~/foo.swift
let foo = print("hi")
$ xcrun swift ~/foo.swift
/Users/jgroff/foo.swift:1:5: warning: constant 'foo' inferred to have type '()', which may be unexpected
let foo = print("hi")
    ^
/Users/jgroff/foo.swift:1:5: note: add an explicit type annotation to silence this warning
let foo = print("hi")
    ^

Void is a typealias for ().

Azoy · 2018-10-26T23:12:17Z

Yea, I had just expected void to be in the diagnostic name. Would it make sense to just reuse this warning?

Disallow stored properties to have uninhabited types

7991aec

Reword comment Add tuple test case

harlanhaskins requested a review from jrose-apple October 23, 2018 00:22

jrose-apple reviewed Oct 23, 2018

View reviewed changes

Not just instance members & better diagnostic

f085c28

fix some tests

jrose-apple reviewed Oct 23, 2018

View reviewed changes

Azoy added 2 commits October 23, 2018 18:31

Assert for unknown structurally uninhabited types

83e0cfa

and generic enums aswell

2973696

Add generic enum test case

Azoy force-pushed the sr-8811 branch from 774281e to 2973696 Compare October 24, 2018 00:33

Fix non instance member tests

5dcf823

jrose-apple merged commit 4710c43 into swiftlang:master Oct 26, 2018

Azoy mentioned this pull request Nov 1, 2018

[Sema] Uninhabited stored property downgrade to warning #20211

Merged

Azoy deleted the sr-8811 branch November 6, 2018 01:29

Azoy mentioned this pull request Oct 26, 2018

[SR-8811] Diagnosing Implicit Accessors for Uninhabited Types Should Be Smarter #51319

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sema] Disallow stored properties to have uninhabited types #19992

[Sema] Disallow stored properties to have uninhabited types #19992

Azoy commented Oct 23, 2018

harlanhaskins commented Oct 23, 2018

jrose-apple left a comment

jrose-apple Oct 23, 2018

jrose-apple Oct 23, 2018

Azoy Oct 23, 2018

beccadax Oct 23, 2018

Azoy Oct 23, 2018 •

edited

Loading

swift-ci commented Oct 23, 2018

swift-ci commented Oct 23, 2018

Azoy commented Oct 23, 2018

CodaFi commented Oct 23, 2018

jrose-apple commented Oct 23, 2018 •

edited

Loading

Azoy commented Oct 23, 2018

jrose-apple Oct 23, 2018

jrose-apple Oct 24, 2018

Azoy Oct 24, 2018

jrose-apple commented Oct 24, 2018

jrose-apple commented Oct 24, 2018

jrose-apple commented Oct 24, 2018

swift-ci commented Oct 24, 2018

swift-ci commented Oct 24, 2018

Azoy commented Oct 26, 2018

jrose-apple commented Oct 26, 2018

jrose-apple commented Oct 26, 2018

jckarter commented Oct 26, 2018

jrose-apple commented Oct 26, 2018

jckarter commented Oct 26, 2018

Azoy commented Oct 26, 2018

Azoy commented Oct 26, 2018

jckarter commented Oct 26, 2018

Azoy commented Oct 26, 2018 •

edited

Loading

[Sema] Disallow stored properties to have uninhabited types #19992

[Sema] Disallow stored properties to have uninhabited types #19992

Conversation

Azoy commented Oct 23, 2018

harlanhaskins commented Oct 23, 2018

jrose-apple left a comment

Choose a reason for hiding this comment

jrose-apple Oct 23, 2018

Choose a reason for hiding this comment

jrose-apple Oct 23, 2018

Choose a reason for hiding this comment

Azoy Oct 23, 2018

Choose a reason for hiding this comment

beccadax Oct 23, 2018

Choose a reason for hiding this comment

Azoy Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

swift-ci commented Oct 23, 2018

swift-ci commented Oct 23, 2018

Azoy commented Oct 23, 2018

CodaFi commented Oct 23, 2018

jrose-apple commented Oct 23, 2018 • edited Loading

Azoy commented Oct 23, 2018

jrose-apple Oct 23, 2018

Choose a reason for hiding this comment

jrose-apple Oct 24, 2018

Choose a reason for hiding this comment

Azoy Oct 24, 2018

Choose a reason for hiding this comment

jrose-apple commented Oct 24, 2018

jrose-apple commented Oct 24, 2018

jrose-apple commented Oct 24, 2018

swift-ci commented Oct 24, 2018

swift-ci commented Oct 24, 2018

Azoy commented Oct 26, 2018

jrose-apple commented Oct 26, 2018

jrose-apple commented Oct 26, 2018

jckarter commented Oct 26, 2018

jrose-apple commented Oct 26, 2018

jckarter commented Oct 26, 2018

Azoy commented Oct 26, 2018

Azoy commented Oct 26, 2018

jckarter commented Oct 26, 2018

Azoy commented Oct 26, 2018 • edited Loading

Azoy Oct 23, 2018 •

edited

Loading

jrose-apple commented Oct 23, 2018 •

edited

Loading

Azoy commented Oct 26, 2018 •

edited

Loading