[SR-1393] [SwiftPM] Enforce Swift module import dependencies

[ddunbar]

Previous ID	SR-1393
Radar	rdar://problem/23237241
Original Reporter	@ddunbar
Type	Bug
Status	In Progress
Resolution

Additional Detail from JIRA

Votes	9
Component/s	Package Manager
Labels	Bug
Assignee	owenvoorhees (JIRA)
Priority	Medium

md5: b673faead2f75706db0bdac53b7b8ef3

is duplicated by:

• SR-4539 Build executable with modules, without specified dependencies
• SR-8344 SPM include path is too permissive, leads to link errors that should have been compile errors
• SR-9359 SwiftPM should have better diagnostics if tests depend on executable target
• SR-11758 Undefined symbol: static allcases.MyEnum.allCases.getter : [allcases.MyEnum]

relates to:

• SR-3033 Poor diagnostic when importing executable target module in a test
• SR-13651 Linker error while linking unit test cases
• SR-3205 Ability to restrict the import of implicit module dependencies
• SR-14212 Removing package from Package.swift doesn't result in import error
• SR-7483 Deserialization Failure

Issue Description:

Currently, SwiftPM passes a -I to the directory containing all built modules, which allows any Swift code to import any module which has been built.

This is unfortunate, as it means code can easily import modules which haven't been explicitly declared as a dependency. That might frequently work but it can be the source of transient build failures or "works for me" type problems.

Instead, we should enforce that Swift code is only allowed to import modules which have been explicitly declared as dependencies. We could also let this include transitive dependencies, but I would actually prefer we require any module explicitly declare all other modules it is allowed to import.

swiftc doesn't expose a mechanism currently we can use to implement this directly. We could:

• Extend swiftc to explicitly support this.

• Build symlink trees for each target we want to build, and point the -I at that instead, to enforce what is allowed.

[ddunbar]

/cc @belkadan any preference on the implementation approach here? For the swiftc route, I was imagining some kind of manifest file we could hand to swiftc which described exactly which modules could or could not be imported. This would allow us to give substantially better diagnostics, and could play into a long term story for automatically assisting with dependency management.

[aciidgh]

We already create build dir for each module, what about creating the swiftmodules in those directories and -I to them instead of entire debug/release folder?

[ddunbar]

We could do that too... I would rather not end up with a massive amount of -I arguments. Also, I think we probably need to tread carefully here with how those modules find the other modules they depend on... it may be the case that my proposed "symlink tree" approach doesn't actually work for this reason.

[belkadan]

I don't like the idea of repeating information, but I guess we already have that (since the imported module name isn't enough to know where to get the thing). How does this work with packages containing multiple modules, though?

[aciidgh]

it works something like this currently:
Internal modules can be imported without declaring them as dependencies but it might result in build failures

1. The module which is imported is not built when the importing module is building.
2. The importing module is a product and it'll produce linker error.

External dependencies are implicit dependencies to all of internal modules so they can always be imported which I think is correct behavior.

[belkadan]

Sorry, I meant "what's the plan for packages containing multiple modules?". Do we want to worry about "you didn't declare a dependency on this module, but someone else did, and you do depend on something else in the same package"?

[swift-ci]

Comment by Shawn Morel (JIRA)

@aciidb0mb3r Going to bump the discussion here since this bug is more than 2 years old and you just closed a bug I filed as a duplicate. I think the usability of SPM is a place I might be able to get might feet wet 🙂

Reading through the comments here, I see some overlap with what I mentioned in [SR-8344] as possible approaches but 1) the state of the toolchain has evolved since may 2016 so no everything still applies and 2) it's not 100% clear to me where along the spm, sbt, swiftc chain the fix belongs.

I think the easiest / quickest fix would be to pass multiple -I import search paths and not keep the .swiftmodules at the top of the build artifacts tree. I don't know how this overlaps with @belkadan's latest proposal for .swiftinterface files and using .swiftmodules as a compiler cache mechanism only.

This also doesn't address the issue of the dependencies implicitly emitted with the '-emit-dependencies' flag and the .o files being directly listed in the Objects.LinkedFileList

From a usability standpoint, I think it's poor behaviour if something compiles only to fail with a linker error about a mangled symbol. It also impedes building tools like a well behaved language server for VScode on linux where explicit search paths can be queried easily.

Any advice on direction is appreciated

[aciidgh]

@belkadan do you have any advice on how to implement this? What if SwiftPM passed the list of module names that are allowed to be imported?

[belkadan]

If you're willing to allow transitive dependencies, multiple search paths seems like a very composable answer. If you want to catch accidental uses of transitive dependencies, you'd need something more like the "allow" list, but I don't know how you'd exempt system modules from that checking.

[aciidgh]

I discussed this with Jordan. We think creating a response file per-module with list of all search paths it depends on is a reasonable solution. There will be some performance impact as the compiler will need to do extra work but it shouldn't be significant.

[ddunbar]

@aciidb0mb3r just so I'm clear, meaning we will do work in SwiftPM to just put all the modules in separate places and pass a long list of them? Or this is depending on some new feature of the Swift compiler.

I'm personally fine trying to solve this in the PM exclusively to start with, since it will involve less moving parts even if it does make for gross command lines. We could also use symlink trees to avoid the gross command lines.

[aciidgh]

Right, we will put module files in target specific directories and add include paths. Command line won't be gross because the compiler can accept response files now. This will be similar to the linked filelist we pass to the compiler.

[weissi]

We still have terrible diagnostics for this. @aciidb0mb3r is the quickest way to get good diagnostics for the common case (https://bugs.swift.org/browse/SR-9359) fixing this issue? If yes, then we should really prioritise it; if no and there's a quicker win, we should probably un-dupe https://bugs.swift.org/browse/SR-9359 .

So many people run into the 'tests depending on executable target' problem, I can't count the numbers I've personally wasted on this or the number of times I've told others.

[abertelrud]

Since the diagnostics are coming from underlying tools, I think that separating out the sets of module files so you can't import the module for a component you can't link against seems like the best way to fix it. As I understand it, that's what this bug report covers. I agree that this should be prioritized.

[weissi]

@tomerd this is still an issue today and causes failures that are hard to debug for anybody who isn't super experienced with SwiftPM and knows that any "missing module XYZ" likely means that some Package.swift file somewhere in the graph has a missing dependency. Even when knowing that fact, it can still be incredibly tedious to track down.