Fix SR-2134: emit 'rawValue' bodies for enums with a generic raw value #4038

karwa · 2016-08-05T03:43:28Z

What's in this pull request?

Resolved bug number: (SR-2134)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

slavapestov · 2016-08-05T03:47:54Z

lib/Sema/DerivedConformanceRawRepresentable.cpp

@@ -82,9 +82,16 @@ static void deriveBodyRawRepresentable_raw(AbstractFunctionDecl *toRawDecl) {

  Type rawTy = enumDecl->getRawType();
  assert(rawTy);
+  // If the raw type is a generic parameter, unwrap it to match the type of the literals.


I'm not sure this is correct. You should not depend on SubstitutedType for anything other than diagnostics...

karwa · 2016-08-05T05:12:19Z

@slavapestov Okay, I've improved the RawRepresentable derived conformance checking. I've tried it with badly-typed cases (generic and non-generic) and it doesn't crash any more.

I haven't run the actual unit tests though

tkremenek · 2016-08-05T05:12:29Z

@swift-ci test

slavapestov · 2016-08-05T05:13:16Z

lib/Sema/DerivedConformanceRawRepresentable.cpp

@@ -357,16 +353,15 @@ ValueDecl *DerivedConformance::deriveRawRepresentable(TypeChecker &tc,
                                                      Decl *parentDecl,
                                                      NominalTypeDecl *type,
                                                      ValueDecl *requirement) {
-  // Check preconditions. These should already have been diagnosed by
-  // type-checking but we may still get here after recovery.
+  // Check preconditions for RawRepresentable conformance.


It would be great to put the precondition check in its own function, since the same logic is duplicated in the two overloads of DerivedConformance::deriveRawRepresentable()

slavapestov · 2016-08-05T05:15:02Z

@karwa Thanks! So it looks like validateDecl() checks that all the types match for us -- can you just add a comment to that effect (and also, see my note about the duplicated checks).

Other than that, this looks good, thanks!

slavapestov · 2016-08-05T05:15:59Z

@karwa One more thing, can you also add tests for the invalid cases that you fixed?

tkremenek · 2016-08-05T05:16:44Z

@swift-ci test os x

tkremenek · 2016-08-05T05:21:44Z

@swift-ci test linux

karwa · 2016-08-05T08:44:50Z

There is one test that still isn't working - decl/protocol/conforms/placement.swift

In placement_2.swift:

enum MFSynthesizedEnum1 : Int { case a }

In placement.swift:

extension MFSynthesizedEnum1 : RawRepresentable { }

The comments say something about 'suppressing' synthesized conformances, which I don't understand. What exactly is supposed to happen in this case? An error?

slavapestov · 2016-08-05T08:49:50Z

Interesting. If I create a file a.swift containing enum MFSynthesizedEnum1 : Int { case a }, and a file main.swift containing extension MFSynthesizedEnum1 : RawRepresentable { }, then swiftc a.swift main.swift crashes.

I wonder why the test passes, then.

In any case, the extension should be a no-op, or perhaps the members should be synthesized in the extension and not in the original type.

karwa · 2016-08-05T08:54:15Z

I have come across so many tests which inexplicably haven't been failing before, I can't understand it.

The mismatched enum types? That's already in test/Parse/enum.swift -- but try it in the interpreter, and it crashes.
Generic raw values? That's already in test/Generics/inheritance.swift -- but it wasn't working, hence the bug report SR-2134

slavapestov · 2016-08-05T08:59:18Z

The mismatched enum types? That's already in test/Parse/enum.swift -- but try it in the interpreter, and it crashes.

I don't see a test here that exercises that path. Maybe I'm missing something, but it could also be that the combination of a generic raw value and mismatched types crashes.

Generic raw values? That's already in test/Generics/inheritance.swift -- but it wasn't working, hence the bug report SR-2134

I think the difference between this test and yours is that you're giving the cases explicit raw values using '='. Try playing with that?

slavapestov · 2016-08-05T09:00:05Z

Thanks for digging into this regardless -- figuring out these corner cases and adding detailed tests for them is really, really important for the long-term health of the language.

karwa · 2016-08-05T09:06:47Z

Happy to contribute!

I forgot how long enum.swift is; the test is on line 316:

enum RawTypeMismatch : Int {
  case Barbur = "foo" // expected-error {{}}
}

karwa · 2016-08-05T09:07:35Z

test/Parse/enum.swift

  case Powell(Int) // expected-error {{enum with raw type cannot have cases with arguments}}
  case Terwilliger(Int) = 17 // expected-error {{enum with raw type cannot have cases with arguments}}
 }

-enum RawTypeMismatch : Int {
+enum RawTypeMismatch : Int { // expected-error {{type 'RawTypeMismatch' does not conform to protocol 'RawRepresentable'}}
  case Barbur = "foo" // expected-error {{}}
 }



Actually, it's this one.

It could be that in the interpreter, we go ahead and type check the bodies all synthesized members, but in the test, we do not.

Remember that bodies of synthesized members are built lazily, and we only crashed because we'd hit a case where a synthesized member subsequently had no body.

slavapestov · 2016-08-05T09:11:06Z

If you can think of a good way to re-organize the test, go ahead :)

Don't kill yourself trying to understand some of the emergent behavior that comes up -- it's more important to make the existing tests pass, and add new tests for the problems you found.

slavapestov · 2016-08-05T09:15:48Z

Do you mind squashing the commits into one patch, and referencing the bug number from the commit message too?

karwa · 2016-08-05T09:20:14Z

Yes, I will, once I've fixed decl/protocol/conforms/placement.swift

karwa · 2016-08-05T18:27:38Z

Okay, so it's been a day of learning :)

The problem is that if the extension is in the primary file and the enum itself in some other file, we'll try to synthesize RawRep before the enum has been type-checked, before implicit cases have been assigned values, etc. The other way around (enum in primary file) works.

I'm going to try and fix it by removing the suppression behaviour (even though it seems intentional). So declaring RawRepresentable will be a no-op, although of course you can provide your own implementation if you don't want the synthesised one. It doesn't really make sense that:

enum Foo : Int { case a }

becomes:

enum Foo : Int, RawRepresentable {
    case a
    Typealias RawValue = Int
    init?(rawValue: Int)
    var rawValue: Int { get }
    var hashValue: Int { get }
}

Meanwhile,

enum Foo : Int { case a = 10 }
extension Foo : RawRepresentable {}

becomes:

enum Foo : Int {
    case a
    var hashValue: Int { get }
}

Feels like a hack for some standard-library thing. @gribozavr, any idea?

karwa · 2016-08-07T15:44:53Z

After a little rethink, I settled on the idea that declaring a raw type (or giving a value to an element, which already requires that you explicitly specify a raw type in the inheritance clause) should count as an explicit declaration to synthesise RawRepresentable conformance.

The inheritance clause and label statements in enums are really just syntactic sugar. It doesn't change anything about the underlying storage of the enum -- and even if it did, you wouldn't be able to change that in an extension. That would be like re-parenting a class.

So that means you can't explicitly declare RawRepresentable on an enum which also has a raw type any more. Doing so would be a redundant declaration, which the compiler considers an error.

slavapestov · 2016-08-09T08:04:49Z

lib/Sema/DerivedConformanceRawRepresentable.cpp

-        !elt->getTypeCheckedRawValueExpr()->getType()->isEqual(rawTy)) {
-      return;
-    }
+    assert(elt->getTypeCheckedRawValueExpr() && "Enum element has no literal - missing a call to checkEnumRawValues()");


