Skip to content

Sema: Diagnose unbound method references on 'super.' #30097

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
Feb 28, 2020
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
6 changes: 4 additions & 2 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -3285,14 +3285,16 @@ ERROR(partial_application_of_function_invalid,none,
"partial application of %select{"
"'mutating' method|"
"'super.init' initializer chain|"
"'self.init' initializer delegation"
"'self.init' initializer delegation|"
"'super' instance method with metatype base"
"}0 is not allowed",
(unsigned))
WARNING(partial_application_of_function_invalid_swift4,none,
"partial application of %select{"
"'mutating' method|"
"'super.init' initializer chain|"
"'self.init' initializer delegation"
"'self.init' initializer delegation|"
"'super' instance method with metatype base"
"}0 is not allowed; calling the function has undefined behavior and will "
"be an error in future Swift versions",
(unsigned))
Expand Down
2 changes: 2 additions & 0 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3756,6 +3756,8 @@ bool PartialApplicationFailure::diagnoseAsError() {
anchor, ConstraintLocator::ConstructorMember))) {
kind = anchor->getBase()->isSuperExpr() ? RefKind::SuperInit
: RefKind::SelfInit;
} else if (anchor->getBase()->isSuperExpr()) {
kind = RefKind::SuperMethod;
}

auto diagnostic = CompatibilityWarning
Expand Down
1 change: 1 addition & 0 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -1091,6 +1091,7 @@ class PartialApplicationFailure final : public FailureDiagnostic {
MutatingMethod,
SuperInit,
SelfInit,
SuperMethod,
};

bool CompatibilityWarning;
Expand Down
174 changes: 88 additions & 86 deletions lib/Sema/ConstraintSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1812,71 +1812,71 @@ static std::pair<Type, Type> getTypeOfReferenceWithSpecialTypeCheckingSemantics(
llvm_unreachable("Unhandled DeclTypeCheckingSemantics in switch.");
}

/// \returns true if given declaration is an instance method marked as
/// `mutating`, false otherwise.
bool isMutatingMethod(const ValueDecl *decl) {
if (!(decl->isInstanceMember() && isa<FuncDecl>(decl)))
return false;
return cast<FuncDecl>(decl)->isMutating();
}

static bool shouldCheckForPartialApplication(ConstraintSystem &cs,
const ValueDecl *decl,
ConstraintLocator *locator) {
auto *anchor = locator->getAnchor();
if (!(anchor && isa<UnresolvedDotExpr>(anchor)))
return false;

// If this is a reference to instance method marked as 'mutating'
// it should be checked for invalid partial application.
if (isMutatingMethod(decl))
return true;

// Another unsupported partial application is related
// to constructor delegation via `self.init` or `super.init`.

if (!isa<ConstructorDecl>(decl))
return false;

auto *UDE = cast<UnresolvedDotExpr>(anchor);
// This is `super.init`
if (UDE->getBase()->isSuperExpr())
return true;

// Or this might be `self.init`.
if (auto *DRE = dyn_cast<DeclRefExpr>(UDE->getBase())) {
if (auto *baseDecl = DRE->getDecl())
return baseDecl->getBaseName() == cs.getASTContext().Id_self;
}

return false;
}

