[Sema]: Add Codable synthesis for enums with associated values #34855

drexin · 2020-11-21T00:28:10Z

This pull request adds Codable synthesis for enums with associated values.

drexin · 2020-11-21T00:28:36Z

There are still some things to clean up and tests to add here.

DougGregor · 2020-12-01T20:36:44Z

@swift-ci build toolchain

swift-ci · 2020-12-01T21:59:08Z

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - a34d00e81b8c29c82ba97a1e2db7b0054dfc8265

Install command
tar zxf swift-PR-34855-510-ubuntu16.04.tar.gz
More info

swift-ci · 2020-12-01T22:57:27Z

macOS Toolchain
Download Toolchain
Git Sha - a34d00e81b8c29c82ba97a1e2db7b0054dfc8265

Install command
tar -zxf swift-PR-34855-798-osx.tar.gz --directory ~/

drexin · 2021-01-22T00:20:01Z

@swift-ci test

drexin · 2021-01-22T17:31:21Z

@swift-ci build toolchain

swift-ci · 2021-01-22T18:55:50Z

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 20bc352

Install command
tar zxf swift-PR-34855-521-ubuntu16.04.tar.gz
More info

drexin · 2021-02-18T22:12:58Z

@swift-ci smoke test

drexin · 2021-02-19T00:22:17Z

@swift-ci smoke test

drexin · 2021-02-19T00:28:16Z

@swift-ci smoke test

beccadax

I don't feel like I've caught everything in this PR that could be improved, but here's a starting point.

If you want to go the extra mile, I think there are pretty strong structural similarities between enums (and their elements), associated values (and their parameters), and structs (and their stored properties) which you could exploit to make this code more principled and easier to follow. But you can always leave that refactoring for someone else.

include/swift/AST/DiagnosticsSema.def

lib/Sema/DerivedConformanceCodable.cpp

beccadax · 2021-02-19T03:40:10Z

lib/Sema/DerivedConformances.cpp

This seems kinda bad, but I guess that's why we have a feature flag.

This is copy pasted from the struct case and not specific to the additions in this PR. It just felt right to have the comment here as well.

It's behind a feature flag, because the proposal has not yet been accepted.

The original context is bogus as well. This deriving mechanism actively suppresses downstream error diagnostics in a bizarre attempt to restructure the diagnostics it emits. This code needs refactoring, but this PR is not the place to do it.

I suspect now that TypeChecker is just a namespace and we have more requests that we can perform real validation here.

lib/Sema/DerivedConformanceCodable.cpp

drexin · 2021-02-19T09:15:11Z

@swift-ci smoke test

drexin · 2021-02-23T01:05:45Z

@swift-ci smoke test

drexin · 2021-02-23T01:07:41Z

@beccadax When you have a minute, could you please give this another read?

drexin · 2021-02-24T17:58:13Z

@swift-ci smoke test

drexin · 2021-02-24T19:53:44Z

@swift-ci smoke test linux

ktoso

The synthesis is looking good, as far as I had to work with similar things recently at least :-)
Very excited to see this polished up!

beccadax

Sorry about the delay. I think there's a bunch more you could do to clean this up, but you can do that in a followup PR if you need to get this in.

beccadax · 2021-02-23T22:17:37Z

lib/Sema/DerivedConformanceCodable.cpp

Id_CodingKeys is known to start with an uppercase letter, so you can just do a normal scratch += here.

beccadax · 2021-02-26T22:12:32Z

lib/Sema/DerivedConformanceCodable.cpp

+  auto *codingKeyCase =
+      lookupEnumCase(C, codingKeysEnum, elementDecl->getBaseIdentifier());
+  if (!codingKeyCase)
+    return nullptr;


You should do this early return before you allocate inherited, and probably as early as possible to avoid unnecessary work.

beccadax · 2021-02-26T22:15:07Z

lib/Sema/DerivedConformanceCodable.cpp

+
+      // if the type conforms to {En,De}codable, add it to the enum.
+      Identifier paramIdentifier = getVarNameForCoding(paramDecl);
+      bool generatedName = false;


This variable doesn't seem to be used anywhere.

beccadax · 2021-02-26T22:17:53Z

lib/Sema/DerivedConformanceCodable.cpp

+  return dyn_cast<NominalTypeDecl>(decl);
+}
+
+static EnumDecl *addImplicitCodingKeys_enum(EnumDecl *target) {


With a couple tweaks to the whitespace, it's clear that this function "rhymes" quite a bit with addImplicitCaseCodingKeys() below:

Do you think you might be able to merge them or extract some common helpers?

Looking further down, I also see some commonalities with existing code in addImplicitCodingKeys().

beccadax · 2021-02-26T22:23:27Z

lib/Sema/DerivedConformanceCodable.cpp

-    return false;
-  }
-
+  auto *codingKeysTypeDecl = _codingKeysTypeDecl;