Lines should be shorter than 80 characters

slavapestov · 2016-08-09T08:06:55Z

This latest version looks good. Please just fix the warnings and line length and we're good to go!

slavapestov · 2016-08-09T08:07:01Z

@swift-ci Please smoke test

karwa · 2016-08-09T15:09:56Z

lib/AST/ArchetypeBuilder.cpp

+  if (!genericParams) {
+    // Failed: dependent type in non-generic context.
+    return Type();
+  }


I'm not sure if this is the right thing to do (how the function is intended to work), but the alternative is to duplicate these checks before attempting to map the type. There's an argument that this function shouldn't assert on failure conditions at all - in most other cases, the type-checker keeps going even after discovering invalid declarations.

I added these checks recently. There is a distinction between failure conditions that arise from invalid user input and failure conditions that arise from bugs inside the type checker. The latter should assert.

Note that if there are no generic parameters, we should not hit the assert, because the given type should not contain any type parameters. So if canType->hasTypeParameter() is true but genericParams is null, we have a bug.

Are you seeing this in one of the tests?

09650-swift-typebase-getcanonicaltype.swift

From the fuzzer, so it's not valid code anyway.
When we get the enum's raw type in canSynthesizeRawRep, it's a generic type param and isInvalid() is false, so I'm not sure how we should know not to try and map it to the context.

