Skip to content

Sr 11159 space engine #26754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 4, 2019
Merged

Sr 11159 space engine #26754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 51 additions & 28 deletions lib/Sema/TypeCheckSwitchStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@
#include "swift/Basic/APIntMap.h"
#include <llvm/ADT/APFloat.h>

#include <numeric>
#include <forward_list>
#include <iterator>
#include <numeric>
#include <utility>

using namespace swift;

Expand Down Expand Up @@ -115,7 +117,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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

std::forward_list<Space> Spaces;

size_t computeSize(TypeChecker &TC, const DeclContext *DC,
Expand Down Expand Up @@ -170,19 +172,18 @@ namespace {
llvm_unreachable("unhandled kind");
}

explicit Space(Type T, Identifier NameForPrinting)
: Kind(SpaceKind::Type), TypeAndVal(T),
Head(NameForPrinting), Spaces({}){}
explicit Space(Type T, DeclName NameForPrinting)
: Kind(SpaceKind::Type), TypeAndVal(T), Head(NameForPrinting),
Spaces({}) {}
explicit Space(UnknownCase_t, bool allowedButNotRequired)
: Kind(SpaceKind::UnknownCase),
TypeAndVal(Type(), allowedButNotRequired), Head(Identifier()),
Spaces({}) {}
explicit Space(Type T, Identifier H, ArrayRef<Space> SP)
: Kind(SpaceKind::Constructor), TypeAndVal(T), Head(H),
Spaces(SP.begin(), SP.end()) {}
explicit Space(Type T, Identifier H, std::forward_list<Space> SP)
: Kind(SpaceKind::Constructor), TypeAndVal(T), Head(H),
Spaces(SP) {}
explicit Space(Type T, DeclName H, ArrayRef<Space> SP)
: Kind(SpaceKind::Constructor), TypeAndVal(T), Head(H),
Spaces(SP.begin(), SP.end()) {}
explicit Space(Type T, DeclName H, std::forward_list<Space> SP)
: Kind(SpaceKind::Constructor), TypeAndVal(T), Head(H), Spaces(SP) {}
explicit Space(ArrayRef<Space> SP)
: Kind(SpaceKind::Disjunct), TypeAndVal(Type()),
Head(Identifier()), Spaces(SP.begin(), SP.end()) {}
Expand All @@ -194,23 +195,23 @@ namespace {
: Kind(SpaceKind::Empty), TypeAndVal(Type()), Head(Identifier()),
Spaces({}) {}

static Space forType(Type T, Identifier NameForPrinting) {
static Space forType(Type T, DeclName NameForPrinting) {
if (T->isStructurallyUninhabited())
return Space();
return Space(T, NameForPrinting);
}
static Space forUnknown(bool allowedButNotRequired) {
return Space(UnknownCase, allowedButNotRequired);
}
static Space forConstructor(Type T, Identifier H, ArrayRef<Space> SP) {
static Space forConstructor(Type T, DeclName H, ArrayRef<Space> SP) {
if (llvm::any_of(SP, std::mem_fn(&Space::isEmpty))) {
// A constructor with an unconstructible parameter can never actually
// be used.
return Space();
}
return Space(T, H, SP);
}
static Space forConstructor(Type T, Identifier H,
static Space forConstructor(Type T, DeclName H,
std::forward_list<Space> SP) {
// No need to filter SP here; this is only used to copy other
// Constructor spaces.
Expand Down Expand Up @@ -263,7 +264,7 @@ namespace {
return TypeAndVal.getPointer();
}

Identifier getHead() const {
DeclName getHead() const {
assert(getKind() == SpaceKind::Constructor
&& "Wrong kind of space tried to access head");
return Head;
Expand All @@ -272,7 +273,7 @@ namespace {
Identifier getPrintingName() const {
assert(getKind() == SpaceKind::Type
&& "Wrong kind of space tried to access printing name");
return Head;
return Head.getBaseIdentifier();
}

const std::forward_list<Space> &getSpaces() const {
Expand Down Expand Up @@ -333,7 +334,7 @@ namespace {
return this->isSubspace(or2Space, TC, DC);
}

return true;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that the validation-test?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's part of it, yes.

}
PAIRCASE (SpaceKind::Type, SpaceKind::Disjunct): {
// (_ : Ty1) <= (S1 | ... | Sn) iff (S1 <= S) || ... || (Sn <= S)
Expand Down Expand Up @@ -372,10 +373,11 @@ namespace {
PAIRCASE (SpaceKind::Constructor, SpaceKind::Constructor): {
// Optimization: If the constructor heads don't match, subspace is
// impossible.

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

// Special Case: Short-circuit comparisons with payload-less
// constructors.
if (other.getSpaces().empty()) {
Expand Down Expand Up @@ -557,7 +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.
if (this->Head != other.Head) {
if (this->Head.getBaseIdentifier() !=
other.Head.getBaseIdentifier()) {
return *this;
}

Expand Down Expand Up @@ -696,19 +699,40 @@ namespace {
buffer << (getBoolValue() ? "true" : "false");
break;
case SpaceKind::Constructor: {
if (!Head.empty()) {
if (!Head.getBaseIdentifier().empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

buffer << ".";
buffer << Head.str();
buffer << Head.getBaseIdentifier().str();
}

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

auto args = Head.getArgumentNames().begin();
auto argEnd = Head.getArgumentNames().end();

// FIXME: Clean up code for performance
buffer << "(";
interleave(Spaces, [&](const Space &param) {
param.show(buffer, forDisplay);
}, [&buffer]() { buffer << ", "; });
llvm::SmallVector<std::pair<Identifier, Space>, 4> labelSpaces;
for (auto param : Spaces) {
if (args != argEnd) {
labelSpaces.push_back(
std::pair<Identifier, Space>(*args, param));
args++;
} else
labelSpaces.push_back(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, just an assert.

Copy link
Contributor Author

@AndrewLitteken AndrewLitteken Sep 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

std::pair<Identifier, Space>(Identifier(), param));
}
interleave(
labelSpaces,
[&](const std::pair<Identifier, Space> &param) {
if (!param.first.empty()) {
buffer << param.first;
buffer << ": ";
}
param.second.show(buffer, forDisplay);
},
[&buffer]() { buffer << ", "; });
buffer << ")";
}
break;
Expand Down Expand Up @@ -806,7 +830,7 @@ namespace {
Space::forType(TTy->getUnderlyingType(), Identifier()));
}
}
return Space::forConstructor(tp, eed->getName(),
return Space::forConstructor(tp, eed->getFullName(),
constElemSpaces);
});

Expand Down Expand Up @@ -967,7 +991,6 @@ namespace {
return;

Space projection = projectPattern(TC, caseItem.getPattern());

bool isRedundant = !projection.isEmpty() &&
llvm::any_of(spaces, [&](const Space &handled) {
return projection.isSubspace(handled, TC, DC);
Expand Down Expand Up @@ -1420,8 +1443,8 @@ namespace {
auto subSpace = projectPattern(TC, OSP->getSubPattern());
// To match patterns like (_, _, ...)?, we must rewrite the underlying
// tuple pattern to .some(_, _, ...) first.
if (subSpace.getKind() == SpaceKind::Constructor
&& subSpace.getHead().empty()) {
if (subSpace.getKind() == SpaceKind::Constructor &&
subSpace.getHead().getBaseIdentifier().empty()) {
return Space::forConstructor(item->getType(), name,
std::move(subSpace.getSpaces()));
}
Expand Down
22 changes: 11 additions & 11 deletions test/FixCode/fixits-switch.swift.result
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,19 +55,19 @@ func foo4(_ e : E2) -> Int {
switch e {
case .e2:
return 1
case .e1(let a, let s):
case .e1(a: let a, s: let s):
<#code#>
case .e3(let a):
case .e3(a: let a):
<#code#>
case .e4(_):
<#code#>
case .e5(_, _):
<#code#>
case .e6(let a, _):
case .e6(a: let a, _):
<#code#>
case .e7:
<#code#>
case .e8(let a, _, _):
case .e8(a: let a, _, _):
<#code#>
case .e9(_, _, _):
<#code#>
Expand All @@ -93,19 +93,19 @@ func foo6(_ e : E2) -> Int {
switch e {
case let .e1(x, y):
return x + y
case .e2(let a):
case .e2(a: let a):
<#code#>
case .e3(let a):
case .e3(a: let a):
<#code#>
case .e4(_):
<#code#>
case .e5(_, _):
<#code#>
case .e6(let a, _):
case .e6(a: let a, _):
<#code#>
case .e7:
<#code#>
case .e8(let a, _, _):
case .e8(a: let a, _, _):
<#code#>
case .e9(_, _, _):
<#code#>
Expand All @@ -117,17 +117,17 @@ func foo7(_ e : E2) -> Int {
case .e2(1): return 0
case .e1: return 0
case .e3: return 0
case .e2(let a):
case .e2(a: let a):
<#code#>
case .e4(_):
<#code#>
case .e5(_, _):
<#code#>
case .e6(let a, _):
case .e6(a: let a, _):
<#code#>
case .e7:
<#code#>
case .e8(let a, _, _):
case .e8(a: let a, _, _):
<#code#>
case .e9(_, _, _):
<#code#>
Expand Down
4 changes: 2 additions & 2 deletions test/Sema/exhaustive_switch.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,13 +1196,13 @@ enum Z {

func sr11160_extra() {
switch Z.z1(a: 1) { // expected-error {{switch must be exhaustive}}
// expected-note@-1 {{add missing case: '.z1(let a)'}}
// expected-note@-1 {{add missing case: '.z1(a: let a)'}}
case .z2(_, _): ()
case .z3(_): ()
}

switch Z.z1(a: 1) { // expected-error {{switch must be exhaustive}}
// expected-note@-1 {{add missing case: '.z2(let a, let b)'}}
// expected-note@-1 {{add missing case: '.z2(a: let a, b: let b)'}}
case .z1(_): ()
case .z3(_): ()
}
Expand Down