What's the purpose of this copy? (It looks like maybe something modifies codingKeysTypeDecl? If so, you might want to explain that.)

beccadax · 2021-02-27T00:43:31Z

lib/Sema/DerivedConformanceCodable.cpp

+        for (auto entry : llvm::enumerate(*elt->getParameterList())) {
+          auto *paramDecl = entry.value();
+          Identifier identifier = getVarNameForCoding(paramDecl);
+          if (identifier.empty()) {
+            identifier = C.getIdentifier("_" + std::to_string(entry.index()));
+          }
+          auto *caseCodingKey = lookupEnumCase(C, caseCodingKeys, identifier);
+
+          params.push_back(getVarNameForCoding(paramDecl));
+
+          // If no key is defined for this parameter, use the default value
+          if (!caseCodingKey) {
+            // This should have been verified to have a default expr in the
+            // CodingKey synthesis
+            assert(paramDecl->hasDefaultExpr());
+            decodeCalls.push_back(paramDecl->getTypeCheckedDefaultExpr());
+            continue;
+          }
+
+          // Type.self
+          auto *parameterTypeExpr = TypeExpr::createImplicit(
+              funcDC->mapTypeIntoContext(paramDecl->getInterfaceType()), C);
+          auto *parameterMetaTypeExpr =
+              new (C) DotSelfExpr(parameterTypeExpr, SourceLoc(), SourceLoc());
+          // BarCodingKeys.x
+          auto *metaTyRef =
+              TypeExpr::createImplicit(caseCodingKeys->getDeclaredType(), C);
+          auto *keyExpr =
+              new (C) MemberRefExpr(metaTyRef, SourceLoc(), caseCodingKey,
+                                    DeclNameLoc(), /*Implicit=*/true);
+
+          auto *nestedContainerExpr = new (C)
+              DeclRefExpr(ConcreteDeclRef(nestedContainerDecl), DeclNameLoc(),
+                          /*Implicit=*/true, AccessSemantics::DirectToStorage);
+          // decode(_:, forKey:)
+          auto *decodeCall = UnresolvedDotExpr::createImplicit(
+              C, nestedContainerExpr, C.Id_decode, {Identifier(), C.Id_forKey});
+
+          // nestedContainer.decode(Type.self, forKey: BarCodingKeys.x)
+          auto *callExpr = CallExpr::createImplicit(
+              C, decodeCall, {parameterMetaTypeExpr, keyExpr},
+              {Identifier(), C.Id_forKey});
+
+          // try nestedContainer.decode(Type.self, forKey: BarCodingKeys.x)
+          auto *tryExpr = new (C) TryExpr(SourceLoc(), callExpr, Type(),
+                                          /*Implicit=*/true);
+
+          decodeCalls.push_back(tryExpr);
+        }


I didn't diff, but I assume that once again this loop body is very similar to deriveBodyDecodable_init()'s.

beccadax · 2021-02-27T00:45:49Z

lib/Sema/DerivedConformanceCodable.cpp

+      llvm::SmallString<128> buffer;
+      buffer.append("Case '");
+      buffer.append(elt->getBaseIdentifier().str());
+      buffer.append(
+          "' cannot be decoded because it is not defined in CodingKeys.");


You might want to look at llvm::Twine—it's really good at building strings like this.

beccadax · 2021-02-27T00:47:51Z

lib/Sema/DerivedConformanceCodable.cpp

+    auto *debugMessage = new (C) StringLiteralExpr(
+        StringRef("Invalid number of keys found, expected one."), SourceRange(),
+        /* Implicit */ true);
+    auto *throwStmt = createThrowDecodingErrorTypeMismatchStmt(
+        C, funcDC, targetEnum, containerExpr, debugMessage);


I'm noticing that you have two helper functions to create throw statements, but you use each of them once and in both cases you need to separately build a StringLiteralExpr first. You might want to see if you can merge them together and reduce duplication by tweaking the parameters slightly.

beccadax · 2021-02-27T00:54:43Z

lib/Sema/DerivedConformanceCodable.cpp

