[SE-0155] Implement (Most of) The Proposal #15418

CodaFi · 2018-03-22T05:12:22Z

This has sat un-reviewed for long enough. The last part of the patch is some nits in the original proposal around pattern matching.

~~In addition, the Sema and SIL plumbing for default arguments in enum cases is there, but does not work in 100% of cases. I have disabled it with a diagnostic in Parse.~~

CodaFi · 2018-03-22T05:56:32Z

Alright, it works. Could I get @slavapestov for SIL and @rudkx for Sema?

CodaFi · 2018-03-22T05:56:40Z

@swift-ci please test source compatibility

CodaFi · 2018-03-22T05:57:03Z

@swift-ci please test

CodaFi · 2018-03-22T05:58:05Z

Source Compatibility Tests Happening Here

dduan · 2018-03-23T06:09:16Z

This test case

enum a { case b(n: Int) }
a.b(n:)(3)

Triggers this assertion currently.

CodaFi · 2018-03-23T15:38:10Z

@dduan Lowering of applies with tuple shuffle sources needs to go thru the normal apply path instead of the open-coded enum case path which forms ill-typed (to the verifier) payloads. The question is how best to make that distinction in SILGenApply.

slavapestov · 2018-03-24T03:18:32Z

include/swift/Serialization/ModuleFormat.h

@@ -1078,11 +1079,13 @@ namespace decls_block {
    IdentifierIDField, // name
    DeclContextIDField,// context decl
    TypeIDField, // interface type
-    BCFixed<1>,  // has argument type?


If you keep this around, you won't need maybeReadParameterList(), you'll know if you're reading a parameter list or not

slavapestov · 2018-03-24T03:19:45Z

lib/Sema/TypeCheckPattern.cpp

+      ByVariableNameMismatch,
+    };
+
+    auto deriveStrategy = [](const TuplePatternElt &patternElt, const TupleTypeElt &typeElt) {


What is this for?

slavapestov · 2018-03-24T03:20:30Z

lib/Sema/TypeCheckStmt.cpp

@@ -207,7 +207,7 @@ static void setAutoClosureDiscriminators(DeclContext *DC, Stmt *S) {
  S->walk(ContextualizeClosures(DC));
 }

-bool TypeChecker::contextualizeInitializer(Initializer *DC, Expr *E) {


Why is this no longer an Initializer?

slavapestov · 2018-03-24T03:21:17Z

lib/Sema/TypeCheckStmt.cpp

+                                   /*respectVersionedAttr=*/true).isPublic())
+      expansion = ResilienceExpansion::Minimal;
+
+    func->setDefaultArgumentResilienceExpansion(expansion);


You'll need this for enums as well, except I guess we don't need the Swift 3 compatibility part

slavapestov · 2018-03-24T03:21:42Z

lib/Sema/TypeChecker.h

  virtual void resolveAccessControl(ValueDecl *VD) override {
-    validateAccessControl(VD);
+    validateAccessibility(VD);


We don't use the term 'accessibility' since there's potential for confusion with the UI feature

slavapestov · 2018-03-24T03:22:04Z

stdlib/public/SDK/Dispatch/Data.swift

@@ -27,7 +27,7 @@ public struct DispatchData : RandomAccessCollection, _ObjectiveCBridgeable {
 		case unmap

 		/// A custom deallocator
-		case custom(DispatchQueue?, @convention(block) () -> Void)
+		case custom(DispatchQueue?, @convention(block) @escaping () -> Void)


You'll need to not break source compatibility with this

slavapestov · 2018-03-24T03:22:26Z

lib/Parse/ParsePattern.cpp

@@ -168,6 +168,21 @@ Parser::parseParameterClause(SourceLoc &leftParenLoc,
                                          SyntaxKind::FunctionParameterList);
    }
    rightParenLoc = consumeToken(tok::r_paren);
+
+    // Per SE-0155, enum elements may not have empty parameter lists.
+    // Diagnose and insert 'Void' where appropriate.


This needs to be conditional on -swift-version 5, and you need test

slavapestov · 2018-03-24T03:23:02Z

lib/Sema/TypeCheckPattern.cpp

-                 elt.getLabel(), tupleTy->getElement(i).getName());
-        hadError = true;
-      }
+//      if (!hadError && !tupleTy->getElement(i).getName().empty() &&


What's this?

slavapestov · 2018-03-24T03:23:12Z

lib/Sema/TypeChecker.h

