Merge pull request #17729 from DougGregor/minimize-override-checking
[Type checker] Minimize checking needed to compute the set of overridden declarations
diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp
index b7e16eb..58b2af2 100644
--- a/lib/Sema/CodeSynthesis.cpp
+++ b/lib/Sema/CodeSynthesis.cpp
@@ -2412,6 +2412,10 @@
ctor, {classDecl, superclassCtor}, ctx);
}
+ // Wire up the overrides.
+ ctor->getAttrs().add(new (ctx) OverrideAttr(/*IsImplicit=*/true));
+ ctor->setOverriddenDecl(superclassCtor);
+
if (superclassCtor->isObjC()) {
// Inherit the @objc name from the superclass initializer, if it
// has one.
@@ -2430,10 +2434,6 @@
ctor->getAttrs().add(new (ctx) RequiredAttr(/*IsImplicit=*/true));
if (superclassCtor->isDynamic())
ctor->getAttrs().add(new (ctx) DynamicAttr(/*IsImplicit*/true));
-
- // Wire up the overrides.
- ctor->getAttrs().add(new (ctx) OverrideAttr(/*IsImplicit=*/true));
- ctor->setOverriddenDecl(superclassCtor);
}
ConstructorDecl *
diff --git a/lib/Sema/TypeCheckDecl.cpp b/lib/Sema/TypeCheckDecl.cpp
index 57facce..df67867 100644
--- a/lib/Sema/TypeCheckDecl.cpp
+++ b/lib/Sema/TypeCheckDecl.cpp
@@ -1393,12 +1393,17 @@
(D->getOverriddenDecl() &&
D->getOverriddenDecl()->hasClangNode());
+ bool overridesDyanmic =
+ (D->getOverriddenDecl() &&
+ D->getOverriddenDecl()->isDynamic());
+
bool isNSManaged = D->getAttrs().hasAttribute<NSManagedAttr>();
bool isExtension = isa<ExtensionDecl>(D->getDeclContext());
// We only infer 'dynamic' in these three cases.
- if (!isExtension && !isNSManaged && !overridesImportedMethod)
+ if (!isExtension && !isNSManaged && !overridesImportedMethod &&
+ !overridesDyanmic)
return;
// The presence of 'final' blocks the inference of 'dynamic'.
@@ -2904,6 +2909,19 @@
}
}
+ if (!checkOverrides(TC, VD)) {
+ // If a property has an override attribute but does not override
+ // anything, complain.
+ auto overridden = VD->getOverriddenDecl();
+ if (auto *OA = VD->getAttrs().getAttribute<OverrideAttr>()) {
+ if (!overridden) {
+ TC.diagnose(VD, diag::property_does_not_override)
+ .highlight(OA->getLocation());
+ OA->setInvalid();
+ }
+ }
+ }
+
TC.checkDeclAttributes(VD);
triggerAccessorSynthesis(TC, VD);
@@ -3049,6 +3067,18 @@
AccessControlChecker::checkAccessControl(TC, SD);
UsableFromInlineChecker::checkUsableFromInline(TC, SD);
+ if (!checkOverrides(TC, SD)) {
+ // If a subscript has an override attribute but does not override
+ // anything, complain.
+ if (auto *OA = SD->getAttrs().getAttribute<OverrideAttr>()) {
+ if (!SD->getOverriddenDecl()) {
+ TC.diagnose(SD, diag::subscript_does_not_override)
+ .highlight(OA->getLocation());
+ OA->setInvalid();
+ }
+ }
+ }
+
triggerAccessorSynthesis(TC, SD);
}
@@ -3539,6 +3569,18 @@
AccessControlChecker::checkAccessControl(TC, FD);
UsableFromInlineChecker::checkUsableFromInline(TC, FD);
+ if (!checkOverrides(TC, FD)) {
+ // If a method has an 'override' keyword but does not
+ // override anything, complain.
+ if (auto *OA = FD->getAttrs().getAttribute<OverrideAttr>()) {
+ if (!FD->getOverriddenDecl()) {
+ TC.diagnose(FD, diag::method_does_not_override)
+ .highlight(OA->getLocation());
+ OA->setInvalid();
+ }
+ }
+ }
+
if (FD->hasBody()) {
// Record the body.
TC.definedFunctions.push_back(FD);
@@ -3668,6 +3710,61 @@
void visitConstructorDecl(ConstructorDecl *CD) {
TC.validateDecl(CD);
+ // Check whether this initializer overrides an initializer in its
+ // superclass.
+ if (!checkOverrides(TC, CD)) {
+ // If an initializer has an override attribute but does not override
+ // anything or overrides something that doesn't need an 'override'
+ // keyword (e.g., a convenience initializer), complain.
+ // anything, or overrides something that complain.
+ if (auto *attr = CD->getAttrs().getAttribute<OverrideAttr>()) {
+ if (!CD->getOverriddenDecl()) {
+ TC.diagnose(CD, diag::initializer_does_not_override)
+ .highlight(attr->getLocation());
+ attr->setInvalid();
+ } else if (attr->isImplicit()) {
+ // Don't diagnose implicit attributes.
+ } else if (!overrideRequiresKeyword(CD->getOverriddenDecl())) {
+ // Special case: we are overriding a 'required' initializer, so we
+ // need (only) the 'required' keyword.
+ if (cast<ConstructorDecl>(CD->getOverriddenDecl())->isRequired()) {
+ if (CD->getAttrs().hasAttribute<RequiredAttr>()) {
+ TC.diagnose(CD, diag::required_initializer_override_keyword)
+ .fixItRemove(attr->getLocation());
+ } else {
+ TC.diagnose(CD, diag::required_initializer_override_wrong_keyword)
+ .fixItReplace(attr->getLocation(), "required");
+ CD->getAttrs().add(
+ new (TC.Context) RequiredAttr(/*IsImplicit=*/true));
+ }
+
+ TC.diagnose(findNonImplicitRequiredInit(CD->getOverriddenDecl()),
+ diag::overridden_required_initializer_here);
+ } else {
+ // We tried to override a convenience initializer.
+ TC.diagnose(CD, diag::initializer_does_not_override)
+ .highlight(attr->getLocation());
+ TC.diagnose(CD->getOverriddenDecl(),
+ diag::convenience_init_override_here);
+ }
+ }
+ }
+
+ // A failable initializer cannot override a non-failable one.
+ // This would normally be diagnosed by the covariance rules;
+ // however, those are disabled so that we can provide a more
+ // specific diagnostic here.
+ if (CD->getFailability() != OTK_None &&
+ CD->getOverriddenDecl() &&
+ CD->getOverriddenDecl()->getFailability() == OTK_None) {
+ TC.diagnose(CD, diag::failable_initializer_override,
+ CD->getFullName());
+ TC.diagnose(CD->getOverriddenDecl(),
+ diag::nonfailable_initializer_override_here,
+ CD->getOverriddenDecl()->getFullName());
+ }
+ }
+
// If this initializer overrides a 'required' initializer, it must itself
// be marked 'required'.
if (!CD->getAttrs().hasAttribute<RequiredAttr>()) {
@@ -4440,19 +4537,6 @@
checkDeclAttributesEarly(VD);
validateAttributes(*this, VD);
- if (!checkOverrides(*this, VD)) {
- // If a property has an override attribute but does not override
- // anything, complain.
- auto overridden = VD->getOverriddenDecl();
- if (auto *OA = VD->getAttrs().getAttribute<OverrideAttr>()) {
- if (!overridden) {
- diagnose(VD, diag::property_does_not_override)
- .highlight(OA->getLocation());
- OA->setInvalid();
- }
- }
- }
-
// Properties need some special validation logic.
if (auto *nominalDecl = VD->getDeclContext()
->getAsNominalTypeOrNominalTypeExtensionContext()) {
@@ -4731,18 +4815,6 @@
// Member functions need some special validation logic.
if (FD->getDeclContext()->isTypeContext()) {
- if (!checkOverrides(*this, FD)) {
- // If a method has an 'override' keyword but does not
- // override anything, complain.
- if (auto *OA = FD->getAttrs().getAttribute<OverrideAttr>()) {
- if (!FD->getOverriddenDecl()) {
- diagnose(FD, diag::method_does_not_override)
- .highlight(OA->getLocation());
- OA->setInvalid();
- }
- }
- }
-
if (FD->isOperator())
checkMemberOperator(*this, FD);
@@ -4929,59 +5001,6 @@
validateAttributes(*this, CD);
- // Check whether this initializer overrides an initializer in its
- // superclass.
- if (!checkOverrides(*this, CD)) {
- // If an initializer has an override attribute but does not override
- // anything or overrides something that doesn't need an 'override'
- // keyword (e.g., a convenience initializer), complain.
- // anything, or overrides something that complain.
- if (auto *attr = CD->getAttrs().getAttribute<OverrideAttr>()) {
- if (!CD->getOverriddenDecl()) {
- diagnose(CD, diag::initializer_does_not_override)
- .highlight(attr->getLocation());
- attr->setInvalid();
- } else if (!overrideRequiresKeyword(CD->getOverriddenDecl())) {
- // Special case: we are overriding a 'required' initializer, so we
- // need (only) the 'required' keyword.
- if (cast<ConstructorDecl>(CD->getOverriddenDecl())->isRequired()) {
- if (CD->getAttrs().hasAttribute<RequiredAttr>()) {
- diagnose(CD, diag::required_initializer_override_keyword)
- .fixItRemove(attr->getLocation());
- } else {
- diagnose(CD, diag::required_initializer_override_wrong_keyword)
- .fixItReplace(attr->getLocation(), "required");
- CD->getAttrs().add(
- new (Context) RequiredAttr(/*IsImplicit=*/true));
- }
-
- diagnose(findNonImplicitRequiredInit(CD->getOverriddenDecl()),
- diag::overridden_required_initializer_here);
- } else {
- // We tried to override a convenience initializer.
- diagnose(CD, diag::initializer_does_not_override)
- .highlight(attr->getLocation());
- diagnose(CD->getOverriddenDecl(),
- diag::convenience_init_override_here);
- }
- }
- }
-
- // A failable initializer cannot override a non-failable one.
- // This would normally be diagnosed by the covariance rules;
- // however, those are disabled so that we can provide a more
- // specific diagnostic here.
- if (CD->getFailability() != OTK_None &&
- CD->getOverriddenDecl() &&
- CD->getOverriddenDecl()->getFailability() == OTK_None) {
- diagnose(CD, diag::failable_initializer_override,
- CD->getFullName());
- diagnose(CD->getOverriddenDecl(),
- diag::nonfailable_initializer_override_here,
- CD->getOverriddenDecl()->getFullName());
- }
- }
-
// An initializer is ObjC-compatible if it's explicitly @objc or a member
// of an ObjC-compatible class.
if (CD->getDeclContext()->isTypeContext()) {
@@ -5123,18 +5142,6 @@
new (C) ImplicitlyUnwrappedOptionalAttr(/* implicit= */ true));
}
- if (!checkOverrides(*this, SD)) {
- // If a subscript has an override attribute but does not override
- // anything, complain.
- if (auto *OA = SD->getAttrs().getAttribute<OverrideAttr>()) {
- if (!SD->getOverriddenDecl()) {
- diagnose(SD, diag::subscript_does_not_override)
- .highlight(OA->getLocation());
- OA->setInvalid();
- }
- }
- }
-
// Member subscripts need some special validation logic.
if (dc->isTypeContext()) {
// If this is a class member, mark it final if the class is final.
diff --git a/lib/Sema/TypeCheckDeclOverride.cpp b/lib/Sema/TypeCheckDeclOverride.cpp
index d571e8c..7cf2352 100644
--- a/lib/Sema/TypeCheckDeclOverride.cpp
+++ b/lib/Sema/TypeCheckDeclOverride.cpp
@@ -72,12 +72,24 @@
assert((method || abstractStorage) && "Not a method or abstractStorage?");
SubscriptDecl *subscript = dyn_cast_or_null<SubscriptDecl>(abstractStorage);
+ if (!member->hasInterfaceType()) {
+ auto lazyResolver = ctx.getLazyResolver();
+ assert(lazyResolver && "Need to resolve interface type");
+ lazyResolver->resolveDeclSignature(member);
+ }
+
auto memberType = member->getInterfaceType();
if (derivedDecl) {
auto *dc = derivedDecl->getDeclContext();
auto owningType = dc->getDeclaredInterfaceType();
assert(owningType);
+ if (!derivedDecl->hasInterfaceType()) {
+ auto lazyResolver = ctx.getLazyResolver();
+ assert(lazyResolver && "Need to resolve interface type");
+ lazyResolver->resolveDeclSignature(derivedDecl);
+ }
+
memberType = owningType->adjustSuperclassMemberDeclType(member, derivedDecl,
memberType);
if (memberType->hasError())
@@ -956,8 +968,21 @@
///
/// \returns true if an error occurred.
bool swift::checkOverrides(TypeChecker &TC, ValueDecl *decl) {
- if (decl->getOverriddenDecl())
- return false;
+ // If we already computed overridden declarations and either succeeded
+ // or invalidated the attribute, there's nothing more to do.
+ if (decl->overriddenDeclsComputed()) {
+ // If we computed an overridden declaration successfully, we're done.
+ if (decl->getOverriddenDecl())
+ return false;
+
+ // If we set the override attribute to "invalid", we already diagnosed
+ // something here.
+ if (decl->getAttrs().hasAttribute<OverrideAttr>(/*AllowInvalid=*/true) &&
+ !decl->getAttrs().hasAttribute<OverrideAttr>())
+ return true;
+
+ // Otherwise, we have more checking to do.
+ }
// Set up matching, but bail out if there's nothing to match.
OverrideMatcher matcher(TC, decl);
@@ -1168,6 +1193,10 @@
if (Override->isFinal())
return;
+ // Must be @objc to be 'dynamic'.
+ if (!Override->isObjC())
+ return;
+
makeDynamic(Override->getASTContext(), Override);
}
@@ -1462,13 +1491,6 @@
diagnoseOverrideForAvailability(override, base);
}
- /// Check attributes associated with the base; some may need to merged with
- /// or checked against attributes in the overriding declaration.
- AttributeOverrideChecker attrChecker(base, override);
- for (auto attr : base->getAttrs()) {
- attrChecker.visit(attr);
- }
-
if (auto overridingFunc = dyn_cast<FuncDecl>(override)) {
overridingFunc->setOverriddenDecl(cast<FuncDecl>(base));
} else if (auto overridingCtor = dyn_cast<ConstructorDecl>(override)) {
@@ -1476,8 +1498,20 @@
} else if (auto overridingASD = dyn_cast<AbstractStorageDecl>(override)) {
auto *baseASD = cast<AbstractStorageDecl>(base);
overridingASD->setOverriddenDecl(baseASD);
+ } else {
+ llvm_unreachable("Unexpected decl");
+ }
+ /// Check attributes associated with the base; some may need to merged with
+ /// or checked against attributes in the overriding declaration.
+ AttributeOverrideChecker attrChecker(base, override);
+ for (auto attr : base->getAttrs()) {
+ attrChecker.visit(attr);
+ }
+
+ if (auto overridingASD = dyn_cast<AbstractStorageDecl>(override)) {
// Make sure we get consistent overrides for the accessors as well.
+ auto *baseASD = cast<AbstractStorageDecl>(base);
assert(baseASD->getGetter());
auto recordAccessorOverride = [&](AccessorKind kind) {
@@ -1515,8 +1549,6 @@
recordAccessorOverride(AccessorKind::Get);
recordAccessorOverride(AccessorKind::Set);
recordAccessorOverride(AccessorKind::MaterializeForSet);
- } else {
- llvm_unreachable("Unexpected decl");
}
return false;
@@ -1647,6 +1679,41 @@
if (!isa<ConstructorDecl>(VD) && !VD->getAttrs().hasAttribute<OverrideAttr>())
return;
- // FIXME: We should perform more minimal validation.
- validateDeclForNameLookup(VD);
+ // Invalidate an existing "override" attribute or add a new invalid "override"
+ // attribute, which will suppress additional checking.
+ auto invalidateOverrideAttribute = [VD]() {
+ auto overrideAttr = VD->getAttrs().getAttribute<OverrideAttr>(true);
+ if (!overrideAttr) {
+ overrideAttr = new (VD->getASTContext()) OverrideAttr(true);
+ VD->getAttrs().add(overrideAttr);
+ }
+
+ overrideAttr->setInvalid();
+ };
+
+ // Try to match potential overridden declarations.
+ OverrideMatcher matcher(*this, VD);
+ if (!matcher) {
+ return;
+ }
+
+ auto matches = matcher.match(OverrideCheckingAttempt::PerfectMatch);
+ if (matches.empty()) {
+ return;
+ }
+
+ // If we have more than one potential match, diagnose the ambiguity and
+ // fail.
+ if (matches.size() > 1) {
+ diagnoseGeneralOverrideFailure(VD, matches,
+ OverrideCheckingAttempt::PerfectMatch);
+ invalidateOverrideAttribute();
+ return;
+ }
+
+ // Check the correctness of the override.
+ // FIXME: This also records the override.
+ if (matcher.checkOverride(matches.front().Decl,
+ OverrideCheckingAttempt::PerfectMatch))
+ invalidateOverrideAttribute();
}
diff --git a/test/ClangImporter/objc_override.swift b/test/ClangImporter/objc_override.swift
index bf8bdd7..f27995f 100644
--- a/test/ClangImporter/objc_override.swift
+++ b/test/ClangImporter/objc_override.swift
@@ -19,7 +19,8 @@
@objc func initWithString(_ string: String) { }
var isEnabled: Bool { // expected-error{{overriding declaration requires an 'override' keyword}}
- get { return super.isEnabled }
+ // FIXE: The error that follows is a bit strange.
+ get { return super.isEnabled } // expected-error{{'@objc' getter for non-'@objc' property}}
}
@objc(enabled)
diff --git a/test/SourceKit/CursorInfo/cursor_overrides.swift b/test/SourceKit/CursorInfo/cursor_overrides.swift
index 1e6415a..666dd7d 100644
--- a/test/SourceKit/CursorInfo/cursor_overrides.swift
+++ b/test/SourceKit/CursorInfo/cursor_overrides.swift
@@ -5,11 +5,11 @@
}
class Cls : S1, Prot {
- func meth() {}
+ override func meth() {}
}
class SubCls : Cls {
- func meth() {}
+ override func meth() {}
}
func goo(x: SubCls) {
@@ -27,7 +27,7 @@
// REQUIRES: objc_interop
// RUN: %sourcekitd-test -req=cursor -pos=16:7 %s -- -embed-bitcode -I %S/Inputs/cursor-overrides %mcp_opt %s | %FileCheck -check-prefix=CHECK1 %s
-// CHECK1: source.lang.swift.ref.function.method.instance (12:8-12:14)
+// CHECK1: source.lang.swift.ref.function.method.instance (12:17-12:23)
// CHECK1: c:@M@cursor_overrides@objc(cs)SubCls(im)meth
// CHECK1: (SubCls) -> () -> ()
// CHECK1: OVERRIDES BEGIN
diff --git a/test/multifile/Inputs/overrideB.swift b/test/multifile/Inputs/overrideB.swift
new file mode 100644
index 0000000..88e6191
--- /dev/null
+++ b/test/multifile/Inputs/overrideB.swift
@@ -0,0 +1,5 @@
+class Super {
+ func bar<T>(_: Into<T>) { }
+
+ func wibble() { }
+}
diff --git a/test/multifile/Inputs/overrideC.swift b/test/multifile/Inputs/overrideC.swift
new file mode 100644
index 0000000..59d7e0c
--- /dev/null
+++ b/test/multifile/Inputs/overrideC.swift
@@ -0,0 +1,5 @@
+class Sub: Super {
+ override func bar<T>(_: Into<T>) { }
+
+ func wibble() { }
+}
diff --git a/test/multifile/override.swift b/test/multifile/override.swift
new file mode 100644
index 0000000..831170a
--- /dev/null
+++ b/test/multifile/override.swift
@@ -0,0 +1,22 @@
+// Test that overrides in other source files get checked lazily.
+
+// Make sure the overrides are resolved... but we don't diagnose a missing
+// 'override' keyword from another source file.
+// RUN: %target-swift-frontend -typecheck -primary-file %s %S/Inputs/overrideB.swift %S/Inputs/overrideC.swift -verify
+
+// Make sure we still diagnose the missing 'override' when looking at the
+// source file where it occurs.
+// RUN: not %target-swift-frontend -typecheck %s %S/Inputs/overrideB.swift -primary-file %S/Inputs/overrideC.swift 2> %t.err
+// RUN: %FileCheck %s < %t.err
+
+// expected-no-diagnostics
+
+class Foo {
+ func foo(sub: Sub) {
+ sub.bar(Into<Int>())
+ }
+}
+
+struct Into<T> { }
+
+// CHECK: overriding declaration requires an 'override' keyword
diff --git a/validation-test/compiler_crashers/28780-hasinterfacetype-no-interface-type-was-set.swift b/validation-test/compiler_crashers_fixed/28780-hasinterfacetype-no-interface-type-was-set.swift
similarity index 83%
rename from validation-test/compiler_crashers/28780-hasinterfacetype-no-interface-type-was-set.swift
rename to validation-test/compiler_crashers_fixed/28780-hasinterfacetype-no-interface-type-was-set.swift
index 3cd2147..075332f 100644
--- a/validation-test/compiler_crashers/28780-hasinterfacetype-no-interface-type-was-set.swift
+++ b/validation-test/compiler_crashers_fixed/28780-hasinterfacetype-no-interface-type-was-set.swift
@@ -5,6 +5,6 @@
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
-// REQUIRES: asserts
-// RUN: not --crash %target-swift-frontend %s -emit-ir
+
+// RUN: not %target-swift-frontend %s -emit-ir
class a{func a:A.a:class A:a{{}func a
diff --git a/validation-test/compiler_crashers/28841-swift-valuedecl-getformalaccessscope-swift-declcontext-const-bool-const.swift b/validation-test/compiler_crashers_fixed/28841-swift-valuedecl-getformalaccessscope-swift-declcontext-const-bool-const.swift
similarity index 87%
rename from validation-test/compiler_crashers/28841-swift-valuedecl-getformalaccessscope-swift-declcontext-const-bool-const.swift
rename to validation-test/compiler_crashers_fixed/28841-swift-valuedecl-getformalaccessscope-swift-declcontext-const-bool-const.swift
index 5aa4c9f..838601b 100644
--- a/validation-test/compiler_crashers/28841-swift-valuedecl-getformalaccessscope-swift-declcontext-const-bool-const.swift
+++ b/validation-test/compiler_crashers_fixed/28841-swift-valuedecl-getformalaccessscope-swift-declcontext-const-bool-const.swift
@@ -5,5 +5,5 @@
// See https://swift.org/LICENSE.txt for license information
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
-// RUN: not --crash %target-swift-frontend %s -emit-ir
+// RUN: not %target-swift-frontend %s -emit-ir
extension a{var f=a}class a:a
