Skip to content

SE-0036 (2/3): Provide warnings when referencing enum elements on instances #2842

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.

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
Jul 10, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ ERROR(could_not_use_type_member,none,
ERROR(could_not_use_type_member_on_instance,none,
"static member %1 cannot be used on instance of type %0",
(Type, DeclName))
WARNING(could_not_use_enum_element_on_instance,none,
"referencing enum element %0 as instance member is deprecated and will "
"be removed in Swift 3",
(DeclName))
ERROR(could_not_use_type_member_on_existential,none,
"static member %1 cannot be used on protocol metatype %0",
(Type, DeclName))
Expand Down
10 changes: 9 additions & 1 deletion include/swift/AST/Expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,13 @@ class TypeExpr : public Expr {
TypeLoc Info;
TypeExpr(Type Ty);
public:
/// Whether this type reference actually references an instance but has been
/// promoted to a type reference to access an enum element
///
/// This is purely transitional and will be removed when referencing enum
/// elements on instance members becomes an error
bool IsPromotedInstanceRef = false;

// Create a TypeExpr with location information.
TypeExpr(TypeLoc Ty);

Expand All @@ -1051,7 +1058,8 @@ class TypeExpr : public Expr {

/// Return a TypeExpr for a TypeDecl and the specified location.
static TypeExpr *createForDecl(SourceLoc Loc, TypeDecl *D,
bool isImplicit);
bool isImplicit,
bool isPromotedInstanceRef = false);
static TypeExpr *createForSpecializedDecl(SourceLoc Loc, TypeDecl *D,
ArrayRef<TypeRepr*> args,
SourceRange angleLocs);
Expand Down
7 changes: 7 additions & 0 deletions include/swift/AST/NameLookup.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ struct UnqualifiedLookupResult {
ValueDecl *Value;

public:
/// Whether this should actually reference an instance but has been promoted
/// to a type reference to access an enum element
///
/// This is purely transitional and will be removed when referencing enum
/// elements on instance members becomes an error
bool IsPromotedInstanceRef = false;

UnqualifiedLookupResult(ValueDecl *value) : Base(nullptr), Value(value) { }

UnqualifiedLookupResult(ValueDecl *base, ValueDecl *value)
Expand Down
4 changes: 3 additions & 1 deletion lib/AST/Expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1345,12 +1345,14 @@ Type TypeExpr::getInstanceType() const {

/// Return a TypeExpr for a simple identifier and the specified location.
TypeExpr *TypeExpr::createForDecl(SourceLoc Loc, TypeDecl *Decl,
bool isImplicit) {
bool isImplicit,
bool isPromotedInstanceRef) {
ASTContext &C = Decl->getASTContext();
assert(Loc.isValid());
auto *Repr = new (C) SimpleIdentTypeRepr(Loc, Decl->getName());
Repr->setValue(Decl);
auto result = new (C) TypeExpr(TypeLoc(Repr, Type()));
result->IsPromotedInstanceRef = isPromotedInstanceRef;
if (isImplicit)
result->setImplicit();
return result;
Expand Down
6 changes: 5 additions & 1 deletion lib/AST/NameLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,11 @@ UnqualifiedLookup::UnqualifiedLookup(DeclName Name, DeclContext *DC,
if (FD->isStatic() && !isMetatypeType)
continue;
} else if (isa<EnumElementDecl>(Result)) {
Results.push_back(UnqualifiedLookupResult(MetaBaseDecl, Result));
auto lookupRes = UnqualifiedLookupResult(MetaBaseDecl, Result);
if (!BaseDecl->getType()->is<MetatypeType>()) {
lookupRes.IsPromotedInstanceRef = true;
}
Results.push_back(lookupRes);
continue;
}

Expand Down
18 changes: 18 additions & 0 deletions lib/Sema/CSApply.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2514,6 +2514,24 @@ namespace {

public:
Expr *visitUnresolvedDotExpr(UnresolvedDotExpr *expr) {
if (auto ty = dyn_cast<TypeExpr>(expr->getBase())) {
if (ty->IsPromotedInstanceRef) {
// An enum element was looked up on an instance. Issue a warning
auto enumMetatype = ty->getType()->castTo<AnyMetatypeType>();
auto enumType = enumMetatype->getInstanceType()->castTo<EnumType>();

SmallString<32> enumTypeName;
llvm::raw_svector_ostream typeNameStream(enumTypeName);
typeNameStream << enumType->getDecl()->getName();
typeNameStream << ".";

TypeChecker &tc = cs.getTypeChecker();
tc.diagnose(expr->getLoc(),
diag::could_not_use_enum_element_on_instance,
expr->getName())
.fixItInsert(expr->getLoc(), typeNameStream.str());
}
}
return applyMemberRefExpr(expr, expr->getBase(), expr->getDotLoc(),
expr->getNameLoc(), expr->isImplicit());
}
Expand Down
7 changes: 6 additions & 1 deletion lib/Sema/TypeCheckConstraints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,7 @@ resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC) {

ResultValues.clear();
bool AllMemberRefs = true;
bool PromotedInstanceRef = false;
ValueDecl *Base = 0;
for (auto Result : Lookup) {
// Track the base for member declarations.
Expand All @@ -566,6 +567,9 @@ resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC) {
break;
}

if (Result.IsPromotedInstanceRef) {
PromotedInstanceRef = true;
}
Base = Result.Base;
continue;
}
Expand All @@ -577,7 +581,8 @@ resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC) {
if (AllMemberRefs) {
Expr *BaseExpr;
if (auto NTD = dyn_cast<NominalTypeDecl>(Base)) {
BaseExpr = TypeExpr::createForDecl(Loc, NTD, /*implicit=*/true);
BaseExpr = TypeExpr::createForDecl(Loc, NTD, /*implicit=*/true,
PromotedInstanceRef);
} else {
BaseExpr = new (Context) DeclRefExpr(Base, UDRE->getNameLoc(),
/*implicit=*/true);
Expand Down
16 changes: 11 additions & 5 deletions lib/Sema/TypeCheckNameLookup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,12 @@ namespace {
///
/// \param foundInType The type through which we found the
/// declaration.
void add(ValueDecl *found, ValueDecl *base, Type foundInType) {
///
/// \param promotedInstanceRef true if the lookup result to be added was
/// actually looked up on an instance but promted to a type to look up an
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - "promted"

I would just name this "fixItAddLeadingDot" or something really explicit.

/// enum element
void add(ValueDecl *found, ValueDecl *base, Type foundInType,
bool promotedInstanceRef = false) {
// If we only want types, AST name lookup should not yield anything else.
assert(!Options.contains(NameLookupFlags::OnlyTypes) ||
isa<TypeDecl>(found));
Expand Down Expand Up @@ -139,7 +144,7 @@ namespace {
isa<GenericTypeParamDecl>(found) ||
(isa<FuncDecl>(found) && cast<FuncDecl>(found)->isOperator())) {
if (Known.insert({{found, base}, false}).second) {
Result.add({found, base});
Result.add({found, base, promotedInstanceRef});
FoundDecls.push_back(found);
}
return;
Expand Down Expand Up @@ -172,7 +177,7 @@ namespace {
// default implementations in protocols.
if (witness && !isa<ProtocolDecl>(witness->getDeclContext())) {
if (Known.insert({{witness, base}, false}).second) {
Result.add({witness, base});
Result.add({witness, base, promotedInstanceRef});
FoundDecls.push_back(witness);
}
}
Expand Down Expand Up @@ -229,7 +234,8 @@ LookupResult TypeChecker::lookupUnqualified(DeclContext *dc, DeclName name,
assert(foundInType && "bogus base declaration?");
}

builder.add(found.getValueDecl(), found.getBaseDecl(), foundInType);
builder.add(found.getValueDecl(), found.getBaseDecl(), foundInType,
found.IsPromotedInstanceRef);
}
return result;
}
Expand Down Expand Up @@ -566,7 +572,7 @@ void TypeChecker::performTypoCorrection(DeclContext *DC, DeclRefKind refKind,
entries.filterMaxScoreRange(MaxCallEditDistanceFromBestCandidate);

for (auto &entry : entries)
result.add({ entry.Value, nullptr });
result.add({ entry.Value, nullptr, false });
Copy link
Contributor

Choose a reason for hiding this comment

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

/promotedInstanceRef=/false would be clearer.

Also, a similar "label" for the nullptr parameter would be nice too.

}

static InFlightDiagnostic
Expand Down
54 changes: 48 additions & 6 deletions lib/Sema/TypeCheckPattern.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,21 @@ extractEnumElement(TypeChecker &TC, DeclContext *DC, SourceLoc UseLoc,
///
/// If there are no enum elements but there are properties, attempts to map
/// an arbitrary property to an enum element using extractEnumElement.
///
/// \param isPromoted If set to anything but the \c nullptr, this will be set to
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, would be best to give this an explicit name. But since this is transitional, maybe not worth it...

/// \c true if the found enum element is referenced as on an instance
/// but the lookup has been promoted to be on the type instead
/// This is purely transitional and will be removed when referencing enum
/// elements on instance members becomes an error

static EnumElementDecl *
filterForEnumElement(TypeChecker &TC, DeclContext *DC, SourceLoc UseLoc,
LookupResult foundElements) {
LookupResult foundElements, bool *isPromoted = nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it actually ever nullptr? Maybe a default value isn't needed

EnumElementDecl *foundElement = nullptr;
VarDecl *foundConstant = nullptr;

for (ValueDecl *e : foundElements) {
for (LookupResult::Result result : foundElements) {
ValueDecl *e = result.Decl;
assert(e);
if (e->isInvalid()) {
continue;
Expand All @@ -80,6 +88,9 @@ filterForEnumElement(TypeChecker &TC, DeclContext *DC, SourceLoc UseLoc,
// Ambiguities should be ruled out by parsing.
assert(!foundElement && "ambiguity in enum case name lookup?!");
foundElement = oe;
if (isPromoted != nullptr) {
*isPromoted = result.IsPromotedInstanceRef;
}
continue;
}

Expand All @@ -96,13 +107,20 @@ filterForEnumElement(TypeChecker &TC, DeclContext *DC, SourceLoc UseLoc,
}

/// Find an unqualified enum element.
///
/// \param IsPromoted If set to anything but the \c nullptr, this will be set to
/// \c true if the found enum element is referenced as on an instance
/// but the lookup has been promoted to be on the type instead
/// This is purely transitional and will be removed when referencing enum
/// elements on instance members becomes an error
static EnumElementDecl *
lookupUnqualifiedEnumMemberElement(TypeChecker &TC, DeclContext *DC,
Identifier name, SourceLoc UseLoc) {
Identifier name, SourceLoc UseLoc,
bool *IsPromoted = nullptr) {
auto lookupOptions = defaultUnqualifiedLookupOptions;
lookupOptions |= NameLookupFlags::KnownPrivate;
auto lookup = TC.lookupUnqualified(DC, name, SourceLoc(), lookupOptions);
return filterForEnumElement(TC, DC, UseLoc, lookup);
return filterForEnumElement(TC, DC, UseLoc, lookup, IsPromoted);
}

/// Find an enum element in an enum type.
Expand Down Expand Up @@ -477,10 +495,22 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
// rdar://20879992 is addressed.
//
// Try looking up an enum element in context.

// Check if the enum element was actually accessed on an instance and was
// promoted to be looked up on a type. If so, provide a warning
bool isPromotedInstance = false;

if (EnumElementDecl *referencedElement
= lookupUnqualifiedEnumMemberElement(TC, DC,
ude->getName().getBaseName(),
ude->getLoc())) {
ude->getLoc(),
&isPromotedInstance)) {
if (isPromotedInstance) {
TC.diagnose(ude->getLoc(), diag::could_not_use_enum_element_on_instance,
ude->getName())
.fixItInsert(ude->getLoc(), ".");
}

auto *enumDecl = referencedElement->getParentEnum();
auto enumTy = enumDecl->getDeclaredTypeInContext();
TypeLoc loc = TypeLoc::withoutLoc(enumTy);
Expand Down Expand Up @@ -563,9 +593,21 @@ class ResolvePattern : public ASTVisitor<ResolvePattern,
// If we had a single component, try looking up an enum element in context.
if (auto compId = dyn_cast<ComponentIdentTypeRepr>(repr)) {
// Try looking up an enum element in context.

// Check if the enum element was actually accessed on an instance and was
// promoted to be looked up on a type. If so, provide a warning
bool isPromoted = false;
EnumElementDecl *referencedElement
= lookupUnqualifiedEnumMemberElement(TC, DC, compId->getIdentifier(),
repr->getLoc());
repr->getLoc(), &isPromoted);

if (isPromoted) {
TC.diagnose(compId->getLoc(),
diag::could_not_use_enum_element_on_instance,
compId->getIdentifier())
.fixItInsert(compId->getLoc(), ".");
}


if (!referencedElement)
return nullptr;
Expand Down
7 changes: 7 additions & 0 deletions lib/Sema/TypeChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@ class LookupResult {
/// The base declaration through which we found the declaration.
ValueDecl *Base;

/// Whether this should actually reference an instance but has been promoted
/// to a type reference to access an enum element
///
/// This is purely transitional and will be removed when referencing enum
/// elements on instance members becomes an error
bool IsPromotedInstanceRef;

operator ValueDecl*() const { return Decl; }
ValueDecl *operator->() const { return Decl; }
};
Expand Down
89 changes: 89 additions & 0 deletions test/Parse/enum.swift
Original file line number Diff line number Diff line change
Expand Up @@ -433,3 +433,92 @@ enum RawValueBTest: Double, RawValueB {
enum foo : String {
case bar = nil // expected-error {{cannot convert nil to raw type 'String'}}
}

// Static member lookup from instance methods

struct EmptyStruct {}

enum EnumWithStaticMember {
static let staticVar = EmptyStruct()

func foo() {
let _ = staticVar // expected-error {{static member 'staticVar' cannot be used on instance of type 'EnumWithStaticMember'}}
}
}

// SE-0036:

struct SE0036_Auxiliary {}

enum SE0036 {
case A
case B(SE0036_Auxiliary)

static func staticReference() {
_ = A
_ = SE0036.A
}

func staticReferencInInstanceMethod() {
_ = A // expected-warning {{referencing enum element 'A' as instance member is deprecated and will be removed in Swift 3}} {{9-9=SE0036.}}
_ = SE0036.A
}

static func staticReferencInSwitchInStaticMethod() {
switch SE0036.A {
case A: break
case B(_): break
}
}

func staticReferencInSwitchInInstanceMethod() {
switch self {
case A: break // expected-warning {{referencing enum element 'A' as instance member is deprecated and will be removed in Swift 3}} {{10-10=.}}
case B(_): break // expected-warning {{referencing enum element 'B' as instance member is deprecated and will be removed in Swift 3}} {{10-10=.}}
}
}

func explicitReferencInSwitch() {
switch SE0036.A {
case SE0036.A: break
case SE0036.B(_): break
}
}

func dotReferencInSwitchInInstanceMethod() {
switch self {
case .A: break
case .B(_): break
}
}

static func dotReferencInSwitchInStaticMethod() {
switch SE0036.A {
case .A: break
case .B(_): break
}
}

init() {
self = .A
self = A // expected-warning {{referencing enum element 'A' as instance member is deprecated and will be removed in Swift 3}} {{12-12=SE0036.}}
self = SE0036.A
self = .B(SE0036_Auxiliary())
self = B(SE0036_Auxiliary()) // expected-warning {{referencing enum element 'B' as instance member is deprecated and will be removed in Swift 3}} {{12-12=SE0036.}}
self = SE0036.B(SE0036_Auxiliary())
}
}

enum SE0036_Generic<T> {
case A(x: T)

func foo() {
switch self {
case A(_): break // expected-warning {{referencing enum element 'A' as instance member is deprecated and will be removed in Swift 3}} {{10-10=.}}
}

switch self {
case SE0036_Generic.A(let a): print(a)
}
}
}