@@ -1327,7 +1327,7 @@ class TypeChecker final : public LazyResolver {
  void checkDefaultArguments(ArrayRef<ParameterList *> params, ValueDecl *VD);

  virtual void resolveAccessControl(ValueDecl *VD) override {
-    validateAccessibility(VD);
+    validateAccessControl(VD);


Squash this back to the other patch

slavapestov · 2018-03-24T03:23:45Z

stdlib/public/SDK/Dispatch/Data.swift

@@ -27,7 +27,7 @@ public struct DispatchData : RandomAccessCollection, _ObjectiveCBridgeable {
 		case unmap

 		/// A custom deallocator
-		case custom(DispatchQueue?, @convention(block) @escaping () -> Void)


Stack this patch below the others to avoid test churn

slavapestov · 2018-03-24T03:24:08Z

lib/Parse/ParsePattern.cpp

@@ -56,9 +56,9 @@ static DefaultArgumentKind getDefaultArgKind(Expr *init) {
  llvm_unreachable("Unhandled MagicIdentifierLiteralExpr in switch.");
 }

-void Parser::DefaultArgumentInfo::setFunctionContext(AbstractFunctionDecl *AFD){
+void Parser::DefaultArgumentInfo::setFunctionContext(DeclContext *DC, MutableArrayRef<ParameterList *> paramList){


Line too long

slavapestov · 2018-03-24T03:25:04Z

lib/Sema/CSApply.cpp

@@ -4965,7 +4965,20 @@ getCallerDefaultArg(ConstraintSystem &cs, DeclContext *dc,
                    unsigned index) {
  auto &tc = cs.getTypeChecker();

-  auto defArg = getDefaultArgumentInfo(cast<ValueDecl>(owner.getDecl()), index);
+  std::pair<DefaultArgumentKind, Type> defArg;


This looks like its duplicating code elsewhere (in SIL maybe?)

slavapestov · 2018-03-24T03:29:46Z

Nice work @CodaFi!

The question is how best to make that distinction in SILGenApply.

Which distinction?

slavapestov · 2018-03-24T03:30:51Z

stdlib/public/SDK/XCTest/XCTest.swift

-        className: exceptionResult["className"]!,
-        name: exceptionResult["name"]!,
-        reason: exceptionResult["reason"]!)
+        exceptionResult["className"]!,


Why did this change?

slavapestov · 2018-03-24T03:31:13Z

lib/Sema/TypeCheckPattern.cpp

        return true;
+      } else {
+        // FIXME: Re-enable this post-discussion on Swift Evolution.


What's this?

slavapestov · 2018-03-24T03:31:44Z

lib/AST/ASTPrinter.cpp

@@ -4233,6 +4252,13 @@ void swift::printEnumElementsAsCases(
    // If the enum element has no payloads, return.
    if (!PL)
      return;
+
+    // Print an empty-but-non-NULL parameter list as '(Void)'


Why not print it as ()?

CodaFi · 2018-03-26T19:44:24Z

@slavapestov Fixed what all I could

Which distinction?

The open-coding for EnumInst in SILGenApply is explicitly set up not to handle tuple shuffles with default arguments. All the SILGen work in this patch plumbs the emission of case-building thunks so they can be emitted as normal applies for that. In the previous PR I tried wiring this up unconditionally and it introduced some odd performance regressions in just a few cases where the optimizer should have seen through it... Regardless, I need some condition I can check so I can properly discern when to emit a normal apply and when to emit the open-coded EnumInst.

FIXME: Re-enable this post-discussion on Swift Evolution.

The proposal has a bit of ambiguous wording around how it wants the compiler to handle pattern matching in its post-implementation world. I raised the issue on the mailing list at the time and received some responses, though none from anybody on the Swift team or @rjmccall himself.

slavapestov · 2018-03-26T20:09:06Z

@CodaFi It would be best to refactor the SILGen code so that it could ultimately either emit an apply or an enum_inst (or enum_addr_inst) and handle default arguments in both. Failing that, we should always go through the thunk except in trivial cases like Optional, and try to recover the performance regression.

rudkx · 2018-04-04T21:39:09Z

I'm going to go refresh my memory on the details of the proposal and then take a look at this.

CodaFi · 2018-04-04T21:44:30Z

@rudkx I need your review on the overload resolution bits both in the constraint system and in TypeCheckPattern.

DevAndArtist · 2018-05-24T17:14:56Z

Cannot wait for this feature to finally see it's light. :)

DanielAsher · 2018-07-03T05:13:11Z

@CodaFi @rudkx Very much looking forward to this implementation for swift 5. Is time running out?

DevAndArtist · 2018-07-03T08:54:31Z

@DanielAsher Swift 5 is set to early 2019 so I think we're sill good on this feature. However I'm not sure it will make it into Swift 4.2 though.

DanielAsher · 2018-07-03T09:02:31Z

@DevAndArtist @CodaFi @rudkx pretty please bring this to Xcode 10 / swift 4.2.
There are so many use-cases for unified function/enum signatures.

IOOI-SqAR · 2018-12-21T01:47:58Z

What is the status of this? @CodaFi @DevAndArtist

CodaFi · 2018-12-21T02:37:04Z

It's being split into smaller chunks. It will not make Swift 5.0. In future, please ask questions like this on the forums.

DanielAsher · 2019-01-10T14:13:05Z

😔

DanielAsher · 2019-01-10T14:13:35Z

Best of luck with the implementation for swift 6!

CodaFi mentioned this pull request Mar 22, 2018

[SE-0155][WIP] Normalize Enum Case Representation #12397

Closed

7 tasks

CodaFi force-pushed the case-closed branch from e0665a0 to a5e3f92 Compare March 22, 2018 05:55

CodaFi requested review from slavapestov and rudkx March 22, 2018 05:56

slavapestov reviewed Mar 24, 2018

View reviewed changes

BasThomas mentioned this pull request Mar 25, 2018

[106] Issue #106 - April 5, 2018 SwiftWeekly/swiftweekly.github.io#376

Closed

CodaFi force-pushed the case-closed branch from a5e3f92 to d2e3036 Compare March 26, 2018 19:38

CodaFi mentioned this pull request Mar 28, 2018

[NFC] Serialize the resilience expansion of EnumElementDecl's default arguments #15577

Merged

CodaFi force-pushed the case-closed branch 2 times, most recently from 6c713a1 to 24e938f Compare April 2, 2018 04:32

CodaFi force-pushed the case-closed branch from 24e938f to 317cbc9 Compare May 24, 2018 18:45

CodaFi mentioned this pull request May 24, 2018

[NFC] Drop mutability from parameter context changes #16820

Merged

CodaFi force-pushed the case-closed branch 2 times, most recently from 43bf0ae to e1b31ab Compare May 24, 2018 23:11

CodaFi force-pushed the case-closed branch from e1b31ab to ba3861f Compare June 4, 2018 16:31

CodaFi force-pushed the case-closed branch 3 times, most recently from ab24db2 to 68652e2 Compare August 24, 2018 19:35

CodaFi force-pushed the case-closed branch 4 times, most recently from 3f047ec to 74236de Compare October 9, 2018 23:06

Implement SE-0155

6b30e81

CodaFi force-pushed the case-closed branch from 74236de to 6b30e81 Compare October 9, 2018 23:12

rudkx removed their request for review October 12, 2018 17:33

CodaFi closed this Dec 21, 2018

CodaFi deleted the case-closed branch December 21, 2018 02:37

[SE-0155] Implement (Most of) The Proposal #15418

[SE-0155] Implement (Most of) The Proposal #15418

Uh oh!

Conversation

CodaFi commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Mar 22, 2018

Uh oh!

CodaFi commented Mar 22, 2018

Uh oh!

CodaFi commented Mar 22, 2018

Uh oh!

CodaFi commented Mar 22, 2018

Uh oh!

dduan commented Mar 23, 2018

Uh oh!

CodaFi commented Mar 23, 2018

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

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!

slavapestov commented Mar 24, 2018

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!

CodaFi commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Mar 26, 2018

Uh oh!

rudkx commented Apr 4, 2018

Uh oh!

CodaFi commented Apr 4, 2018

Uh oh!

DevAndArtist commented May 24, 2018

Uh oh!

DanielAsher commented Jul 3, 2018

Uh oh!

DevAndArtist commented Jul 3, 2018

Uh oh!

DanielAsher commented Jul 3, 2018

Uh oh!

IOOI-SqAR commented Dec 21, 2018

Uh oh!

CodaFi commented Dec 21, 2018

Uh oh!

DanielAsher commented Jan 10, 2019

Uh oh!

DanielAsher commented Jan 10, 2019

Uh oh!

Uh oh!

CodaFi commented Mar 22, 2018 •

edited

Loading

CodaFi commented Mar 26, 2018 •

edited

Loading