/// Try to identify and fix failures related to partial function application
/// e.g. partial application of `init` or 'mutating' instance methods.
static std::pair<bool, unsigned>
isInvalidPartialApplication(ConstraintSystem &cs, const ValueDecl *member,
isInvalidPartialApplication(ConstraintSystem &cs,
const AbstractFunctionDecl *member,
ConstraintLocator *locator) {
if (!shouldCheckForPartialApplication(cs, member, locator))
return {false, 0};
auto *UDE = dyn_cast_or_null<UnresolvedDotExpr>(locator->getAnchor());
if (UDE == nullptr)
return {false,0};

auto anchor = cast<UnresolvedDotExpr>(locator->getAnchor());
// If this choice is a partial application of `init` or
// `mutating` instance method we should report that it's not allowed.
auto baseTy =
cs.simplifyType(cs.getType(anchor->getBase()))->getWithoutSpecifierType();
cs.simplifyType(cs.getType(UDE->getBase()))->getWithoutSpecifierType();

auto isInvalidIfPartiallyApplied = [&]() {
if (auto *FD = dyn_cast<FuncDecl>(member)) {
// 'mutating' instance methods cannot be partially applied.
if (FD->isMutating())
return true;

// Instance methods cannot be referenced on 'super' from a static
// context.
if (UDE->getBase()->isSuperExpr() &&
baseTy->is<MetatypeType>() &&
!FD->isStatic())
return true;
}

// Partial applications are not allowed only for constructor
// delegation, reference on the metatype is considered acceptable.
if (baseTy->is<MetatypeType>() && isa<ConstructorDecl>(member))
return {false, 0};
// Another unsupported partial application is related
// to constructor delegation via 'self.init' or 'super.init'.
//
// Note that you can also write 'self.init' or 'super.init'
// inside a static context -- since 'self' is a metatype there
// it doesn't have the special delegation meaning that it does
// in the body of a constructor.
if (isa<ConstructorDecl>(member) && !baseTy->is<MetatypeType>()) {
// Check for a `super.init` delegation...
if (UDE->getBase()->isSuperExpr())
return true;

// ... and `self.init` delegation. Note that in a static context,
// `self.init` is just an ordinary partial application; it's OK
// because there's no associated instance for delegation.
if (auto *DRE = dyn_cast<DeclRefExpr>(UDE->getBase())) {
if (auto *baseDecl = DRE->getDecl()) {
if (baseDecl->getBaseName() == cs.getASTContext().Id_self)
return true;
}
}
}

return false;
};

if (!isInvalidIfPartiallyApplied())
return {false,0};

// If base is a metatype it would be ignored (unless this is an initializer
// call), but if it is some other type it means that we have a single
// application level already.
unsigned level = baseTy->is<MetatypeType>() ? 0 : 1;
if (auto *call = dyn_cast_or_null<CallExpr>(cs.getParentExpr(anchor))) {
level += dyn_cast_or_null<CallExpr>(cs.getParentExpr(call)) ? 2 : 1;
Copy link
Contributor

@davezarzycki davezarzycki Feb 28, 2020

Choose a reason for hiding this comment

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

Hi @slavapestov – I'm trying to understand why this line was removed. Why do we no longer need to check for the grandparent CallExpr?

unsigned level = 0;
if (!baseTy->is<MetatypeType>())
level++;

if (auto *call = dyn_cast_or_null<CallExpr>(cs.getParentExpr(UDE))) {
level += 1;
}

return {true, level};
Expand Down Expand Up @@ -2357,39 +2357,41 @@ void ConstraintSystem::resolveOverload(ConstraintLocator *locator,
}
}

// Check whether applying this overload would result in invalid
// partial function application e.g. partial application of
// mutating method or initializer.

// This check is supposed to be performed without
// `shouldAttemptFixes` because name lookup can't
// detect that particular partial application is
// invalid, so it has to return all of the candidates.

bool isInvalidPartialApply;
unsigned level;

std::tie(isInvalidPartialApply, level) =
isInvalidPartialApplication(*this, decl, locator);

if (isInvalidPartialApply) {
// No application at all e.g. `Foo.bar`.
if (level == 0) {
// Swift 4 and earlier failed to diagnose a reference to a mutating
// method without any applications at all, which would get
// miscompiled into a function with undefined behavior. Warn for
// source compatibility.
bool isWarning = !getASTContext().isSwiftVersionAtLeast(5);
(void)recordFix(
AllowInvalidPartialApplication::create(isWarning, *this, locator));
} else if (level == 1) {
// `Self` parameter is applied, e.g. `foo.bar` or `Foo.bar(&foo)`
(void)recordFix(AllowInvalidPartialApplication::create(
/*isWarning=*/false, *this, locator));
}
if (auto *afd = dyn_cast<AbstractFunctionDecl>(decl)) {
// Check whether applying this overload would result in invalid
// partial function application e.g. partial application of
// mutating method or initializer.

// This check is supposed to be performed without
// `shouldAttemptFixes` because name lookup can't
// detect that particular partial application is
// invalid, so it has to return all of the candidates.

bool isInvalidPartialApply;
unsigned level;

std::tie(isInvalidPartialApply, level) =
isInvalidPartialApplication(*this, afd, locator);

if (isInvalidPartialApply) {
// No application at all e.g. `Foo.bar`.
if (level == 0) {
// Swift 4 and earlier failed to diagnose a reference to a mutating
// method without any applications at all, which would get
// miscompiled into a function with undefined behavior. Warn for
// source compatibility.
bool isWarning = !getASTContext().isSwiftVersionAtLeast(5);
(void)recordFix(
AllowInvalidPartialApplication::create(isWarning, *this, locator));
} else if (level == 1) {
// `Self` parameter is applied, e.g. `foo.bar` or `Foo.bar(&foo)`
(void)recordFix(AllowInvalidPartialApplication::create(
/*isWarning=*/false, *this, locator));
}

// Otherwise both `Self` and arguments are applied,
// e.g. `foo.bar()` or `Foo.bar(&foo)()`, and there is nothing to do.
// Otherwise both `Self` and arguments are applied,
// e.g. `foo.bar()` or `Foo.bar(&foo)()`, and there is nothing to do.
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/Constraints/mutating_members.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ func bar() -> Foo.Type { fatalError() }

_ = bar().boom // expected-error{{partial application of 'mutating' method}}
_ = bar().boom(&y) // expected-error{{partial application of 'mutating' method}}
_ = bar().boom(&y)() // ok
_ = bar().boom(&y)() // expected-error{{partial application of 'mutating' method}}

func foo(_ foo: Foo.Type) {
_ = foo.boom // expected-error{{partial application of 'mutating' method}}
_ = foo.boom(&y) // expected-error{{partial application of 'mutating' method}}
_ = foo.boom(&y)() // ok
_ = foo.boom(&y)() // expected-error{{partial application of 'mutating' method}}
}
4 changes: 2 additions & 2 deletions test/Constraints/mutating_members_compat.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ func bar() -> Foo.Type { fatalError() }

_ = bar().boom // expected-warning{{partial application of 'mutating' method}}
_ = bar().boom(&y) // expected-error{{partial application of 'mutating' method}}
_ = bar().boom(&y)() // ok
_ = bar().boom(&y)() // expected-error{{partial application of 'mutating' method}}

func foo(_ foo: Foo.Type) {
_ = foo.boom // expected-warning{{partial application of 'mutating' method}}
_ = foo.boom(&y) // expected-error{{partial application of 'mutating' method}}
_ = foo.boom(&y)() // ok
_ = foo.boom(&y)() // expected-error{{partial application of 'mutating' method}}
}
7 changes: 5 additions & 2 deletions test/NameBinding/name_lookup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -292,16 +292,19 @@ class ThisDerived1 : ThisBase1 {
super.baseInstanceVar = 42 // expected-error {{member 'baseInstanceVar' cannot be used on type 'ThisBase1'}}
super.baseProp = 42 // expected-error {{member 'baseProp' cannot be used on type 'ThisBase1'}}
super.baseFunc0() // expected-error {{instance member 'baseFunc0' cannot be used on type 'ThisBase1'}}
super.baseFunc0(ThisBase1())()
// expected-error@-1 {{partial application of 'super' instance method with metatype base is not allowed}}
super.baseFunc0(ThisBase1())() // expected-error {{partial application of 'super' instance method with metatype base is not allowed}}
super.baseFunc1(42) // expected-error {{instance member 'baseFunc1' cannot be used on type 'ThisBase1'}}
super.baseFunc1(ThisBase1())(42)
// expected-error@-1 {{partial application of 'super' instance method with metatype base is not allowed}}
super.baseFunc1(ThisBase1())(42) // expected-error {{partial application of 'super' instance method with metatype base is not allowed}}
super[0] = 42.0 // expected-error {{instance member 'subscript' cannot be used on type 'ThisBase1'}}
super.baseStaticVar = 42
super.baseStaticProp = 42
super.baseStaticFunc0()

super.baseExtProp = 42 // expected-error {{member 'baseExtProp' cannot be used on type 'ThisBase1'}}
super.baseExtFunc0() // expected-error {{instance member 'baseExtFunc0' cannot be used on type 'ThisBase1'}}
// expected-error@-1 {{partial application of 'super' instance method with metatype base is not allowed}}
super.baseExtStaticVar = 42 // expected-error {{instance member 'baseExtStaticVar' cannot be used on type 'ThisBase1'}}
super.baseExtStaticProp = 42 // expected-error {{member 'baseExtStaticProp' cannot be used on type 'ThisBase1'}}
super.baseExtStaticFunc0()
Expand Down