Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

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

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
4 changes: 4 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.",())
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

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

and a nice note that rewrites things?

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.",())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, what's a "variable pattern"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing here as above.

ERROR(possibly_non_exhaustive_switch,none,
"the compiler is unable to check that this switch is exhaustive in reasonable time",
())
Expand Down
9 changes: 4 additions & 5 deletions include/swift/AST/Pattern.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,14 +513,13 @@ class EnumElementPattern : public Pattern {
EnumElementPattern(TypeLoc ParentType, SourceLoc DotLoc, DeclNameLoc NameLoc,
DeclNameRef Name, EnumElementDecl *Element,
Pattern *SubPattern, Optional<bool> Implicit = None)
: Pattern(PatternKind::EnumElement),
ParentType(ParentType), DotLoc(DotLoc), NameLoc(NameLoc), Name(Name),
ElementDeclOrUnresolvedOriginalExpr(Element),
SubPattern(SubPattern) {
: Pattern(PatternKind::EnumElement), ParentType(ParentType),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

DotLoc(DotLoc), NameLoc(NameLoc), Name(Name),
ElementDeclOrUnresolvedOriginalExpr(Element), SubPattern(SubPattern) {
if (Implicit.hasValue() && *Implicit)
setImplicit();
}

/// Create an unresolved EnumElementPattern for a `.foo` pattern relying on
/// contextual type.
EnumElementPattern(SourceLoc DotLoc,
Expand Down
227 changes: 193 additions & 34 deletions lib/Sema/TypeCheckPattern.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,83 @@
#include "TypeCheckAvailability.h"
#include "TypeCheckType.h"
#include "swift/Basic/StringExtras.h"
#include "swift/AST/ASTWalker.h"
#include "swift/AST/ASTVisitor.h"
#include "swift/AST/SourceFile.h"
#include "swift/AST/ASTWalker.h"
#include "swift/AST/NameLookup.h"
#include "swift/AST/SourceFile.h"
#include "swift/AST/ParameterList.h"
#include "swift/AST/TypeCheckRequests.h"
#include "llvm/Support/SaveAndRestore.h"
#include <utility>
using namespace swift;

llvm::SmallVector<Identifier, 4> extractArgsFromPattern(ASTContext &Context,
Pattern *pattern) {
auto &diags = Context.Diags;
llvm::SmallVector<Identifier, 4> params;
// Get Pattern kind
switch (pattern->getKind()) {
case PatternKind::Tuple: {
// Walk through tuple to get argument names
TuplePattern *tp = cast<TuplePattern>(pattern);
Optional<bool> usingLabels = None;
for (auto elem : tp->getElements()) {
Identifier label = elem.getLabel();
if (label.empty()) {
if (elem.getPattern()->getKind() == PatternKind::Any)
params.push_back(Identifier());
else
// Set None if not using labels, throw error if contradicts
if (usingLabels == None)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

usingLabels = false;
else if (usingLabels == true) {
diags.diagnose(pattern->getLoc(),
diag::enum_element_pattern_assoc_values_using_labels);
}
llvm::SmallVector<Identifier, 4> subParams =
extractArgsFromPattern(Context, elem.getPattern());
if (subParams.size() == 1)
params.push_back(subParams[0]);
else
params.push_back(Identifier());
} else {
// Set None if using labels, throw error if contradicts
if (usingLabels == None)
usingLabels = true;
else if (usingLabels == false) {
diags.diagnose(
pattern->getLoc(),
diag::enum_element_pattern_assoc_values_using_var_patterns);
}
params.push_back(label);
}
}
return params;
break;
}
case PatternKind::Paren: {
ParenPattern *pp = cast<ParenPattern>(pattern);
return extractArgsFromPattern(Context, pp->getSubPattern());
}
case PatternKind::Var: {
VarPattern *vp = cast<VarPattern>(pattern);
return extractArgsFromPattern(Context, vp->getSubPattern());
}
// Relevant variable names are contained in elements in Tuple
case PatternKind::Named: {
// Get name and set argument to be from a variable
NamedPattern *np = cast<NamedPattern>(pattern);
Identifier id = np->getBoundName();
params.push_back(id);
return params;
}
default:
params.push_back(Identifier());
return params;
}
return params;
}

/// If the given VarDecl is a computed property whose getter always returns a
/// particular enum element, return that element.
///
Expand Down Expand Up @@ -78,7 +145,6 @@ filterForEnumElement(DeclContext *DC, SourceLoc UseLoc,
bool unqualifiedLookup, LookupResult foundElements) {
EnumElementDecl *foundElement = nullptr;
VarDecl *foundConstant = nullptr;

for (LookupResultEntry result : foundElements) {
ValueDecl *e = result.getValueDecl();
assert(e);
Expand Down Expand Up @@ -135,13 +201,78 @@ lookupEnumMemberElement(DeclContext *DC, Type ty,
return nullptr;

// FIXME: We should probably pay attention to argument labels someday.
name = name.withoutArgumentLabels();
DeclNameRef nameWithout = name.withoutArgumentLabels();

// Look up the case inside the enum.
// FIXME: We should be able to tell if this is a private lookup.
NameLookupOptions lookupOptions = defaultMemberLookupOptions;
LookupResult foundElements =
TypeChecker::lookupMember(DC, ty, name, lookupOptions);
TypeChecker::lookupMember(DC, ty, nameWithout, lookupOptions);

ArrayRef<Identifier> queryParams = name.getArgumentNames();

if (foundElements.size() != 1) {
// Look up enum element only with base name
foundElements = TypeChecker::lookupMember(
DC, ty, name.withoutArgumentLabels(), lookupOptions);

int numFoundEntries = 0;
LookupResult singleResult;
for (LookupResultEntry resultEnt : foundElements) {
auto *resultDecl = resultEnt.getValueDecl();
if (isa<VarDecl>(resultDecl)) {
singleResult.add(resultEnt, false);
continue;
}
if (numFoundEntries == 0) {
singleResult.add(resultEnt, false);
}
numFoundEntries++;
}
if (numFoundEntries == 1)
return filterForEnumElement(DC, UseLoc,
/*unqualifiedLookup=*/false, singleResult);

LookupResult revisedResult;
// Prune results that don't match
for (LookupResultEntry resultEnt : foundElements) {
auto *e = resultEnt.getValueDecl();
// Maintaining part of list list for filter results
if (auto *var = dyn_cast<VarDecl>(e)) {
revisedResult.add(resultEnt, false);
continue;
}
DeclName enumElement = e->getFullName();
ArrayRef<Identifier> enumElementParams = enumElement.getArgumentNames();

// If size is not the same, and query is not just a single value
// do not consider
if (enumElementParams.size() != queryParams.size())
continue;
bool addCandidate = true;
for (auto index : indices(enumElementParams)) {
// If enum empty, case must be empty or have let var form
if (!enumElementParams[index].empty()) {
// If element not empty, enum element can be empty or from var
// if this is the only element found with basename
if (queryParams[index].empty()) {
addCandidate = false;
break;
}
// must match the label
if (!queryParams[index].compare(enumElementParams[index]))
continue;
addCandidate = false;
break;
}
}
if (addCandidate) {
revisedResult.add(resultEnt, false);
}
}
foundElements = revisedResult;
}

return filterForEnumElement(DC, UseLoc,
/*unqualifiedLookup=*/false, foundElements);
}
Expand Down Expand Up @@ -443,13 +574,26 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
if (auto arg = ume->getArgument()) {
subPattern = getSubExprPattern(arg);
}

if (ume->getName().getBaseName().isSpecial())
return nullptr;

return new (Context)
EnumElementPattern(ume->getDotLoc(), ume->getNameLoc(), ume->getName(),
subPattern, ume);
// FIXME: Compound Name. <- Decide if this is covered
if (subPattern) {
ArrayRef<Identifier> namedParams =
extractArgsFromPattern(Context, subPattern);

if (namedParams.size() != 0) {
// FIXME: Compound names.
return new (Context) EnumElementPattern(
ume->getDotLoc(), ume->getNameLoc(),
DeclNameRef(
DeclName(Context, ume->getName().getBaseName(), namedParams)),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

subPattern, ume);
}
}
return new (Context) EnumElementPattern(ume->getDotLoc(), ume->getNameLoc(),
ume->getName(), subPattern, ume);
}

// Member syntax 'T.Element' forms a pattern if 'T' is an enum and the
Expand All @@ -473,25 +617,26 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
if (!enumDecl)
return nullptr;

EnumElementDecl *referencedElement
= lookupEnumMemberElement(DC, ty, ude->getName(), ude->getLoc());
// FIXME: Argument labels?
EnumElementDecl *referencedElement =
lookupEnumMemberElement(DC, ty, ude->getName(), ude->getLoc());
if (!referencedElement)
return nullptr;

// Build a TypeRepr from the head of the full path.
TypeLoc loc(repr);
loc.setType(ty);
return new (Context) EnumElementPattern(
loc, ude->getDotLoc(), ude->getNameLoc(), ude->getName(),
referencedElement, nullptr);
return new (Context)
EnumElementPattern(loc, ude->getDotLoc(), ude->getNameLoc(),
ude->getName(), referencedElement, nullptr);
}

// A DeclRef 'E' that refers to an enum element forms an EnumElementPattern.
Pattern *visitDeclRefExpr(DeclRefExpr *de) {
auto *elt = dyn_cast<EnumElementDecl>(de->getDecl());
if (!elt)
return nullptr;

// Use the type of the enum from context.
TypeLoc loc = TypeLoc::withoutLoc(
elt->getParentEnum()->getDeclaredTypeInContext());
Expand All @@ -511,9 +656,9 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
auto enumTy = enumDecl->getDeclaredTypeInContext();
TypeLoc loc = TypeLoc::withoutLoc(enumTy);

return new (Context) EnumElementPattern(
loc, SourceLoc(), ude->getNameLoc(), ude->getName(),
referencedElement, nullptr);
return new (Context)
EnumElementPattern(loc, SourceLoc(), ude->getNameLoc(),
ude->getName(), referencedElement, nullptr);
}


Expand Down Expand Up @@ -1273,11 +1418,12 @@ Pattern *TypeChecker::coercePatternToType(ContextualPattern pattern,
Optional<CheckedCastKind> castKind;

EnumElementDecl *elt = EEP->getElementDecl();

Type enumTy;
if (!elt) {
elt = lookupEnumMemberElement(dc, type, EEP->getName(),
EEP->getLoc());

if (!elt) {
if (!type->hasError()) {
// Lowercasing of Swift.Optional's cases is handled in the
Expand All @@ -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" ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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>

Copy link
Contributor

Choose a reason for hiding this comment

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

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

EEP->getName().getBaseIdentifier().str() == "Some") {
SmallString<4> Rename;
camel_case::toLowercaseWord(EEP->getName()
.getBaseIdentifier().str(),
Rename);
camel_case::toLowercaseWord(
EEP->getName().getBaseIdentifier().str(), Rename);
diags.diagnose(
EEP->getLoc(), diag::availability_decl_unavailable_rename,
/*"getter" prefix*/ 2, EEP->getName().getBaseName(),
/*"getter" prefix*/ 2, EEP->getName().getBaseIdentifier(),
/*replaced*/ false, /*special kind*/ 0, Rename.str(),
/*message*/ StringRef())
.fixItReplace(EEP->getLoc(), Rename.str());
/*message*/ StringRef());

return nullptr;
}

// If we have the original expression parse tree, try reinterpreting
// it as an expr-pattern if enum element lookup failed, since `.foo`
// could also refer to a static member of the context type.
Expand Down Expand Up @@ -1427,18 +1571,33 @@ Pattern *TypeChecker::coercePatternToType(ContextualPattern pattern,

// If there is a subpattern, push the enum element type down onto it.
auto argType = elt->getArgumentInterfaceType();
if (EEP->hasSubPattern()) {
if (EEP->hasSubPattern() || EEP->getName().getArgumentNames().size() > 0) {
Pattern *sub = EEP->getSubPattern();
if (!elt->hasAssociatedValues()) {
diags.diagnose(EEP->getLoc(),
diag::enum_element_pattern_assoc_values_mismatch,
EEP->getName());
diags.diagnose(EEP->getLoc(),
diag::enum_element_pattern_assoc_values_remove)
.fixItRemove(sub->getSourceRange());
DeclNameRef(EEP->getName().getBaseName()));
diags
.diagnose(EEP->getLoc(),
diag::enum_element_pattern_assoc_values_remove)
.fixItRemove(sub->getSourceRange());
;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: stray semicolon

return nullptr;
}


if (!sub) {
SmallVector<TuplePatternElt, 8> elements;
for (auto &elt : EEP->getName().getArgumentNames()) {
auto *subPattern =
new (Context) AnyPattern(EEP->getNameLoc().getBaseNameLoc());
elements.push_back(TuplePatternElt(
elt, EEP->getNameLoc().getBaseNameLoc(), subPattern));
}
sub = TuplePattern::create(Context, EEP->getNameLoc().getBaseNameLoc(),
elements, EEP->getEndLoc(),
/*implicit*/ true);
}

Type elementType;
if (argType)
elementType = enumTy->getTypeOfMember(elt->getModuleContext(),
Expand Down
6 changes: 5 additions & 1 deletion lib/Sema/TypeCheckSwitchStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,9 @@ namespace {
// Optimization: If the constructor heads don't match, subspace is
// impossible.

if (this->Head != other.Head) {
// FIXME: Extraneous warnings for cases with labels
if (this->Head.getBaseIdentifier() !=
other.Head.getBaseIdentifier()) {
return false;
}

Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Hold on: is this still not implemented properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (this->Head.getBaseIdentifier() !=
other.Head.getBaseIdentifier()) {
return *this;
Expand Down
Loading