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

AndrewLitteken · 2019-08-20T16:36:42Z

This is a fix to SR-11159 which was a bug where having two enum elements with different named arguments could cause the compiler to crash in a switch statement. I do my best to follow the following swift evolution proposal . There are still a few errors surrounding error messages that I would help from if possible.

Resolves SR-11159.

AndrewLitteken · 2019-08-20T17:24:11Z

@swift-ci Please smoke test

theblixguy

I only had a very quick look, but here are some initial comments.

(btw, welcome to the project! 🎉)

theblixguy · 2019-08-20T17:46:25Z

include/swift/AST/DiagnosticsSema.def

+ERROR(enum_element_pattern_assoc_values_using_labels,none,
+      "using label to match to enum, enum element pattern cannot match against mixed labels and variable patterns.",())
+ERROR(enum_element_pattern_assoc_values_using_var_patterns,none,
+      "using variable patterns to match against enum, enum element pattern cannot match against mixed labels and variable patterns.",())


Hmm, what's a "variable pattern"?

In the proposal when doing an case statement you can't mix labels and variables. So in (a: _, let y) the variable patterns is let y. There's also something added in the Identifier to flag that it came from a variable pattern. I don't think that this method is the best position though.

Still, I don't think we call them "variable patterns" in user-facing diagnostics.

theblixguy · 2019-08-20T17:47:06Z

lib/AST/NameLookup.cpp

