From 304f9c4bf0286c8d4156de13e919be9fd17c6e82 Mon Sep 17 00:00:00 2001 From: Allan Shortlidge Date: Mon, 12 Aug 2024 18:02:04 -0700 Subject: [PATCH] AST: Look through missing imports in OverriddenDeclsRequest. If `OverriddenDeclsRequest` fails to find any overridden declarations, query again ignoring missing imports to find declarations that were excluded due to the `MemberImportVisibility` feature being enabled. --- lib/Sema/TypeCheckDeclOverride.cpp | 167 ++++++++++++++++------- test/NameLookup/members_transitive.swift | 4 +- 2 files changed, 116 insertions(+), 55 deletions(-) diff --git a/lib/Sema/TypeCheckDeclOverride.cpp b/lib/Sema/TypeCheckDeclOverride.cpp index 0cfb6af0c99e4..5e7d6cabad8aa 100644 --- a/lib/Sema/TypeCheckDeclOverride.cpp +++ b/lib/Sema/TypeCheckDeclOverride.cpp @@ -814,8 +814,11 @@ namespace { /// The type of the declaration, cached here once it has been computed. Type cachedDeclType; + /// Whether to ignore missing imports when looking for overridden methods. + bool ignoreMissingImports; + public: - OverrideMatcher(ValueDecl *decl); + OverrideMatcher(ValueDecl *decl, bool ignoreMissingImports); /// Returns true when it's possible to perform any override matching. explicit operator bool() const { @@ -860,8 +863,9 @@ namespace { }; } -OverrideMatcher::OverrideMatcher(ValueDecl *decl) - : ctx(decl->getASTContext()), decl(decl) { +OverrideMatcher::OverrideMatcher(ValueDecl *decl, bool ignoreMissingImports) + : ctx(decl->getASTContext()), decl(decl), + ignoreMissingImports(ignoreMissingImports) { // The final step for this constructor is to set up the superclass type, // without which we will not perform an matching. Early exits therefore imply // that there is no way we can match this declaration. @@ -916,8 +920,12 @@ SmallVector OverrideMatcher::match( for (auto *ctx : superContexts) { ctx->synthesizeSemanticMembersIfNeeded(membersName); } - dc->lookupQualified(superContexts, DeclNameRef(membersName), - decl->getLoc(), NL_QualifiedDefault, members); + auto lookupOptions = NL_QualifiedDefault; + if (ignoreMissingImports) + lookupOptions |= NL_IgnoreMissingImports; + + dc->lookupQualified(superContexts, DeclNameRef(membersName), decl->getLoc(), + lookupOptions, members); } // Check each member we found. @@ -1373,50 +1381,12 @@ TinyPtrVector OverrideMatcher::checkPotentialOverrides( return overridden; } -/// Determine which method or subscript this method or subscript overrides -/// (if any). -/// -/// \returns true if an error occurred. -bool swift::checkOverrides(ValueDecl *decl) { - // If there is a @_nonoverride attribute, this does not override anything. - if (decl->getAttrs().hasAttribute()) - 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(/*AllowInvalid=*/true) && - !decl->getAttrs().hasAttribute()) - return true; - - // Otherwise, we have more checking to do. - } - - // Members of constrained extensions are not considered to be overrides. - if (auto *ext = dyn_cast(decl->getDeclContext())) - if (ext->isConstrainedExtension()) - return false; - - // Accessor methods get overrides through their storage declaration, and - // all checking can be performed via that mechanism. - if (isa(decl)) { - (void)decl->getOverriddenDecls(); - return false; - } - - // Don't bother checking any further for invalid decls since they won't match - // anything. - if (decl->isInvalid()) - return true; - +static bool +checkPotentialOverrides(ValueDecl *decl, + TinyPtrVector &potentialOverrides, + bool ignoreMissingImports) { // Set up matching, but bail out if there's nothing to match. - OverrideMatcher matcher(decl); + OverrideMatcher matcher(decl, ignoreMissingImports); if (!matcher) return false; // Look for members with the same name and matching types as this @@ -1470,10 +1440,80 @@ bool swift::checkOverrides(ValueDecl *decl) { // FIXME: Check for missing 'override' keyword here? + for (auto match : matcher.checkPotentialOverrides(matches, attempt)) { + potentialOverrides.push_back(match); + } + + return true; +} + +/// Determine which method or subscript this method or subscript overrides +/// (if any). +/// +/// \returns true if an error occurred. +bool swift::checkOverrides(ValueDecl *decl) { + // If there is a @_nonoverride attribute, this does not override anything. + if (decl->getAttrs().hasAttribute()) + 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(/*AllowInvalid=*/true) && + !decl->getAttrs().hasAttribute()) + return true; + + // Otherwise, we have more checking to do. + } + + // Members of constrained extensions are not considered to be overrides. + if (auto *ext = dyn_cast(decl->getDeclContext())) + if (ext->isConstrainedExtension()) + return false; + + // Accessor methods get overrides through their storage declaration, and + // all checking can be performed via that mechanism. + if (isa(decl)) { + (void)decl->getOverriddenDecls(); + return false; + } + + // Don't bother checking any further for invalid decls since they won't match + // anything. + if (decl->isInvalid()) + return true; + + TinyPtrVector overridden; + if (!checkPotentialOverrides(decl, overridden, + /*ignoreMissingImports=*/false)) + return false; + + auto &ctx = decl->getASTContext(); + if (overridden.empty() && + ctx.LangOpts.hasFeature(Feature::MemberImportVisibility)) { + // If we didn't find anything, try broadening the search by ignoring missing + // imports. + if (!checkPotentialOverrides(decl, overridden, + /*ignoreMissingImports=*/true)) + return false; + + if (!overridden.empty()) { + auto first = overridden.front(); + if (maybeDiagnoseMissingImportForMember(first, decl->getDeclContext(), + decl->getLoc())) + return true; + } + } + // We performed override checking, so record the overrides. // FIXME: It's weird to be pushing state here, but how do we say that // this check subsumes the normal 'override' check? - auto overridden = matcher.checkPotentialOverrides(matches, attempt); if (overridden.empty()) invalidateOverrideAttribute(decl); decl->setOverriddenDecls(overridden); @@ -2263,8 +2303,8 @@ computeOverriddenAssociatedTypes(AssociatedTypeDecl *assocType) { return overriddenAssocTypes; } -llvm::TinyPtrVector -OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const { +static llvm::TinyPtrVector +computeOverriddenDecls(ValueDecl *decl, bool ignoreMissingImports) { // Value to return in error cases auto noResults = llvm::TinyPtrVector(); @@ -2359,7 +2399,7 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const { return noResults; // Check the correctness of the overrides. - OverrideMatcher matcher(accessor); + OverrideMatcher matcher(accessor, ignoreMissingImports); return matcher.checkPotentialOverrides( matches, OverrideCheckingAttempt::PerfectMatch); @@ -2373,7 +2413,7 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const { return noResults; // Try to match potential overridden declarations. - OverrideMatcher matcher(decl); + OverrideMatcher matcher(decl, ignoreMissingImports); if (!matcher) { return noResults; } @@ -2398,6 +2438,27 @@ OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const { OverrideCheckingAttempt::PerfectMatch); } +llvm::TinyPtrVector +OverriddenDeclsRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const { + auto &ctx = decl->getASTContext(); + auto overridden = computeOverriddenDecls(decl, false); + + // If we didn't find anything, try broadening the search by ignoring missing + // imports. + if (overridden.empty() && + ctx.LangOpts.hasFeature(Feature::MemberImportVisibility)) { + overridden = computeOverriddenDecls(decl, true); + if (!overridden.empty()) { + auto first = overridden.front(); + if (maybeDiagnoseMissingImportForMember(first, decl->getDeclContext(), + decl->getLoc())) + return {}; + } + } + + return overridden; +} + bool IsABICompatibleOverrideRequest::evaluate(Evaluator &evaluator, ValueDecl *decl) const { auto base = decl->getOverriddenDecl(); diff --git a/test/NameLookup/members_transitive.swift b/test/NameLookup/members_transitive.swift index dbd0dc71a9e63..cdb29ec29a486 100644 --- a/test/NameLookup/members_transitive.swift +++ b/test/NameLookup/members_transitive.swift @@ -7,7 +7,7 @@ // RUN: %target-swift-frontend -typecheck %s -I %t -verify -swift-version 5 -package-name TestPackage -enable-experimental-feature MemberImportVisibility -verify-additional-prefix member-visibility- import members_C -// expected-member-visibility-note 11{{add import of module 'members_B'}}{{1-1=internal import members_B\n}} +// expected-member-visibility-note 12{{add import of module 'members_B'}}{{1-1=internal import members_B\n}} func testExtensionMembers(x: X, y: Y) { @@ -70,7 +70,7 @@ func testTopLevelTypes() { class DerivedFromClassInC: DerivedClassInC { override func methodInA() {} - override func methodInB() {} // expected-member-visibility-error{{method does not override any method from its superclass}} + override func methodInB() {} // expected-member-visibility-error{{instance method 'methodInB()' is not available due to missing import of defining module 'members_B'}} override func methodInC() {} }