Skip to content

Sema: Omit internal from MemberImportVisibility fix-its when appropriate #81781

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
May 27, 2025
Merged

Sema: Omit internal from MemberImportVisibility fix-its when appropriate #81781

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
426f48d
Tests: Rename test cases associated with MemberImportVisibility.
tshortli May 23, 2025
dbb1d78
Sema: Omit `internal` from `MemberImportVisibility` fix-its when appr…
tshortli May 24, 2025
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
21 changes: 18 additions & 3 deletions lib/Sema/TypeCheckNameLookup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,6 +850,7 @@ struct MissingImportFixItInfo {
class MissingImportFixItCache {
SourceFile &sf;
llvm::DenseMap<const ModuleDecl *, MissingImportFixItInfo> infos;
bool internalImportsByDefaultEnabled;

MissingImportFixItInfo getFixItInfo(ModuleDecl *mod) {
auto existing = infos.find(mod);
Expand Down Expand Up @@ -881,15 +882,29 @@ class MissingImportFixItCache {

// Add an appropriate access level as long as it would not conflict with
// existing imports that lack access levels.
if (!foundImport || anyImportHasAccessLevel)
info.accessLevel = sf.getMaxAccessLevelUsingImport(mod);
auto accessLevelForImport = [&]() -> std::optional<AccessLevel> {
auto maxAccessLevel = sf.getMaxAccessLevelUsingImport(mod);
if (internalImportsByDefaultEnabled) {
if (!foundImport && maxAccessLevel <= AccessLevel::Internal)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check for imports in other files here? In internal-imports-by-default mode the access-level in other files shouldn't mater, the inconsistency diagnostics is disabled in that mode.

Copy link
Contributor Author

@tshortli tshortli May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My thinking is that we should follow the style that the developer is already using. If they write explicit access levels on imports of the module in other files, then the fix-it should too (which is handled by the logic below).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, that makes sense.

return std::nullopt;
}

if (foundImport && !anyImportHasAccessLevel)
return std::nullopt;

return maxAccessLevel;
};

info.accessLevel = accessLevelForImport();
infos[mod] = info;
return info;
}

public:
MissingImportFixItCache(SourceFile &sf) : sf(sf) {};
MissingImportFixItCache(SourceFile &sf) : sf(sf) {
internalImportsByDefaultEnabled = sf.getASTContext().LangOpts.hasFeature(
Feature::InternalImportsByDefault);
};

std::pair<SmallVector<ModuleDecl *, 2>,
SmallVector<MissingImportFixItInfo, 2>>
Expand Down
0 test/NameLookup/members_transitive.swift → ...NameLookup/member_import_visibility.swift
View file
Open in desktop
File renamed without changes.
0 ...mbers_transitive_compiler_protocols.swift → ...mport_visibility_compiler_protocols.swift
View file
Open in desktop
File renamed without changes.
0 ...Lookup/members_transitive_multifile.swift → .../member_import_visibility_multifile.swift
View file
Open in desktop
File renamed without changes.
35 changes: 24 additions & 11 deletions ...s_transitive_multifile_access_level.swift → ...t_visibility_multifile_access_level.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
// RUN: %target-swift-frontend -emit-module -o %t %t/PublicUsesOnly.swift
// RUN: %target-swift-frontend -emit-module -o %t %t/PublicUsesOnlyDefaultedImport.swift
// RUN: %target-swift-frontend -emit-module -o %t %t/MixedUses.swift
// RUN: %target-swift-frontend -emit-module -o %t %t/InternalUsesOnlyReexported.swift
// RUN: %target-swift-frontend -emit-module -o %t %t/InternalUsesOnlyTransitivelyImported.swift
// RUN: %target-swift-frontend -emit-module -o %t %t/Exports.swift -I %t
// RUN: %target-swift-frontend -emit-module -o %t %t/ImportsOtherModules.swift -I %t
// RUN: %target-swift-frontend -emit-module -o %t %t/InternalUsesOnlySPIOnly.swift -I %t
// RUN: %target-swift-frontend -emit-module -o %t %t/InternalUsesOnlyDefaultedImportSPIOnly.swift -I %t
// RUN: %target-swift-frontend -emit-module -o %t %t/PublicUsesOnlySPIOnly.swift -I %t
Expand Down Expand Up @@ -46,20 +47,23 @@ import Swift // Just here to anchor the fix-its
// expected-note@-3 {{add import of module 'PublicUsesOnly'}}{{1-1=public import PublicUsesOnly\n}}
// expected-note@-4 {{add import of module 'PublicUsesOnlyDefaultedImport'}}{{1-1=import PublicUsesOnlyDefaultedImport\n}}
// expected-note@-5 3 {{add import of module 'MixedUses'}}{{1-1=public import MixedUses\n}}
// expected-note@-6 {{add import of module 'InternalUsesOnlyTransitivelyImported'}}{{1-1=internal import InternalUsesOnlyTransitivelyImported\n}}
// expected-note@-7 {{add import of module 'InternalUsesOnlySPIOnly'}}{{1-1=internal import InternalUsesOnlySPIOnly\n}}
// expected-public-by-default-note@-8 {{add import of module 'InternalUsesOnlyDefaultedImportSPIOnly'}}{{1-1=@_spiOnly import InternalUsesOnlyDefaultedImportSPIOnly\n}}
// expected-internal-by-default-note@-9 {{add import of module 'InternalUsesOnlyDefaultedImportSPIOnly'}}{{1-1=import InternalUsesOnlyDefaultedImportSPIOnly\n}}
// expected-note@-10 {{add import of module 'PublicUsesOnlySPIOnly'}}{{1-1=@_spiOnly public import PublicUsesOnlySPIOnly\n}}

// expected-public-by-default-note@-6 {{add import of module 'InternalUsesOnlyReexported'}}{{1-1=internal import InternalUsesOnlyReexported\n}}
// expected-internal-by-default-note@-7 {{add import of module 'InternalUsesOnlyReexported'}}{{1-1=import InternalUsesOnlyReexported\n}}
// expected-public-by-default-note@-8 {{add import of module 'InternalUsesOnlyTransitivelyImported'}}{{1-1=internal import InternalUsesOnlyTransitivelyImported\n}}
// expected-internal-by-default-note@-9 {{add import of module 'InternalUsesOnlyTransitivelyImported'}}{{1-1=import InternalUsesOnlyTransitivelyImported\n}}
// expected-note@-10 {{add import of module 'InternalUsesOnlySPIOnly'}}{{1-1=internal import InternalUsesOnlySPIOnly\n}}
// expected-public-by-default-note@-11 {{add import of module 'InternalUsesOnlyDefaultedImportSPIOnly'}}{{1-1=@_spiOnly import InternalUsesOnlyDefaultedImportSPIOnly\n}}
// expected-internal-by-default-note@-12 {{add import of module 'InternalUsesOnlyDefaultedImportSPIOnly'}}{{1-1=import InternalUsesOnlyDefaultedImportSPIOnly\n}}
// expected-note@-13 {{add import of module 'PublicUsesOnlySPIOnly'}}{{1-1=@_spiOnly public import PublicUsesOnlySPIOnly\n}}

func internalFunc(_ x: Int) {
_ = x.memberInInternalUsesOnly // expected-error {{property 'memberInInternalUsesOnly' is not available due to missing import of defining module 'InternalUsesOnly'}}
_ = x.memberInInternalUsesOnlyDefaultedImport // expected-error {{property 'memberInInternalUsesOnlyDefaultedImport' is not available due to missing import of defining module 'InternalUsesOnlyDefaultedImport'}}
_ = x.memberInMixedUses // expected-error {{property 'memberInMixedUses' is not available due to missing import of defining module 'MixedUses'}}
_ = x.memberInInternalUsesOnlyTransitivelyImported // expected-error {{property 'memberInInternalUsesOnlyTransitivelyImported' is not available due to missing import of defining module 'InternalUsesOnlyTransitivelyImported'}}
_ = x.memberInInternalUsesOnlyReexported // expected-error {{property 'memberInInternalUsesOnlyReexported' is not available due to missing import of defining module 'InternalUsesOnlyReexported'}}
_ = x.memberInInternalUsesOnlySPIOnly // expected-error {{property 'memberInInternalUsesOnlySPIOnly' is not available due to missing import of defining module 'InternalUsesOnlySPIOnly'}}
_ = x.memberInInternalUsesOnlyDefaultedImportSPIOnly // expected-error {{property 'memberInInternalUsesOnlyDefaultedImportSPIOnly' is not available due to missing import of defining module 'InternalUsesOnlyDefaultedImportSPIOnly'}}
_ = x.memberInInternalUsesOnlyTransitivelyImported // expected-error {{property 'memberInInternalUsesOnlyTransitivelyImported' is not available due to missing import of defining module 'InternalUsesOnlyTransitivelyImported'}}
}

@inlinable package func packageInlinableFunc(_ x: Int) {
Expand Down Expand Up @@ -146,7 +150,7 @@ internal import PackageUsesOnly
internal import PublicUsesOnly
import PublicUsesOnlyDefaultedImport
internal import MixedUses
internal import Exports
internal import ImportsOtherModules
@_spiOnly public import InternalUsesOnlySPIOnly
@_spiOnly import InternalUsesOnlyDefaultedImportSPIOnly
@_spiOnly public import PublicUsesOnlySPIOnly
Expand Down Expand Up @@ -199,6 +203,14 @@ extension Int {
public var memberInMixedUses: Int { return self }
}

//--- InternalUsesOnlyReexported.swift

extension Int {
public typealias TypealiasInInternalUsesOnlyReexported = Self
public struct NestedInInternalUsesOnlyReexported {}
public var memberInInternalUsesOnlyReexported: Int { return self }
}

//--- InternalUsesOnlyTransitivelyImported.swift

extension Int {
Expand All @@ -207,9 +219,10 @@ extension Int {
public var memberInInternalUsesOnlyTransitivelyImported: Int { return self }
}

//--- Exports.swift
//--- ImportsOtherModules.swift

@_exported import InternalUsesOnlyTransitivelyImported
@_exported import InternalUsesOnlyReexported
import InternalUsesOnlyTransitivelyImported

//--- InternalUsesOnlySPIOnly.swift

Expand Down
0 .../NameLookup/members_transitive_objc.swift → ...ookup/member_import_visibility_objc.swift
View file
Open in desktop
File renamed without changes.
0 ...p/members_transitive_objc_overrides.swift → ...er_import_visibility_objc_overrides.swift
View file
Open in desktop
File renamed without changes.
0 ...mbers_transitive_public_module_name.swift → ...mport_visibility_public_module_name.swift
View file
Open in desktop
File renamed without changes.
0 ...members_transitive_underlying_clang.swift → ..._import_visibility_underlying_clang.swift
View file
Open in desktop
File renamed without changes.