@@ -1442,12 +1443,14 @@ bool DeclContext::lookupQualified(Type type,
  assert(decls.empty() && "additive lookup not supported");

  // Handle AnyObject lookup.
-  if (type->isAnyObject())
+  if (type->isAnyObject()){


Could you remove these changes? There are a lot of them and are unrelated to your fix and makes the PR really long.

I would also suggest running git-clang-format to format your changes.

lib/Sema/TypeCheckNameLookup.cpp

lib/Sema/TypeCheckPattern.cpp

theblixguy · 2019-08-20T18:11:37Z

lib/Sema/TypeCheckPattern.cpp

+        continue;
+      }
+      bool addCandidate = true;
+      if(queryParams.size() == 1){


This condition would always be true, right? Since we continue above if the size is not equal to 1.

It isn't always true because it will continue only if the sizes are not equal and the size is not equal to 1. If both the case and the enum element are the same it should not continue. It should be ((enumElementParams.size() != queryParams.size()) && (queryParams.size() != 1)) for clarity.

lib/Sema/TypeCheckPattern.cpp

theblixguy · 2019-08-20T18:18:40Z

lib/Sema/TypeCheckPattern.cpp

+      bool enumHasLabels = false;
+      for(size_t i = 0;i<enumElementParams.size();++i)
+        enumHasLabels = enumHasLabels || !enumElementParams[i].empty();
+      if(foundElements.size() == 1 && !enumHasLabels){


Hmm, if you only care about a single result, you can do foundElements .getSingleTypeResult() (will return nullptr if size() != 1). Perhaps you can lift this check outside the for loop in that case?

CodaFi · 2019-08-20T21:41:16Z

I think this patch is trying to do a few separate things. I'd like to review the DeclName patch to the space engine in a separate PR, if you could split it off.

AndrewLitteken · 2019-08-20T21:55:07Z

The SpaceEngine changes are to take care of exhaustivness/redundancy errors that pop up when the changes is added to the Enum typechecking. Should I try to resolve those issues just within the Enum checking?

CodaFi · 2019-08-20T22:03:46Z

Yes. That change is easiest to take at this stage.

AndrewLitteken · 2019-08-20T22:31:15Z

SpaceEngine patch is now in #26754

AndrewLitteken · 2019-10-10T16:13:20Z

I've made a few changes to this after my last pull request to try to clean this up a bit and minimize changes. It currently passes both the standard and validation tests.

lib/Sema/TypeCheckPattern.cpp

lib/Sema/TypeCheckSwitchStmt.cpp

CodaFi · 2019-10-17T22:06:30Z

The general approach looks good to me so far. I'm going to spend a little more time staring at the matching algorithm (it was the hardest part of this when I came along and tried to do it a while ago). In the mean time, could you please write a bunch of tests!

CodaFi · 2019-10-21T20:24:39Z

Here's a good one to add as a test https://bugs.swift.org/browse/SR-11159

AndrewLitteken · 2019-10-29T18:16:46Z

Added a few tests. I haven't added anything concerning warnings and exhaustiveness. There are some issues checking for this when the labels are created from the variable declaration in the constructor. It seems like that could be broken into its own issue.

AndrewLitteken · 2020-01-24T15:59:59Z

I was wondering what the current status of this pull request is?

CodaFi · 2020-01-24T23:35:06Z

Give it a rebase and let’s see about getting it merged.

… value, adding fix for if enum element has no var/let pattern but has associated values adding additional file removing print statement addition to identifier to identify from let or var, used to match against enum cases to avoid ambiguities removing optimization for exhaustiveness checking to work properly adding new functionality, removing debug statements removing stray iostream includes, enforcing 80 column width formatting corrections Update TypeCheckSwitchStmt.cpp committing column widths for TypeCheckPattern.cpp removing some sublime place brackets and extra lines Update TypeCheckStmt.cpp adding range based for loop strategies removing SpaceEngine changes to be put in different pull request fixing DeclName and Identifier problems fixing error where a label with no let/var pattern would not show error with wrong label, and so finding enum element if only one member with same head found Update TypeCheckStmt.cpp cleaning up code fixing full enum description being included and correct warnings clang format changes cleaning up code, Optional Bools, and removing unused code fix small bug, add fixmes and tests correcting changes from rebase clang format fixing tests

AndrewLitteken · 2020-01-31T04:27:28Z

Rebase should be complete, and passes the basic build script tests

theblixguy · 2020-01-31T08:01:12Z

@swift-ci please test

swift-ci · 2020-01-31T08:22:07Z

Build failed
Swift Test OS X Platform
Git Sha - 5415e94

swift-ci · 2020-01-31T09:41:00Z

Build failed
Swift Test Linux Platform
Git Sha - 5415e94

theblixguy · 2020-01-31T11:18:14Z

Hmm, maybe something broke:

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/stdlib/public/core/StringObject.swift:95:16: error: cannot convert value of type 'StringObject.Variant' to expected argument type 'Optional<>'
08:18:33 if case .immortal = self { return true }

CodaFi · 2020-01-31T16:13:59Z

test/stmt/switch_stmt2.swift

+
+// Do not crash
+enum Foo1 
+{


Nit: Please match stdlib style in here.

CodaFi · 2020-01-31T16:14:20Z

lib/Sema/TypeCheckSwitchStmt.cpp

@@ -557,6 +559,8 @@ namespace {
        PAIRCASE (SpaceKind::Constructor, SpaceKind::Constructor): {
          // Optimization: If the heads of the constructors don't match then
          // the two are disjoint and their difference is the first space.
+
+          // FIXME: Exhuastiveness checking for labelled Enums


Hold on: is this still not implemented properly?

This has to do with extraneous warnings when there are labelled enums with the same base name. There wasn't a good solution developed in the last pull request, and it was sort of out of the scope of the initial bug report, I thought it could be fixed in a different pull request.

Ah, OK. That's usually called "redundancy checking". Had a small heart attack at the thought we had regressed the exhaustiveness checker.

CodaFi · 2020-01-31T16:14:56Z

lib/Sema/TypeCheckPattern.cpp

+            .diagnose(EEP->getLoc(),
+                      diag::enum_element_pattern_assoc_values_remove)
+            .fixItRemove(sub->getSourceRange());
+        ;


Nit: stray semicolon

CodaFi · 2020-01-31T16:18:10Z

lib/Sema/TypeCheckPattern.cpp

@@ -1286,22 +1432,20 @@ Pattern *TypeChecker::coercePatternToType(ContextualPattern pattern,
          // isn't a static VarDecl, so the existing mechanics in
          // extractEnumElement won't work.
          if (type->getAnyNominal() == Context.getOptionalDecl()) {
-            if (EEP->getName().isSimpleName("None") ||
-                EEP->getName().isSimpleName("Some")) {
+            if (EEP->getName().getBaseIdentifier().str() == "None" ||


You shouldn't have to touch this code. The guard above checks if the nominal type we're dealing with is precisely Swift.Optional<T>

If anything we oughta just get rid of it. These are Swift 2-3 migration guides.

CodaFi · 2020-01-31T16:19:51Z

lib/Sema/TypeCheckPattern.cpp

+        return new (Context) EnumElementPattern(
+            ume->getDotLoc(), ume->getNameLoc(),
+            DeclNameRef(
+                DeclName(Context, ume->getName().getBaseName(), namedParams)),


Nit: DeclName::withArgumentLabels is a touch cleaner here.

CodaFi · 2020-01-31T16:21:12Z

include/swift/AST/Pattern.h

-      ParentType(ParentType), DotLoc(DotLoc), NameLoc(NameLoc), Name(Name),
-      ElementDeclOrUnresolvedOriginalExpr(Element),
-      SubPattern(SubPattern) {
+      : Pattern(PatternKind::EnumElement), ParentType(ParentType),


Nothing seems to have changed in this file except whitespace. Can you revert this out?

CodaFi · 2020-01-31T16:24:24Z

lib/Sema/TypeCheckPattern.cpp

+          params.push_back(Identifier());
+        else
+            // Set None if not using labels, throw error if contradicts
+            if (usingLabels == None)


This if-else tree and the one below it absolutely positively need explicit braces. I can't track anything here.

CodaFi · 2020-01-31T16:26:58Z

include/swift/AST/DiagnosticsSema.def

@@ -4731,6 +4731,10 @@ ERROR(empty_switch_stmt,none,
      "'switch' statement body must have at least one 'case' or 'default' "
      "block; do you want to add a default case?",())
 ERROR(non_exhaustive_switch,none, "switch must be exhaustive", ())
+ERROR(enum_element_pattern_assoc_values_using_labels,none,
+      "using label to match to enum, enum element pattern cannot match against mixed labels and variable patterns.",())


How about:

pattern matching case '%0' must specify all labels if it specifies any labels

and a nice note that rewrites things?

CodaFi · 2020-01-31T16:27:09Z

include/swift/AST/DiagnosticsSema.def

+ERROR(enum_element_pattern_assoc_values_using_labels,none,
+      "using label to match to enum, enum element pattern cannot match against mixed labels and variable patterns.",())
+ERROR(enum_element_pattern_assoc_values_using_var_patterns,none,
+      "using variable patterns to match against enum, enum element pattern cannot match against mixed labels and variable patterns.",())


Same thing here as above.

CodaFi

Left behind a bunch of nits. I have a high-level comment as well which is:
If the general idea is to rewrite patterns to have the appropriate label set, the only correct way to do that is to query the Type itself for that information and rewrite.

This patch also needs to make affordances for other language modes and provide compatibility warnings and notes to help fix these errors.

AndrewLitteken · 2020-01-31T16:41:24Z

I think I may need to take a step back and try to go about this a different way. The fix is pretty different from the initial suggestion in SR-11159 and needs to solve a bigger problem than I thought it was initially (essentially adding Proposal 0155 in).

shahmishal · 2020-10-01T07:00:12Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

AndrewLitteken changed the title ~~SR-11159~~ Fix SR-11159, better checking for enum elements with the same head and different labels Aug 20, 2019

theblixguy reviewed Aug 20, 2019

View reviewed changes

jrose-apple requested review from jrose-apple and CodaFi August 20, 2019 21:11

AndrewLitteken mentioned this pull request Aug 20, 2019

Sr 11159 space engine #26754

Merged

AndrewLitteken force-pushed the SR-11159 branch from 8a12543 to 1a61a7f Compare August 22, 2019 02:06

AndrewLitteken force-pushed the SR-11159 branch from 4585997 to 03e9e8a Compare October 10, 2019 16:11

AndrewLitteken force-pushed the SR-11159 branch from 03e9e8a to 12779a4 Compare October 10, 2019 20:11

theblixguy reviewed Oct 11, 2019

View reviewed changes

CodaFi reviewed Oct 17, 2019

View reviewed changes

lib/Sema/TypeCheckSwitchStmt.cpp Show resolved Hide resolved

AndrewLitteken force-pushed the SR-11159 branch from 52ce73d to 5415e94 Compare January 31, 2020 04:23

CodaFi reviewed Jan 31, 2020

View reviewed changes

test/stmt/switch_stmt2.swift

// Do not crash

enum Foo1

{

Copy link

Contributor

CodaFi Jan 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Please match stdlib style in here.

CodaFi reviewed Jan 31, 2020

View reviewed changes

CodaFi requested changes Jan 31, 2020

View reviewed changes

shahmishal closed this Oct 5, 2020

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

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

Uh oh!

Conversation

AndrewLitteken commented Aug 20, 2019

Uh oh!

AndrewLitteken commented Aug 20, 2019

Uh oh!

theblixguy left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewLitteken Aug 20, 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!

Uh oh!

Uh oh!

theblixguy Aug 20, 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!

CodaFi commented Aug 20, 2019

Uh oh!

AndrewLitteken commented Aug 20, 2019

Uh oh!

CodaFi commented Aug 20, 2019

Uh oh!

AndrewLitteken commented Aug 20, 2019

Uh oh!

AndrewLitteken commented Oct 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CodaFi commented Oct 17, 2019

Uh oh!

CodaFi commented Oct 21, 2019

Uh oh!

AndrewLitteken commented Oct 29, 2019

Uh oh!

AndrewLitteken commented Jan 24, 2020

Uh oh!

CodaFi commented Jan 24, 2020

Uh oh!

AndrewLitteken commented Jan 31, 2020

Uh oh!

theblixguy commented Jan 31, 2020

Uh oh!

swift-ci commented Jan 31, 2020

Uh oh!

swift-ci commented Jan 31, 2020

Uh oh!

theblixguy commented Jan 31, 2020

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

theblixguy left a comment •

edited

Loading

AndrewLitteken Aug 20, 2019 •

edited

Loading

theblixguy Aug 20, 2019 •

edited

Loading

AndrewLitteken commented Jan 31, 2020 •

edited

Loading