From 80531206244fc6f1fecef6a50c26ea061247d58f Mon Sep 17 00:00:00 2001 From: Allan Shortlidge Date: Thu, 2 Oct 2025 16:24:12 -0700 Subject: [PATCH] Sema: Fix a MemberImportVisibility regression in `resolveDeclRefExpr()`. The changes from https://github.com/swiftlang/swift/pull/84259/ caused a regression in which declarations from an outer scope could be shadowed inappropriately by member declarations from a module that has not been imported. This fix addresses the issue by refactoring `resolveDeclRefExpr()` so that it performs lookups in two passes. First, it attempts to resolve the decl ref using the complete lookup algorithm and standard name lookup options, which will ignore members from modules that haven't been imported if `MemberImportVisibility` is enabled. Then, if no results were found it re-runs the full lookup algorithm again with `NameLookupFlags::IgnoreMissingImports` included to find additional results that ought to be diagnosed as unavailable. This insures that the unavailable results are not considered until after the main lookup has already failed. Resolves rdar://161078015. --- lib/Sema/PreCheckTarget.cpp | 301 ++++++++++-------- .../MemberImportVisibility/members_A.swift | 21 ++ .../MemberImportVisibility/members_B.swift | 5 + .../MemberImportVisibility/members_C.swift | 3 + .../NameLookup/member_import_visibility.swift | 18 ++ 5 files changed, 211 insertions(+), 137 deletions(-) diff --git a/lib/Sema/PreCheckTarget.cpp b/lib/Sema/PreCheckTarget.cpp index 4475f2a07790f..6f297a7a2f1c5 100644 --- a/lib/Sema/PreCheckTarget.cpp +++ b/lib/Sema/PreCheckTarget.cpp @@ -518,11 +518,12 @@ diagnoseUnqualifiedInit(UnresolvedDeclRefExpr *initExpr, DeclContext *dc, initExpr->getNameLoc(), /*implicit=*/true); } -/// Bind an UnresolvedDeclRefExpr by performing name lookup and -/// returning the resultant expression. Context is the DeclContext used -/// for the lookup. -Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, - DeclContext *DC) { +/// Bind an UnresolvedDeclRefExpr by performing name lookup using the given +/// options and returning the resultant expression. `DC` is the DeclContext +/// used for the lookup. If the lookup doesn't find any results, returns +/// `nullptr`. +static Expr *resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC, + NameLookupOptions lookupOptions) { auto &Context = DC->getASTContext(); DeclNameRef Name = UDRE->getName(); SourceLoc Loc = UDRE->getLoc(); @@ -562,14 +563,6 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, LookupName = DeclNameRef(lookupName); } - // Perform standard value name lookup. - NameLookupOptions lookupOptions = defaultUnqualifiedLookupOptions; - // TODO: Include all of the possible members to give a solver a - // chance to diagnose name shadowing which requires explicit - // name/module qualifier to access top-level name. - lookupOptions |= NameLookupFlags::IncludeOuterResults; - lookupOptions |= NameLookupFlags::IgnoreMissingImports; - LookupResult Lookup; bool AllDeclRefs = true; @@ -628,17 +621,8 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, } if (!Lookup) { - // If we failed lookup of an operator, check to see if this is a range - // operator misspelling. Otherwise try to diagnose a juxtaposition - // e.g. (x*-4) that needs whitespace. - if (diagnoseRangeOperatorMisspell(Context.Diags, UDRE) || - diagnoseIncDecOperator(Context.Diags, UDRE) || - diagnoseOperatorJuxtaposition(UDRE, DC) || - diagnoseNonexistentPowerOperator(Context.Diags, UDRE, DC)) { - return errorResult(); - } - - // Try ignoring access control. + // For the purpose of diagnosing inaccessible results, try the lookup again + // but ignore access control. NameLookupOptions relookupOptions = lookupOptions; relookupOptions |= NameLookupFlags::IgnoreAccessControl; auto inaccessibleResults = @@ -663,117 +647,8 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, return errorResult(); } - // TODO: Name will be a compound name if it was written explicitly as - // one, but we should also try to propagate labels into this. - DeclNameLoc nameLoc = UDRE->getNameLoc(); - - Identifier simpleName = Name.getBaseIdentifier(); - const char *buffer = simpleName.get(); - llvm::SmallString<64> expectedIdentifier; - bool isConfused = false; - uint32_t codepoint; - uint32_t firstConfusableCodepoint = 0; - int totalCodepoints = 0; - int offset = 0; - while ((codepoint = validateUTF8CharacterAndAdvance(buffer, - buffer + - strlen(buffer))) - != ~0U) { - int length = (buffer - simpleName.get()) - offset; - if (auto expectedCodepoint = - confusable::tryConvertConfusableCharacterToASCII(codepoint)) { - if (firstConfusableCodepoint == 0) { - firstConfusableCodepoint = codepoint; - } - isConfused = true; - expectedIdentifier += expectedCodepoint; - } else { - expectedIdentifier += (char)codepoint; - } - - totalCodepoints++; - - offset += length; - } - - auto emitBasicError = [&] { - - if (Name.isSimpleName(Context.Id_self)) { - // `self` gets diagnosed with a different error when it can't be found. - Context.Diags - .diagnose(Loc, diag::cannot_find_self_in_scope) - .highlight(UDRE->getSourceRange()); - } else { - Context.Diags - .diagnose(Loc, diag::cannot_find_in_scope, Name, - Name.isOperator()) - .highlight(UDRE->getSourceRange()); - } - - if (!Context.LangOpts.DisableExperimentalClangImporterDiagnostics) { - Context.getClangModuleLoader()->diagnoseTopLevelValue( - Name.getFullName()); - } - }; - - if (!isConfused) { - if (Name.isSimpleName(Context.Id_Self)) { - if (DeclContext *typeContext = DC->getInnermostTypeContext()){ - Type SelfType = typeContext->getSelfInterfaceType(); - - if (typeContext->getSelfClassDecl() && - !typeContext->getSelfClassDecl()->isForeignReferenceType()) - SelfType = DynamicSelfType::get(SelfType, Context); - return new (Context) - TypeExpr(new (Context) SelfTypeRepr(SelfType, Loc)); - } - } - - TypoCorrectionResults corrections(Name, nameLoc); - - // FIXME: Don't perform typo correction inside macro arguments, because it - // will invoke synthesizing declarations in this scope, which will attempt to - // expand this macro which leads to circular reference errors. - if (!namelookup::isInMacroArgument(DC->getParentSourceFile(), UDRE->getLoc())) { - TypeChecker::performTypoCorrection(DC, UDRE->getRefKind(), Type(), - lookupOptions, corrections); - } - - if (auto typo = corrections.claimUniqueCorrection()) { - auto diag = Context.Diags.diagnose( - Loc, diag::cannot_find_in_scope_corrected, Name, - Name.isOperator(), typo->CorrectedName.getBaseIdentifier().str()); - diag.highlight(UDRE->getSourceRange()); - typo->addFixits(diag); - } else { - emitBasicError(); - } - - corrections.noteAllCandidates(); - } else { - emitBasicError(); - - if (totalCodepoints == 1) { - auto charNames = confusable::getConfusableAndBaseCodepointNames( - firstConfusableCodepoint); - Context.Diags - .diagnose(Loc, diag::single_confusable_character, - UDRE->getName().isOperator(), simpleName.str(), - charNames.first, expectedIdentifier, charNames.second) - .fixItReplace(Loc, expectedIdentifier); - } else { - Context.Diags - .diagnose(Loc, diag::confusable_character, - UDRE->getName().isOperator(), simpleName.str(), - expectedIdentifier) - .fixItReplace(Loc, expectedIdentifier); - } - } - - // TODO: consider recovering from here. We may want some way to suppress - // downstream diagnostics, though. - - return errorResult(); + // No results were found. + return nullptr; } // FIXME: Need to refactor the way we build an AST node from a lookup result! @@ -892,8 +767,9 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, UDRE->getNameLoc().getStartLoc()); } - return buildRefExpr(ResultValues, DC, UDRE->getNameLoc(), - UDRE->isImplicit(), UDRE->getFunctionRefInfo()); + return TypeChecker::buildRefExpr(ResultValues, DC, UDRE->getNameLoc(), + UDRE->isImplicit(), + UDRE->getFunctionRefInfo()); } ResultValues.clear(); @@ -983,6 +859,157 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, return errorResult(); } +Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, + DeclContext *DC) { + auto &Context = DC->getASTContext(); + + // Perform standard value name lookup. + NameLookupOptions lookupOptions = defaultUnqualifiedLookupOptions; + // TODO: Include all of the possible members to give a solver a + // chance to diagnose name shadowing which requires explicit + // name/module qualifier to access top-level name. + lookupOptions |= NameLookupFlags::IncludeOuterResults; + + Expr *result = ::resolveDeclRefExpr(UDRE, DC, lookupOptions); + if (!result && Context.LangOpts.hasFeature(Feature::MemberImportVisibility, + /*allowMigration=*/true)) { + // If we didn't find a result, try again but this time relax + // MemberImportVisibility restrictions. Note that diagnosing the missing + // import is already handled by resolveDeclRefExpr(). + lookupOptions |= NameLookupFlags::IgnoreMissingImports; + result = ::resolveDeclRefExpr(UDRE, DC, lookupOptions); + } + + if (result) + return result; + + // We didn't find any results. Look for common mistakes to diagnose. + DeclNameRef Name = UDRE->getName(); + SourceLoc Loc = UDRE->getLoc(); + if (Loc.isInvalid()) + DC = DC->getModuleScopeContext(); + + auto errorResult = [&]() -> Expr * { + return new (Context) ErrorExpr(UDRE->getSourceRange()); + }; + + // If we failed lookup of an operator, check to see if this is a range + // operator misspelling. Otherwise try to diagnose a juxtaposition + // e.g. (x*-4) that needs whitespace. + if (diagnoseRangeOperatorMisspell(Context.Diags, UDRE) || + diagnoseIncDecOperator(Context.Diags, UDRE) || + diagnoseOperatorJuxtaposition(UDRE, DC) || + diagnoseNonexistentPowerOperator(Context.Diags, UDRE, DC)) { + return errorResult(); + } + + // TODO: Name will be a compound name if it was written explicitly as + // one, but we should also try to propagate labels into this. + DeclNameLoc nameLoc = UDRE->getNameLoc(); + + Identifier simpleName = Name.getBaseIdentifier(); + const char *buffer = simpleName.get(); + llvm::SmallString<64> expectedIdentifier; + bool isConfused = false; + uint32_t codepoint; + uint32_t firstConfusableCodepoint = 0; + int totalCodepoints = 0; + int offset = 0; + while ((codepoint = validateUTF8CharacterAndAdvance( + buffer, buffer + strlen(buffer))) != ~0U) { + int length = (buffer - simpleName.get()) - offset; + if (auto expectedCodepoint = + confusable::tryConvertConfusableCharacterToASCII(codepoint)) { + if (firstConfusableCodepoint == 0) { + firstConfusableCodepoint = codepoint; + } + isConfused = true; + expectedIdentifier += expectedCodepoint; + } else { + expectedIdentifier += (char)codepoint; + } + + totalCodepoints++; + + offset += length; + } + + auto emitBasicError = [&] { + if (Name.isSimpleName(Context.Id_self)) { + // `self` gets diagnosed with a different error when it can't be found. + Context.Diags.diagnose(Loc, diag::cannot_find_self_in_scope) + .highlight(UDRE->getSourceRange()); + } else { + Context.Diags + .diagnose(Loc, diag::cannot_find_in_scope, Name, Name.isOperator()) + .highlight(UDRE->getSourceRange()); + } + + if (!Context.LangOpts.DisableExperimentalClangImporterDiagnostics) { + Context.getClangModuleLoader()->diagnoseTopLevelValue(Name.getFullName()); + } + }; + + if (!isConfused) { + if (Name.isSimpleName(Context.Id_Self)) { + if (DeclContext *typeContext = DC->getInnermostTypeContext()) { + Type SelfType = typeContext->getSelfInterfaceType(); + + if (typeContext->getSelfClassDecl() && + !typeContext->getSelfClassDecl()->isForeignReferenceType()) + SelfType = DynamicSelfType::get(SelfType, Context); + return new (Context) + TypeExpr(new (Context) SelfTypeRepr(SelfType, Loc)); + } + } + + TypoCorrectionResults corrections(Name, nameLoc); + + // FIXME: Don't perform typo correction inside macro arguments, because it + // will invoke synthesizing declarations in this scope, which will attempt + // to expand this macro which leads to circular reference errors. + if (!namelookup::isInMacroArgument(DC->getParentSourceFile(), + UDRE->getLoc())) { + TypeChecker::performTypoCorrection(DC, UDRE->getRefKind(), Type(), + lookupOptions, corrections); + } + + if (auto typo = corrections.claimUniqueCorrection()) { + auto diag = Context.Diags.diagnose( + Loc, diag::cannot_find_in_scope_corrected, Name, Name.isOperator(), + typo->CorrectedName.getBaseIdentifier().str()); + diag.highlight(UDRE->getSourceRange()); + typo->addFixits(diag); + } else { + emitBasicError(); + } + + corrections.noteAllCandidates(); + } else { + emitBasicError(); + + if (totalCodepoints == 1) { + auto charNames = confusable::getConfusableAndBaseCodepointNames( + firstConfusableCodepoint); + Context.Diags + .diagnose(Loc, diag::single_confusable_character, + UDRE->getName().isOperator(), simpleName.str(), + charNames.first, expectedIdentifier, charNames.second) + .fixItReplace(Loc, expectedIdentifier); + } else { + Context.Diags + .diagnose(Loc, diag::confusable_character, + UDRE->getName().isOperator(), simpleName.str(), + expectedIdentifier) + .fixItReplace(Loc, expectedIdentifier); + } + } + + // TODO: consider recovering from here. We may want some way to suppress + // downstream diagnostics, though. + return errorResult(); +} + /// If an expression references 'self.init' or 'super.init' in an /// initializer context, returns the implicit 'self' decl of the constructor. /// Otherwise, return nil. diff --git a/test/NameLookup/Inputs/MemberImportVisibility/members_A.swift b/test/NameLookup/Inputs/MemberImportVisibility/members_A.swift index 1788d9aa02287..5cebe137152d9 100644 --- a/test/NameLookup/Inputs/MemberImportVisibility/members_A.swift +++ b/test/NameLookup/Inputs/MemberImportVisibility/members_A.swift @@ -16,6 +16,24 @@ infix operator <<< infix operator >>> infix operator <> +@available(*, deprecated) +public func shadowedByMemberOnXinA() { } + +@available(*, deprecated) +public func shadowedByMemberOnXinB() { } + +@available(*, deprecated) +public func shadowedByMemberOnXinC() { } + +@available(*, deprecated) +public func shadowedByStaticMemberOnXinA() { } + +@available(*, deprecated) +public func shadowedByStaticMemberOnXinB() { } + +@available(*, deprecated) +public func shadowedByStaticMemberOnXinC() { } + extension X { public func XinA() { } @@ -28,6 +46,9 @@ extension X { public struct NestedInA {} public protocol ProtoNestedInA {} + + public func shadowedByMemberOnXinA() { } + public static func shadowedByStaticMemberOnXinA() { } } extension Y { diff --git a/test/NameLookup/Inputs/MemberImportVisibility/members_B.swift b/test/NameLookup/Inputs/MemberImportVisibility/members_B.swift index d27dcfdf72e89..05c8cd51fe03a 100644 --- a/test/NameLookup/Inputs/MemberImportVisibility/members_B.swift +++ b/test/NameLookup/Inputs/MemberImportVisibility/members_B.swift @@ -11,6 +11,11 @@ extension X { public var propXinB: Bool { return true } package var propXinB_package: Bool { return true } + public func shadowedByMemberOnXinB() { } + public static func shadowedByStaticMemberOnXinB() { } + + public static var max: Int { return Int.min } + public static func >>>(a: Self, b: Self) -> Self { b } public struct NestedInB {} diff --git a/test/NameLookup/Inputs/MemberImportVisibility/members_C.swift b/test/NameLookup/Inputs/MemberImportVisibility/members_C.swift index c80eb9abb895a..8cc0492998ad6 100644 --- a/test/NameLookup/Inputs/MemberImportVisibility/members_C.swift +++ b/test/NameLookup/Inputs/MemberImportVisibility/members_C.swift @@ -10,6 +10,9 @@ extension X { public var propXinC: Bool { return true } + public func shadowedByMemberOnXinC() { } + public static func shadowedByStaticMemberOnXinC() { } + public static func <>(a: Self, b: Self) -> Self { a } public struct NestedInC {} diff --git a/test/NameLookup/member_import_visibility.swift b/test/NameLookup/member_import_visibility.swift index 0f58fd75ece6d..5e23718056901 100644 --- a/test/NameLookup/member_import_visibility.swift +++ b/test/NameLookup/member_import_visibility.swift @@ -99,6 +99,24 @@ extension X { func hasNestedInAInGenericReqs(_ t: T) where T: ProtoNestedInA { } func hasNestedInBInGenericReqs(_ t: T) where T: ProtoNestedInB { } // expected-member-visibility-error{{protocol 'ProtoNestedInB' is not available due to missing import of defining module 'members_B'}} func hasNestedInCInGenericReqs(_ t: T) where T: ProtoNestedInC { } + + func testMembersShadowingGlobals() { + shadowedByMemberOnXinA() + shadowedByMemberOnXinB() // expected-member-visibility-warning{{'shadowedByMemberOnXinB()' is deprecated}} + shadowedByMemberOnXinC() + + _ = max(0, 1) // expected-ambiguity-error{{static member 'max' cannot be used on instance of type 'X'}} + // expected-ambiguity-error@-1{{cannot call value of non-function type 'Int'}} + } + + static func testStaticMembersShadowingGlobals() { + shadowedByStaticMemberOnXinA() + shadowedByStaticMemberOnXinB() // expected-member-visibility-warning{{'shadowedByStaticMemberOnXinB()' is deprecated}} + shadowedByStaticMemberOnXinC() + + _ = max(0, 1) // expected-ambiguity-error{{use of 'max' refers to instance method rather than global function 'max' in module 'Swift'}} + // expected-ambiguity-note@-1{{use 'Swift.' to reference the global function in module 'Swift'}} + } } extension X.NestedInA {}