Skip to content

[Diagnostics] Fix diagnostics to produce valid rename fixit for SubscriptExpr #39091

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 2 commits into from
Aug 31, 2021
Merged

[Diagnostics] Fix diagnostics to produce valid rename fixit for SubscriptExpr #39091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51176d4
Fix diagnostics to produce valid rename fixit for SubscriptExpr
mininny Aug 28, 2021
3e8b076
PR Review
mininny Aug 31, 2021
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
2 changes: 2 additions & 0 deletions include/swift/Parse/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1869,6 +1869,8 @@ struct ParsedDeclName {
/// Whether this is a setter for the named property.
bool IsSetter = false;

bool IsSubscript = false;

/// For a declaration name that makes the declaration into an
/// instance member, the index of the "Self" parameter.
Optional<unsigned> SelfIndex;
Expand Down
5 changes: 5 additions & 0 deletions lib/Parse/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,11 @@ ParsedDeclName swift::parseDeclName(StringRef name) {
baseName = baseName.substr(7);
}

// If the base name is prefixed by "subscript", it's an subscript.
if (baseName == "subscript") {
result.IsSubscript = true;
}

// Parse the base name.
if (parseBaseName(baseName)) return ParsedDeclName();

Expand Down
112 changes: 73 additions & 39 deletions lib/Sema/TypeCheckAvailability.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1857,7 +1857,7 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
SourceRange referenceRange,
const ValueDecl *renamedDecl,
const AvailableAttr *attr,
const ApplyExpr *call) {
const Expr *call) {
if (isa<AccessorDecl>(renamedDecl))
return;

Expand All @@ -1877,21 +1877,21 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,

auto &ctx = renamedDecl->getASTContext();
SourceManager &sourceMgr = ctx.SourceMgr;

if (parsed.isInstanceMember()) {
auto *CE = dyn_cast_or_null<CallExpr>(call);
if (!CE)
return;

// Replace the base of the call with the "self argument".
// We can only do a good job with the fix-it if we have the whole call
// expression.
// FIXME: Should we be validating the ContextName in some way?
if (!call || !isa<CallExpr>(call))
return;

unsigned selfIndex = parsed.SelfIndex.getValue();
const Expr *selfExpr = nullptr;
SourceLoc removeRangeStart;
SourceLoc removeRangeEnd;

auto *argExpr = call->getArg();
auto *argExpr = CE->getArg();
auto argList = getOriginalArgumentList(argExpr);

size_t numElementsWithinParens = argList.args.size();
Expand Down Expand Up @@ -1988,54 +1988,91 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
selfReplace += base;
if (needsParens)
selfReplace.push_back(')');

selfReplace.push_back('.');
selfReplace += parsed.BaseName;
diag.fixItReplace(call->getFn()->getSourceRange(), selfReplace);

diag.fixItReplace(CE->getFn()->getSourceRange(), selfReplace);

if (!parsed.isPropertyAccessor())
diag.fixItRemoveChars(removeRangeStart, removeRangeEnd);

// Continue on to diagnose any argument label renames.

} else if (parsed.BaseName == "init" && isa_and_nonnull<CallExpr>(call)) {
auto *CE = cast<CallExpr>(call);

// For initializers, replace with a "call" of the context type...but only
// if we know we're doing a call (rather than a first-class reference).
if (parsed.isMember()) {
diag.fixItReplace(call->getFn()->getSourceRange(), parsed.ContextName);

} else if (auto *dotCall = dyn_cast<DotSyntaxCallExpr>(call->getFn())) {
diag.fixItReplace(CE->getFn()->getSourceRange(), parsed.ContextName);
} else if (auto *dotCall = dyn_cast<DotSyntaxCallExpr>(CE->getFn())) {
SourceLoc removeLoc = dotCall->getDotLoc();
if (removeLoc.isInvalid())
return;

diag.fixItRemove(SourceRange(removeLoc, dotCall->getFn()->getEndLoc()));
} else if (!isa<ConstructorRefCallExpr>(call->getFn())) {
} else if (!isa<ConstructorRefCallExpr>(CE->getFn())) {
return;
}

// Continue on to diagnose any constructor argument label renames.


} else if (parsed.IsSubscript) {
if (auto *CE = dyn_cast_or_null<CallExpr>(call)) {
// Renaming from CallExpr to SubscriptExpr. Remove function name and
// replace parens with square brackets.

if (auto *DSCE = dyn_cast<DotSyntaxCallExpr>(CE->getFn())) {
if (DSCE->getBase()->isImplicit()) {
// If self is implicit, self must be inserted before subscript syntax.
diag.fixItInsert(CE->getStartLoc(), "self");
}
}

diag.fixItReplace(CE->getFn()->getEndLoc(), "[");
diag.fixItReplace(CE->getEndLoc(), "]");
}
} else {
// Just replace the base name.
SmallString<64> baseReplace;

if (!parsed.ContextName.empty()) {
baseReplace += parsed.ContextName;
baseReplace += '.';
}
baseReplace += parsed.BaseName;
if (parsed.IsFunctionName && parsed.ArgumentLabels.empty() &&
isa<VarDecl>(renamedDecl)) {
// If we're going from a var to a function with no arguments, emit an
// empty parameter list.
baseReplace += "()";

if (parsed.IsFunctionName && isa_and_nonnull<SubscriptExpr>(call)) {
auto *SE = cast<SubscriptExpr>(call);

// Renaming from SubscriptExpr to CallExpr. Insert function name and
// replace square brackets with parens.
diag.fixItReplace(SE->getIndex()->getStartLoc(),
("." + baseReplace.str() + "(").str());
diag.fixItReplace(SE->getEndLoc(), ")");
} else {
if (parsed.IsFunctionName && parsed.ArgumentLabels.empty() &&
isa<VarDecl>(renamedDecl)) {
// If we're going from a var to a function with no arguments, emit an
// empty parameter list.
baseReplace += "()";
}
diag.fixItReplace(referenceRange, baseReplace);
}
diag.fixItReplace(referenceRange, baseReplace);
}

if (!call || !isa<CallExpr>(call))
if (!call)
return;

Expr *argExpr;
if (auto *CE = dyn_cast<CallExpr>(call))
argExpr = CE->getArg();
else if (auto *SE = dyn_cast<SubscriptExpr>(call))
argExpr = SE->getIndex();
else
return;

auto *argExpr = call->getArg();
auto argList = getOriginalArgumentList(argExpr);

if (parsed.IsGetter) {
Expand Down Expand Up @@ -2148,7 +2185,8 @@ static void fixItAvailableAttrRename(InFlightDiagnostic &diag,
if (argList.isUnlabeledTrailingClosureIdx(i))
continue;
if (argumentLabelIDs[i] != argList.labels[i]) {
diagnoseArgumentLabelError(ctx, argExpr, argumentLabelIDs, false, &diag);
diagnoseArgumentLabelError(ctx, argExpr, argumentLabelIDs,
parsed.IsSubscript, &diag);
return;
}
}
Expand Down Expand Up @@ -2191,7 +2229,7 @@ describeRename(ASTContext &ctx, const AvailableAttr *attr, const ValueDecl *D,
name << parsed.ContextName << '.';

if (parsed.IsFunctionName) {
name << parsed.formDeclName(ctx);
name << parsed.formDeclName(ctx, (D && isa<SubscriptDecl>(D)));
} else {
name << parsed.BaseName;
}
Expand All @@ -2208,7 +2246,7 @@ describeRename(ASTContext &ctx, const AvailableAttr *attr, const ValueDecl *D,
void TypeChecker::diagnoseIfDeprecated(SourceRange ReferenceRange,
const ExportContext &Where,
const ValueDecl *DeprecatedDecl,
const ApplyExpr *Call) {
const Expr *Call) {
const AvailableAttr *Attr = TypeChecker::getDeprecated(DeprecatedDecl);
if (!Attr)
return;
Expand Down Expand Up @@ -2392,10 +2430,9 @@ void swift::diagnoseUnavailableOverride(ValueDecl *override,

/// Emit a diagnostic for references to declarations that have been
/// marked as unavailable, either through "unavailable" or "obsoleted:".
bool swift::diagnoseExplicitUnavailability(const ValueDecl *D,
SourceRange R,
bool swift::diagnoseExplicitUnavailability(const ValueDecl *D, SourceRange R,
const ExportContext &Where,
const ApplyExpr *call,
const Expr *call,
DeclAvailabilityFlags Flags) {
return diagnoseExplicitUnavailability(D, R, Where, Flags,
[=](InFlightDiagnostic &diag) {
Expand Down Expand Up @@ -2762,7 +2799,7 @@ class ExprAvailabilityWalker : public ASTWalker {
diagnoseDeclRefAvailability(DS->getMember(), DS->getSourceRange());
if (auto S = dyn_cast<SubscriptExpr>(E)) {
if (S->hasDecl()) {
diagnoseDeclRefAvailability(S->getDecl(), S->getSourceRange());
diagnoseDeclRefAvailability(S->getDecl(), S->getSourceRange(), S);
maybeDiagStorageAccess(S->getDecl().getDecl(), S->getSourceRange(), DC);
}
}
Expand Down Expand Up @@ -2820,7 +2857,7 @@ class ExprAvailabilityWalker : public ASTWalker {
}

bool diagnoseDeclRefAvailability(ConcreteDeclRef declRef, SourceRange R,
const ApplyExpr *call = nullptr,
const Expr *call = nullptr,
DeclAvailabilityFlags flags = None) const;

private:
Expand Down Expand Up @@ -3003,9 +3040,8 @@ class ExprAvailabilityWalker : public ASTWalker {

/// Diagnose uses of unavailable declarations. Returns true if a diagnostic
/// was emitted.
bool
ExprAvailabilityWalker::diagnoseDeclRefAvailability(
ConcreteDeclRef declRef, SourceRange R, const ApplyExpr *call,
bool ExprAvailabilityWalker::diagnoseDeclRefAvailability(
ConcreteDeclRef declRef, SourceRange R, const Expr *call,
DeclAvailabilityFlags Flags) const {
if (!declRef)
return false;
Expand All @@ -3014,7 +3050,8 @@ ExprAvailabilityWalker::diagnoseDeclRefAvailability(
if (auto *attr = AvailableAttr::isUnavailable(D)) {
if (diagnoseIncDecRemoval(D, R, attr))
return true;
if (call && diagnoseMemoryLayoutMigration(D, R, attr, call))
if (isa_and_nonnull<ApplyExpr>(call) &&
diagnoseMemoryLayoutMigration(D, R, attr, cast<ApplyExpr>(call)))
return true;
}

Expand All @@ -3032,12 +3069,10 @@ ExprAvailabilityWalker::diagnoseDeclRefAvailability(

/// Diagnose uses of unavailable declarations. Returns true if a diagnostic
/// was emitted.
bool
swift::diagnoseDeclAvailability(const ValueDecl *D,
SourceRange R,
const ApplyExpr *call,
const ExportContext &Where,
DeclAvailabilityFlags Flags) {
bool swift::diagnoseDeclAvailability(const ValueDecl *D, SourceRange R,
const Expr *call,
const ExportContext &Where,
DeclAvailabilityFlags Flags) {
assert(!Where.isImplicit());

// Generic parameters are always available.
Expand Down Expand Up @@ -3097,7 +3132,6 @@ swift::diagnoseDeclAvailability(const ValueDecl *D,
return false;
}


/// Return true if the specified type looks like an integer of floating point
/// type.
static bool isIntegerOrFloatingPointType(Type ty, ModuleDecl *M) {
Expand Down
11 changes: 4 additions & 7 deletions lib/Sema/TypeCheckAvailability.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,8 @@ diagnoseSubstitutionMapAvailability(SourceLoc loc,

/// Diagnose uses of unavailable declarations. Returns true if a diagnostic
/// was emitted.
bool diagnoseDeclAvailability(const ValueDecl *D,
SourceRange R,
const ApplyExpr *call,
const ExportContext &where,
bool diagnoseDeclAvailability(const ValueDecl *D, SourceRange R,
const Expr *call, const ExportContext &where,
DeclAvailabilityFlags flags = None);

void diagnoseUnavailableOverride(ValueDecl *override,
Expand All @@ -242,10 +240,9 @@ void diagnoseUnavailableOverride(ValueDecl *override,

/// Emit a diagnostic for references to declarations that have been
/// marked as unavailable, either through "unavailable" or "obsoleted:".
bool diagnoseExplicitUnavailability(const ValueDecl *D,
SourceRange R,
bool diagnoseExplicitUnavailability(const ValueDecl *D, SourceRange R,
const ExportContext &Where,
const ApplyExpr *call,
const Expr *call,
DeclAvailabilityFlags Flags = None);

/// Emit a diagnostic for references to declarations that have been
Expand Down
6 changes: 2 additions & 4 deletions lib/Sema/TypeChecker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1042,10 +1042,8 @@ void diagnosePotentialAccessorUnavailability(
const AvailableAttr *getDeprecated(const Decl *D);

/// Emits a diagnostic for a reference to a declaration that is deprecated.
void diagnoseIfDeprecated(SourceRange SourceRange,
const ExportContext &Where,
const ValueDecl *DeprecatedDecl,
const ApplyExpr *Call);
void diagnoseIfDeprecated(SourceRange SourceRange, const ExportContext &Where,
const ValueDecl *DeprecatedDecl, const Expr *Call);

/// Emits a diagnostic for a reference to a conformnace that is deprecated.
bool diagnoseIfDeprecated(SourceLoc loc,
Expand Down
52 changes: 52 additions & 0 deletions test/attr/attr_availability.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,3 +1179,55 @@ func testMultipleTrailingClosures(_ x: TypeWithTrailingClosures) {
// expected-note@-1 {{use 'TypeWithTrailingClosures.variadicTrailingClosures(a:b:c:)' instead}} {{3-27=x.variadicTrailingClosures}} {{28-29=}} {{none}}
x.variadicTrailingClosures() {} _: {} _: {}
}

struct UnavailableSubscripts {
@available(*, unavailable, renamed: "subscript(new:)")
subscript(old index: Int) -> Int { 3 } // expected-note * {{'subscript(old:)' has been explicitly marked unavailable here}}
@available(*, unavailable, renamed: "subscript(new:)")
func getValue(old: Int) -> Int { 3 } // expected-note * {{'getValue(old:)' has been explicitly marked unavailable here}}

subscript(new index: Int) -> Int { 3 }

@available(*, unavailable, renamed: "getAValue(new:)")
subscript(getAValue index: Int) -> Int { 3 } // expected-note * {{'subscript(getAValue:)' has been explicitly marked unavailable here}}
func getAValue(new: Int) -> Int { 3 }

@available(*, unavailable, renamed: "subscript(arg1:arg2:arg3:)")
subscript(_ argg1: Int, _ argg2: Int, _ argg3: Int) -> Int { 3 } // expected-note * {{'subscript(_:_:_:)' has been explicitly marked unavailable here}}

@available(*, deprecated, renamed: "subscript(arg1:arg2:arg3:)")
subscript(argg1 argg1: Int, argg2 argg2: Int, argg3 argg3: Int) -> Int { 3 }

@available(*, deprecated, renamed: "subscript(arg1:arg2:arg3:)")
subscript(only1 only1: Int, only2 only2: Int) -> Int { 3 }

subscript(arg1 arg1: Int, arg2 arg2: Int, arg3 arg3: Int) -> Int { 3 }

@available(*, deprecated, renamed: "subscriptTo(_:)")
subscript(to to: Int) -> Int { 3 }
func subscriptTo(_ index: Int) -> Int { 3 }

func testUnavailableSubscripts(_ x: UnavailableSubscripts) {
_ = self[old: 3] // expected-error {{'subscript(old:)' has been renamed to 'subscript(new:)'}} {{14-17=new}}
_ = x[old: 3] // expected-error {{'subscript(old:)' has been renamed to 'subscript(new:)'}} {{11-14=new}}
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 please add a couple of examples where subscript is renamed into a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a test case for subscript→function few lines below on 1213-1214 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I completely missed [ fix-it for that case...


_ = self.getValue(old: 3) // expected-error {{'getValue(old:)' has been renamed to 'subscript(new:)'}} {{14-22=[}} {{29-30=]}} {{23-26=new}}
_ = getValue(old: 3) // expected-error {{'getValue(old:)' has been renamed to 'subscript(new:)'}} {{9-9=self}} {{9-17=[}} {{24-25=]}} {{18-21=new}}
_ = x.getValue(old: 3) // expected-error {{'getValue(old:)' has been renamed to 'subscript(new:)'}} {{11-19=[}} {{26-27=]}} {{20-23=new}}

_ = self[getAValue: 3] // expected-error {{'subscript(getAValue:)' has been renamed to 'getAValue(new:)'}} {{13-14=.getAValue(}} {{26-27=)}} {{14-23=new}}
_ = x[getAValue: 3] // expected-error {{'subscript(getAValue:)' has been renamed to 'getAValue(new:)'}} {{10-11=.getAValue(}} {{23-24=)}} {{11-20=new}}

_ = self[argg1: 3, argg2: 3, argg3: 3] // expected-warning {{'subscript(argg1:argg2:argg3:)' is deprecated: renamed to 'subscript(arg1:arg2:arg3:)'}} // expected-note {{use 'subscript(arg1:arg2:arg3:)' instead}} {{14-19=arg1}} {{24-29=arg2}} {{34-39=arg3}}
_ = x[argg1: 3, argg2: 3, argg3: 3] // expected-warning {{'subscript(argg1:argg2:argg3:)' is deprecated: renamed to 'subscript(arg1:arg2:arg3:)'}} // expected-note {{use 'subscript(arg1:arg2:arg3:)' instead}} {{11-16=arg1}} {{21-26=arg2}} {{31-36=arg3}}

// Different number of parameters emit no fixit
_ = self[only1: 3, only2: 3] // expected-warning {{'subscript(only1:only2:)' is deprecated: renamed to 'subscript(arg1:arg2:arg3:)'}} // expected-note {{use 'subscript(arg1:arg2:arg3:)' instead}} {{none}}

_ = self[3, 3, 3] // expected-error {{'subscript(_:_:_:)' has been renamed to 'subscript(arg1:arg2:arg3:)'}} {{14-14=arg1: }} {{17-17=arg2: }} {{20-20=arg3: }}
_ = x[3, 3, 3] // expected-error {{'subscript(_:_:_:)' has been renamed to 'subscript(arg1:arg2:arg3:)'}} {{11-11=arg1: }} {{14-14=arg2: }} {{17-17=arg3: }}

_ = self[to: 3] // expected-warning {{'subscript(to:)' is deprecated: renamed to 'subscriptTo(_:)'}} // expected-note {{use 'subscriptTo(_:)' instead}} {{13-14=.subscriptTo(}} {{19-20=)}} {{14-18=}}
_ = x[to: 3] // expected-warning {{'subscript(to:)' is deprecated: renamed to 'subscriptTo(_:)'}} // expected-note {{use 'subscriptTo(_:)' instead}} {{10-11=.subscriptTo(}} {{16-17=)}} {{11-15=}}
}
}