The problem then is we have an enum with a raw type that's generic, but no generic parameter list. This doesn't make sense, because any generic types that appear in declarations must come from a generic parameter list from an outer scope.

Note that even if the user's code is invalid, we might build invalid declarations, but they should still be self-consistent.

This means some other part of the compiler upstream of you constructed an invalid enum declaration, or forgot to set the generic parameter list somehow. If you figure out what's going on and fix the underlying problem, you might be able to squash a few more compiler crashers.

Or, you can just XFAIL the compiler crasher for now. Having one or two regressions isn't the end of the world, especially since you improved matters in other respects.

slavapestov · 2016-08-09T20:07:45Z

lib/Sema/DerivedConformanceRawRepresentable.cpp

@@ -259,6 +261,7 @@ static ConstructorDecl *deriveRawRepresentable_init(TypeChecker &tc,
                                       KnownProtocolKind::Equatable);
  assert(equatableProto);
  assert(tc.conformsToProtocol(rawType, equatableProto, enumDecl, None));
+  (void)equatableProto; // silence unused variable warning when assertions off.


The comment is not necessary, this is a common idiom in our code. :)

slavapestov · 2016-08-18T04:38:16Z

@karwa Are you still working on this? Let me know if you need some help!

karwa · 2016-08-18T13:08:36Z

@slavapestov yes! Been ill, will fix it up tonight

slavapestov · 2016-08-18T19:57:25Z

No worries, get well soon!

conformance Fixes SR-2134

karwa · 2016-08-22T21:10:08Z

@slavapestov Fixed, and while I was there I didn't like the look of the new errors (not very descriptive where the RawRepresentable conformance comes from), so I made them more descriptive.

slavapestov · 2016-08-22T21:13:07Z

@swift-ci Please smoke test os x

slavapestov · 2016-08-22T21:13:46Z

lib/Sema/TypeCheckProtocol.cpp

@@ -4718,6 +4730,19 @@ void TypeChecker::checkConformancesInContext(DeclContext *dc,
             dc->getDeclaredTypeInContext(),
             diag.Protocol->getName());

+	// Special case: explain that 'RawRepresentable' conformance


Indentation

slavapestov · 2016-08-22T21:14:53Z

Thanks!

…ntable enums

slavapestov · 2016-08-24T02:55:44Z

@swift-ci Please smoke test macOS and merge

slavapestov · 2016-08-24T02:56:20Z

@swift-ci Please smoke test macOS

slavapestov · 2016-08-24T04:53:05Z

Alright @karwa, I think you can close that JIRA now :)