+    // generate: case .<Case>:
+    auto pat = new (C) EnumElementPattern(
+        TypeExpr::createImplicit(enumDecl->getDeclaredType(), C), SourceLoc(),
+        DeclNameLoc(), DeclNameRef(), elt, subpattern);
+    pat->setImplicit();
+
+    auto labelItem = CaseLabelItem(pat);
+    auto body = BraceStmt::create(C, SourceLoc(), caseStatements, SourceLoc());
+    cases.push_back(CaseStmt::create(C, CaseParentKind::Switch, SourceLoc(),
+                                     labelItem, SourceLoc(), SourceLoc(), body,
+                                     /*case body vardecls*/ caseBodyVarDecls));
+  }
+
+  // generate: switch self { }
+  auto enumRef =
+      new (C) DeclRefExpr(ConcreteDeclRef(selfRef), DeclNameLoc(),
+                          /*implicit*/ true, AccessSemantics::Ordinary);
+
+  auto switchStmt = SwitchStmt::create(LabeledStmtInfo(), SourceLoc(), enumRef,
+                                       SourceLoc(), cases, SourceLoc(), C);
+  statements.push_back(switchStmt);
+
+  auto *body = BraceStmt::create(C, SourceLoc(), statements, SourceLoc(),
+                                 /*implicit=*/true);
+  return {body, /*isTypeChecked=*/false};
+}


You know, this part, and probably a good chunk of the preamble, is fairly similar to the switch statement generation in deriveBodyDecodable_enum_init(). I wonder if you could extract a helper that takes a lambda to call with each case from the enum it's switching over?

beccadax · 2021-02-27T00:56:39Z

include/swift/AST/DiagnosticsSema.def

+NOTE(codable_enum_duplicate_case_name_here,none,
+     "cannot automatically synthesize %0 because %1 has duplicate "
+     "case name %2", (Type, Type, Identifier))
+NOTE(codable_enum_duplicate_parameter_name_here,none,
+     "cannot automatically synthesize %0 for %1 because "
+     "user defined parameter name %2 in %3 conflicts with "
+     "automatically generated parameter name", (Type, Type, Identifier, Identifier))


Nit: We usually put the argument types on a separate line, partially to avoid line limit violations.

drexin · 2021-02-27T01:16:29Z

@beccadax Thanks for the feedback. Follow up PR sounds great. I‘d love to get this in as soon as possible.

drexin force-pushed the wip-enum-codable branch 2 times, most recently from 6814196 to 20bc352 Compare January 21, 2021 23:44

drexin force-pushed the wip-enum-codable branch from 20bc352 to 4269d71 Compare February 18, 2021 21:51

beccadax requested changes Feb 19, 2021

View reviewed changes

drexin requested a review from beccadax February 19, 2021 09:15

drexin added 11 commits February 23, 2021 16:00

[Sema]: Add Codable synthesis for enums with associated values

40e5fc5

Incorporate review feedback for enum Codable synthesis

cb888d2

Implement enum specific versions of existing Codable tests

3065fea

Encode parameterless enum cases as

2e8df15

Add test for overloaded case identifiers

e3105ce

Align code generation with latest proposal revision

43b7203

Put enum codable derivation behind flag

fc619b6

clang-format sources

9f84a87

Address review feedback and fix tests

84406b7

Add diagnostic for conflicting parameter identifiers

4a49724

Restructure code after rebase

677a701

drexin force-pushed the wip-enum-codable branch from 6a92c2f to 677a701 Compare February 24, 2021 17:57

ktoso approved these changes Feb 26, 2021

View reviewed changes

beccadax reviewed Feb 27, 2021

View reviewed changes

beccadax approved these changes Feb 27, 2021

View reviewed changes

drexin merged commit 8e0a260 into swiftlang:main Feb 27, 2021

[Sema]: Add Codable synthesis for enums with associated values #34855

[Sema]: Add Codable synthesis for enums with associated values #34855

Uh oh!

Conversation

drexin commented Nov 21, 2020

Uh oh!

drexin commented Nov 21, 2020

Uh oh!

DougGregor commented Dec 1, 2020

Uh oh!

swift-ci commented Dec 1, 2020

Uh oh!

swift-ci commented Dec 1, 2020

Uh oh!

drexin commented Jan 22, 2021

Uh oh!

drexin commented Jan 22, 2021

Uh oh!

swift-ci commented Jan 22, 2021

Uh oh!

drexin commented Feb 18, 2021

Uh oh!

drexin commented Feb 19, 2021

Uh oh!

drexin commented Feb 19, 2021

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

drexin commented Feb 19, 2021

Uh oh!

drexin commented Feb 23, 2021

Uh oh!

drexin commented Feb 23, 2021

Uh oh!

drexin commented Feb 24, 2021

Uh oh!

drexin commented Feb 24, 2021

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment