Add missing imports for MemberImportVisibility - #3395
Conversation
Building swift-syntax with the MemberImportVisibility upcoming feature enabled fails because a number of files use declarations from modules that are only imported transitively. Add the missing direct imports. Also declare SwiftBasicFormat as a dependency of SwiftSyntaxMacroExpansion in Package.swift; the module was previously only available transitively, and its CMakeLists.txt already links it. Fixes swiftlang#3301
|
Thanks!
I think we should do that in this PR to ensure we don't regress |
Now that all targets build with the upcoming feature, turn it on for the package so that missing imports can't creep back in. Toolchains that don't know the feature ignore the flag.
|
Done — enabled it for all regular and test targets in |
hamishknight
left a comment
hamishknight
left a comment
There was a problem hiding this comment.
Thanks! Can you also update CMakeLists.txt to match? (sorry I should have mentioned this in my original comment)
|
|
||
| // Require every file to import the modules that define the members it uses, so that missing | ||
| // imports can't creep back in. Toolchains that don't know the upcoming feature ignore the flag. | ||
| for target in package.targets where target.type == .regular || target.type == .test { |
There was a problem hiding this comment.
I don't think we need to be this restrictive on the target kind, let's do type != .plugin instead, matching what we do for sourcekit-lsp (https://github.com/swiftlang/sourcekit-lsp/blob/a31d272f3cb392b443f42e1f97042a2a607073ec/Package.swift#L706)
There was a problem hiding this comment.
Done — switched to type != .plugin and added the flag to CMakeLists.txt. Verified the CMake build locally with the feature enabled.
Pass the upcoming feature in CMakeLists.txt as well, so the CMake build enforces the same imports as the SwiftPM build. Also relax the target filter in Package.swift to everything but plugins, matching sourcekit-lsp.
2 participants
Building swift-syntax with the
MemberImportVisibilityupcoming feature enabled currently fails: a number of files use declarations from modules that are only imported transitively — mostly the string-interpolation conformances fromSwiftSyntaxBuilderand the indentation helpers fromSwiftBasicFormat.This PR adds the missing direct imports so that
builds cleanly. All changes are plain
importadditions, with two exceptions:SwiftSyntaxMacroExpansionusedSwiftBasicFormatwithout declaring it inPackage.swift(the module was only available transitively; itsCMakeLists.txtalready links it), so the dependency is now declared.import SwiftBasicFormatinMacroExpansion.swiftbecomesinternal importto avoid an "ambiguous implicit access level for import" error once other files in the module import it asinternal.No
CMakeLists.txtchanges are needed — all affected modules already link the corresponding libraries.The full test suite passes with the feature enabled, and
swift format lintis clean on the touched files. A possible follow-up would be to enable the feature inPackage.swiftso new violations can't creep in.Fixes #3301