…ynthesized` again. This reverts part of swiftlang#4038 which made the compiler consider it to be an `Explicit` conformance, breaking source code that was accepted in Swift 3.0 which declared a raw type as well as explicit conformance to `RawRepresentable` (reported as rdar://problem/30386658). While I'm here, a couple of spot fixes: - Ensure an enum's raw value exprs are type-checked before checking conformances of any of its extensions, since the RawRepresentable conformance derivation will blow up if the raw value exprs haven't been checked. Fixes an order dependency issue if `extension Foo: RawRepresentable {}` gets checked before `enum Foo: Int { ... }`. - Don't display the custom `enum_declares_rawrep_with_raw_type` diagnostic if the source location for the enum's inheritance clause is invalid, so that we don't emit a dislocated diagnostic.

slavapestov reviewed Aug 5, 2016
View reviewed changes

karwa force-pushed the generic-enums branch from 3970b11 to 6cd1397 Compare August 5, 2016 05:10

slavapestov reviewed Aug 5, 2016
View reviewed changes

slavapestov self-assigned this Aug 5, 2016

karwa force-pushed the generic-enums branch from 6cd1397 to 42cc38d Compare August 5, 2016 08:39

karwa force-pushed the generic-enums branch from 42cc38d to 89211ec Compare August 5, 2016 08:51

karwa reviewed Aug 5, 2016
View reviewed changes

karwa force-pushed the generic-enums branch from 62a79dc to d39d991 Compare August 7, 2016 15:36

karwa force-pushed the generic-enums branch from d39d991 to 8cca10c Compare August 7, 2016 16:13

karwa force-pushed the generic-enums branch from 8cca10c to 53e8c91 Compare August 7, 2016 17:10

slavapestov reviewed Aug 9, 2016
View reviewed changes

karwa reviewed Aug 9, 2016
View reviewed changes

karwa force-pushed the generic-enums branch 2 times, most recently from cc8790d to c1fb3a6 Compare August 9, 2016 18:33

slavapestov reviewed Aug 9, 2016
View reviewed changes

Tighter type-checking of enums with synthesized RawRepresentable

f9af125

conformance Fixes SR-2134

karwa force-pushed the generic-enums branch from c1fb3a6 to a1061f8 Compare August 22, 2016 20:50

slavapestov reviewed Aug 22, 2016
View reviewed changes

[Diagnostics QOI]: More descriptive errors for synthesized RawReprese…

9432a84

…ntable enums

karwa force-pushed the generic-enums branch from b6033a4 to 9432a84 Compare August 22, 2016 23:03

slavapestov merged commit 1705caa into swiftlang:master Aug 24, 2016

jckarter mentioned this pull request Feb 8, 2017

Sema: Treat the implicit RawRepresentable conformance for enums as Synthesized again. #7345

Merged

karwa mentioned this pull request Aug 5, 2016

[SR-2134] Generic RawRepresentable enum does not emit getter for 'rawValue' #44742

Closed

Fix SR-2134: emit 'rawValue' bodies for enums with a generic raw value #4038

Fix SR-2134: emit 'rawValue' bodies for enums with a generic raw value #4038

Uh oh!

Conversation

karwa commented Aug 5, 2016

What's in this pull request?

Resolved bug number: (SR-2134)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karwa commented Aug 5, 2016

Uh oh!

tkremenek commented Aug 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Aug 5, 2016

Uh oh!

slavapestov commented Aug 5, 2016

Uh oh!

tkremenek commented Aug 5, 2016

Uh oh!

tkremenek commented Aug 5, 2016

Uh oh!

karwa commented Aug 5, 2016

Uh oh!

slavapestov commented Aug 5, 2016

Uh oh!

karwa commented Aug 5, 2016

Uh oh!

slavapestov commented Aug 5, 2016

Uh oh!

slavapestov commented Aug 5, 2016

Uh oh!

karwa commented Aug 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Aug 5, 2016

Uh oh!

slavapestov commented Aug 5, 2016

Uh oh!

karwa commented Aug 5, 2016

Uh oh!

karwa commented Aug 5, 2016

Uh oh!

karwa commented Aug 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Aug 9, 2016

Uh oh!

slavapestov commented Aug 9, 2016

Uh oh!

karwa Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karwa Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Aug 18, 2016

Uh oh!

karwa commented Aug 18, 2016

Uh oh!

slavapestov commented Aug 18, 2016

Uh oh!

karwa commented Aug 22, 2016

karwa commented Aug 7, 2016 •

edited

Loading

karwa Aug 9, 2016 •

edited

Loading

karwa Aug 9, 2016 •

edited

Loading

slavapestov Aug 9, 2016 •

edited

Loading