-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
/// 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)); | ||
|
@@ -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; | ||
|
@@ -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); | ||
} | ||
} | ||
|
@@ -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; | ||
} | ||
|
@@ -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 }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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; | ||
} | ||
|
||
|
@@ -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. | ||
|
@@ -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); | ||
|
@@ -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; | ||
|
There was a problem hiding this comment.
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.