Sr 11159 space engine #26754

AndrewLitteken · 2019-08-20T22:17:49Z

Split off of #26741 to separate Enum changes and Space Engine changes, related to #26741

Changes Identifiers to DeclNames so that in checking elements the associated values are taken into account. Other exhaustiveness and redundancy to compare these associated values are taken to make sure the correct message is shown.

Resolves SR-11159.

CodaFi · 2019-08-20T23:08:44Z

Please let's just restrict this to the SpaceEngine changes. The engine should pick up DeclNames by projection, so the rest of this patch isn't needed.

CodaFi · 2019-08-21T16:54:44Z

lib/Sema/TypeCheckSwitchStmt.cpp

@@ -604,8 +612,9 @@ namespace {
            // a disjunction of its sub-spaces because nothing got reduced.
            // This is especially helpful when dealing with `unknown` case
            // in parameter positions.
-            if (s1 == reducedSpace)
+            if (s1 == reducedSpace){


I appreciate the bracing, but please standardize on the spacing in the rest of this file. If in doubt, run clang-format.

CodaFi · 2019-08-21T16:57:24Z

lib/Sema/TypeCheckSwitchStmt.cpp

@@ -696,9 +705,9 @@ namespace {
          buffer << (getBoolValue() ? "true" : "false");
          break;
        case SpaceKind::Constructor: {
-          if (!Head.empty()) {
+          if (!Head.getBaseIdentifier().empty()) {


We're going to need to print the labels into the pattern as well.

CodaFi · 2019-08-21T16:58:57Z

lib/Sema/TypeCheckSwitchStmt.cpp

@@ -1435,6 +1443,11 @@ namespace {
        if (!SP) {
          // If there's no sub-pattern then there's no further recursive
          // structure here.  Yield the constructor space.
+          if(VP->hasUnresolvedOriginalExpr()){


The space engine does not need to concern itself with ill-formed pattern trees. They will have been structurally diagnosed by the rest of pattern type checking, and we should not project them.

CodaFi · 2019-08-21T16:59:35Z

lib/Sema/TypeCheckSwitchStmt.cpp

@@ -333,7 +334,7 @@ namespace {
            return this->isSubspace(or2Space, TC, DC);
          }

-          return true;
+          return false;


I'm curious if this affects any of our tests. I suspect it may close some SRs if this turns out to be salient.

I ran the --test with the build script, the tests still passed in those cases.

Hm, let's leave it in and run the broader test suite to see if this catches anything.

Is that the validation-test?

That's part of it, yes.

CodaFi

The changes and the general approach look pretty good. Some feedback inline.

CodaFi · 2019-08-21T17:01:05Z

lib/Sema/TypeCheckSwitchStmt.cpp

@@ -115,7 +115,7 @@ namespace {

      // In type space, we reuse HEAD to help us print meaningful name, e.g.,
      // tuple element name in fixits.
-      Identifier Head;
+      DeclName Head;


This isn't your bug, but "Head" no longer makes sense under these changes. Perhaps Name or Spine (definitely rooting more for the former than the latter).

CodaFi · 2019-08-21T17:03:01Z

Let's also try to clean up the history a little here. In general, you can run

git pull --rebase upstream master

where upstream points to the Swift remote endpoint. This will keep the merge commits out of your history. An interactive rebase can also help to squash away these intermediate commits. If this is too much trouble, I can also manually squash these in after we run CI.

CodaFi · 2019-08-21T19:34:48Z

lib/Sema/TypeCheckSwitchStmt.cpp

+                other.Head.getBaseIdentifier())
+              return false;
+            if (this->Head.getArgumentNames().size() != 0 &&
+                other.Head.getArgumentNames().size() != 0) {


This sequence of checks doesn't make sense to me. To preserve the optimization, we should be checking if the baseIdentifiers are different and the argumentNames differ in number.

I may have misinterpreted some fo the tests. I think this was attempting to take care of the case where if the case statement had no arguments/associated values (i.e. .a) compared to one where it does .a(x:) then it should be considered a subspace.

Should 0 be considered special in these cases?

We pre-expand patterns via lookup to make that impossible. Otherwise we wouldn't be able to detect overlapping patterns.

Ah ok, makes sense

CodaFi · 2019-08-27T00:06:52Z

lib/Sema/TypeCheckSwitchStmt.cpp

          }

          if (Spaces.empty()) {
+            if (Head.getArgumentNames().size() > 0) {


This isn't the right place to do this. If Spaces is empty then you shouldn't have any argument names to print. The interleave call below this needs to be adjusted to print labels.

AndrewLitteken · 2019-08-27T03:21:24Z

lib/Sema/TypeCheckSwitchStmt.cpp

          }

          if (Spaces.empty()) {
            return;
          }

+          llvm::SmallVector<std::pair<Identifier, Space>, 4> labelSpaces;


Is there a more llvm-style way to do this? This seemed like the best way to attach the label to the space.

This also causes a few of the tests to fail since they produce a label that wasn't there before.

Those tests should be updated then.

jrose-apple · 2019-08-28T00:35:22Z

lib/Sema/TypeCheckSwitchStmt.cpp

+            if (this->Head.getArgumentNames().size() != 
+                other.Head.getArgumentNames().size()) {
+              return false;
+            }


Hm, why isn't this still just returning false? When would we have different labels but want to treat the constructors as the same?

This might be from when the two pull requests were together, but in the future I think there is a situation where the constructor could just have variable patterns in place of the labels, and in that case they could possibly be a sub space even if the constructors do not match exactly

Makes sense, but I'm worried that this was a load-bearing optimization. The rest of the code just checks the subspaces, which means it would treat even .x(a: Int) and .x(b: Int) as equivalent.

I see, so should these be removed and other checks be put earlier? Or just removed?

Just going back to checking the whole DeclName is probably okay for now. Unless that uncovers problems with existing tests.

should be fixed and passes the basic/validation tests from the build script

CodaFi · 2019-09-04T17:41:17Z

lib/Sema/TypeCheckSwitchStmt.cpp

+              labelSpaces.push_back(std::pair<Identifier, Space>(*args, param));
+              args++;
+            } else
+              labelSpaces.push_back(


Could you assert that you always have the same number of argument names in the base identifier as there are subspaces? If that much is true, then you can swap the interleave for the variant of std::transform that takes two input iterators and keep track of placing the commas yourself. Honestly, if it's all the same we can just take this code and you can leave a FIXME behind to come clean this up. I'm not too concerned about printing being efficient since this will only come up when you're diagnosing invalid code.

Do you mean a full assertion that stops compilation? Or just a check that runs a different path if that is the case?

Yeah, just an assert.

I am not sure just having an assertion allows all cases.

For instance, if we check the number of argument names against spaces in the following example:

enum Threepeat { case a, b, c } func test3(x: Threepeat, y: Threepeat) { switch (x, y) { // expected-error {{switch must be exhaustive}} // expected-note@-1 {{add missing case: '(.a, .c)'}} case (.a, .a): () case (.b, _): () case (.c, _): () case (_, .b): () } }

with the assertion, it fails where it should not.

AndrewLitteken · 2019-10-03T19:25:09Z

I added a FIXME to the initial implementation of showing the constructors, I'm not sure that an assertion can cover all the cases needed.

CodaFi · 2019-10-03T19:49:16Z

Looks good.

@swift-ci please test

swift-ci · 2019-10-03T20:43:01Z

Build failed
Swift Test OS X Platform
Git Sha - 84a7c1379cd57bd470038ee78ef24041f334f451

swift-ci · 2019-10-03T20:57:23Z

Build failed
Swift Test Linux Platform
Git Sha - 84a7c1379cd57bd470038ee78ef24041f334f451

adding full spaceengine changes using clang-format adding argument name size comparison adding space engine changes adding full spaceengine changes using clang-format adding argument printing to interleave function updating tests for new print formatting removing extra checks from constructor subspace checking adding alternate path if the argument and space lenght are the same clang format adding FIXME for code performance adding fixme for showing constructor switching direction of beginningparenthesis

AndrewLitteken · 2019-10-03T21:03:02Z

Apologies for that bug, the parenthesis should now be in the right direction, which I believe was the cause of the failures

theblixguy · 2019-10-03T21:14:51Z

@swift-ci please test

CodaFi · 2019-10-04T15:39:22Z

Thanks

⛵️

AndrewLitteken mentioned this pull request Aug 20, 2019

Fix SR-11159, better checking for enum elements with the same head and different labels #26741

Closed

CodaFi reviewed Aug 21, 2019

View reviewed changes

CodaFi requested changes Aug 21, 2019

View reviewed changes

CodaFi reviewed Aug 21, 2019

View reviewed changes

AndrewLitteken force-pushed the SR-11159-SpaceEngine branch 2 times, most recently from 3a63596 to 6380295 Compare August 21, 2019 19:24

CodaFi reviewed Aug 21, 2019

View reviewed changes

CodaFi reviewed Aug 27, 2019

View reviewed changes

AndrewLitteken force-pushed the SR-11159-SpaceEngine branch from 6dc15df to 9a2b3eb Compare August 27, 2019 03:19

AndrewLitteken commented Aug 27, 2019

View reviewed changes

jrose-apple reviewed Aug 28, 2019

View reviewed changes

AndrewLitteken force-pushed the SR-11159-SpaceEngine branch 2 times, most recently from 9a0c25a to 455af70 Compare August 29, 2019 22:00

CodaFi reviewed Sep 4, 2019

View reviewed changes

AndrewLitteken force-pushed the SR-11159-SpaceEngine branch from 455af70 to 84a7c13 Compare October 3, 2019 19:24

AndrewLitteken force-pushed the SR-11159-SpaceEngine branch from 84a7c13 to d7e977b Compare October 3, 2019 20:59

This comment has been minimized.

Sign in to view

AndrewLitteken requested a review from CodaFi October 4, 2019 05:18

CodaFi merged commit cb3f9e6 into swiftlang:master Oct 4, 2019

AndrewLitteken deleted the SR-11159-SpaceEngine branch January 26, 2020 04:25

Sr 11159 space engine #26754

Sr 11159 space engine #26754

Uh oh!

Conversation

AndrewLitteken commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Aug 20, 2019

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!

CodaFi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Aug 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewLitteken Aug 21, 2019 • 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!

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!

AndrewLitteken Aug 28, 2019 • 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!

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!

AndrewLitteken Sep 17, 2019 • 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!

AndrewLitteken commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Oct 3, 2019

Uh oh!

swift-ci commented Oct 3, 2019

AndrewLitteken commented Aug 20, 2019 •

edited

Loading

AndrewLitteken Aug 21, 2019 •

edited

Loading

AndrewLitteken Aug 28, 2019 •

edited

Loading

AndrewLitteken Sep 17, 2019 •

edited

Loading

AndrewLitteken commented Oct 3, 2019 •

edited

Loading