Verify that only targets that are manifest dependencies can be imported. #3562

artemcm · 2021-06-16T21:57:46Z

This change introduces a build verification step that attempts to detect scenarios where a target contains an import of another target in the package without declaring the imported target as a dependency in the manifest.
This is done via SwiftDriver's import-prescan capability which relies on libSwiftScan to quickly parse a target's sources and identify all imported modules.

artemcm · 2021-06-25T18:04:17Z

@swift-ci please smoke test

tomerd · 2021-06-25T19:43:19Z

cc @elsh

neonichu

Thanks a lot for this PR, Artem!

artemcm · 2021-06-25T20:49:22Z

@swift-ci please smoke test

artemcm · 2021-06-25T21:38:13Z

Oof, emitCommandLine has side-effects which change build outputs. Its usage in this PR caused several tests to fail because we're now building more than we used to.

I'm looking at how to work around this/remove these side-effects.

artemcm · 2021-06-25T22:10:08Z

@swift-ci please smoke test

artemcm · 2021-06-25T23:41:48Z

The current failure seems to be due to the fact that the toolchain being tested does not come with libSwiftScan, so the driver falls back on invoking the scanning action using a swift-frontend invocation, which works just as well, but causes the driver to emit an additional warning:

warning: Fallback to `swift-frontend` dependency scanner invocation

Which is not expected to be seen in some of these tests.

artemcm · 2021-06-25T23:51:16Z

Together with apple/swift-driver#731
@swift-ci please smoke test

artemcm · 2021-06-28T17:23:45Z

@swift-ci please smoke test

artemcm · 2021-06-28T19:40:19Z

@swift-ci please smoke test

artemcm · 2021-06-29T16:57:55Z

@swift-ci please smoke test

tomerd · 2021-06-29T20:19:45Z

@swift-ci please test package compatibility

tomerd · 2021-06-29T20:20:13Z

@artemcm also running the compatibility test suite to make sure we dont break any packages

neonichu · 2021-06-29T21:55:50Z

I don't see how the Linux self-hosted failure could be caused by the changes in this PR and I also can't reproduce them locally so far. I think we should still understand why they're failing even though we don't require them to pass for merging.

artemcm · 2021-06-30T21:00:09Z

@swift-ci please smoke test

ktoso · 2021-10-06T03:31:54Z

Coming over here with some friendly cheer leading -- hit such issue hard right now and took us a while to debug it.
Really really looking forward to this PR landing, would have saved us much time ❤️

artemcm · 2021-10-26T23:11:51Z

@swift-ci please smoke test

ktoso · 2021-10-27T01:36:10Z

Thanks @artemcm !

I talked with @tomerd and @abertelrud and we'll try and see if we could solve the swift-syntax being usable here, and then we could try to use that for the parsing instead... Thank you for the rebase, we'll report back soon what the plan here is!

artemcm · 2021-10-27T16:02:12Z

In the meantime, I've reversed the build command-line option I had here from a disable to an enable option, if we would like to land this as an opt-in feature for the time being. It would at the very least be useful to have enabled in CI configurations.

artemcm · 2021-10-27T17:07:17Z

@swift-ci please smoke test

artemcm · 2021-10-27T17:21:33Z

@swift-ci please smoke test Linux platform

artemcm · 2021-10-27T19:31:37Z

@swift-ci please smoke test Linux platform

ktoso · 2022-07-05T23:20:02Z

Periodic ping here... this has hit us again and broke users trying to play around with distributed actors because it sneaked through CI which did not validate it, nor did it show up on dev machines, since we're not using Xcode most of the time; apple/swift-distributed-actors#981

SDGGiesbrecht · 2022-07-05T23:55:08Z

+1

Originally this did not seem particularly important. But ever since Xcode started adding import statements behind your back, stray invalid imports have been spreading like wildfire. Consequently, this feature has rocketed to the very highest ranks of my most wanted list.

This change introduces a build verification step that attempts to detect scenarios where a target contains an `import` of another target in the package without declaring the imported target as a dependency in the manifest. This is done via SwiftDriver's import-prescan capability which relies on libSwiftScan to quickly parse a target's sources and identify all `import`ed modules. Related to rdar://79423257

… host compiler does not support it.

…ted source-code may not be available at the time of scan.

`--disable-explicit-target-dependency-import-checking` prevents the verification code from being run as a just-in-case option to opt-out from this warning

…n to an opt-in `enable` option. While we work on optimizing incremental build performance of this check, it would be nice to have it as an opt-in option.

…warning', or 'none' modes, with 'none' being the default.

artemcm · 2022-07-06T18:57:23Z

@swift-ci smoke test

ktoso · 2022-07-07T13:59:09Z

Thank you very much folks!

MaxDesiatov · 2023-12-11T15:26:04Z

Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/main.swift

@@ -0,0 +1 @@
+print(baz(x: 11))


@artemcm @neonichu this test started failing recently, due to

error: cannot find 'baz' in scope print(baz(x: 11))

but only on macOS. I'm not sure how it was suposed to work in the first place and how it worked before and still works on other platforms, since baz(x:) is not defined anywhere?

CI failure log for now available at https://ci.swift.org/job/swift-package-manager-with-xcode-self-hosted-PR-osx/2994/console

I think the error is expected, right? The test is using XCTAssertThrowsError in all cases.

I think the problem is that Target A imports another target (B) in the package without declaring it a dependency. isn't present in the output at all.

My assumption would be that <unknown>:0: error: unknown argument: '-dwarf-version=4 interferes with the import detection, e.g. we may not get a list of imports at all and from that follows the test failure.

AFAIK, we haven't updated the installed Swift version, so it's a bit of a mystery how this broke?

Actually maybe this will be the fix? apple/swift-driver#1502

artemcm force-pushed the VerifyOnlyDependencyTargetsImported branch from b9faf24 to 6af65b0 Compare June 16, 2021 23:16

artemcm changed the title ~~[WIP][DNM] Verify that only targets that are manifest dependencies can be imported.~~ Verify that only targets that are manifest dependencies can be imported. Jun 23, 2021

artemcm force-pushed the VerifyOnlyDependencyTargetsImported branch 3 times, most recently from 3eda4d7 to 3b840ca Compare June 25, 2021 18:04

artemcm marked this pull request as ready for review June 25, 2021 18:11

artemcm requested review from abertelrud, friedbunny, neonichu and tomerd as code owners June 25, 2021 18:11

tomerd requested a review from elsh June 25, 2021 19:43

tomerd assigned elsh Jun 25, 2021

neonichu approved these changes Jun 25, 2021

View reviewed changes

artemcm force-pushed the VerifyOnlyDependencyTargetsImported branch 2 times, most recently from 8e377b8 to 11e0f93 Compare June 25, 2021 20:49

artemcm force-pushed the VerifyOnlyDependencyTargetsImported branch from 11e0f93 to c8ba669 Compare June 25, 2021 22:10

abertelrud added the WIP Work in progress label Sep 8, 2021

artemcm force-pushed the VerifyOnlyDependencyTargetsImported branch from 54097aa to 2e620d1 Compare October 26, 2021 23:11

artemcm force-pushed the VerifyOnlyDependencyTargetsImported branch from 2e620d1 to 60bac7b Compare October 27, 2021 17:05

neonichu added the next waiting for next merge window label Apr 11, 2022

ktoso mentioned this pull request Jul 5, 2022

fix wrong cross module usage; seems this was not caught by CI / SwiftPM apple/swift-distributed-actors#981

Merged

artemcm added 5 commits July 6, 2022 10:29

Skip target-import dependency verification procedure and test, if the…

ef4b0c9

… host compiler does not support it.

Skip targets that contain or depend-on generated code, because genera…

08725a9

…ted source-code may not be available at the time of scan.

Add an opt-out from explicit target dependency import checking.

01eb92d

`--disable-explicit-target-dependency-import-checking` prevents the verification code from being run as a just-in-case option to opt-out from this warning

Flip the --disable-explicit-target-dependency-import-checking optio…

a4114eb

…n to an opt-in `enable` option. While we work on optimizing incremental build performance of this check, it would be nice to have it as an opt-in option.

artemcm force-pushed the VerifyOnlyDependencyTargetsImported branch from 60bac7b to a4114eb Compare July 6, 2022 17:50

Allow configuring the explicit import checking with either 'error', '…

e5020d8

…warning', or 'none' modes, with 'none' being the default.

neonichu approved these changes Jul 6, 2022

View reviewed changes

neonichu merged commit 2af09a8 into apple:main Jul 6, 2022

artemcm removed WIP Work in progress next waiting for next merge window labels Sep 28, 2022

mantuness mentioned this pull request Mar 6, 2023

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

Open

neonichu mentioned this pull request May 24, 2023

Missing diagnostic about missing dependency #6607

Open

MaxDesiatov reviewed Dec 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify that only targets that are manifest dependencies can be imported. #3562

Verify that only targets that are manifest dependencies can be imported. #3562

artemcm commented Jun 16, 2021 •

edited

artemcm commented Jun 25, 2021

tomerd commented Jun 25, 2021

neonichu left a comment

artemcm commented Jun 25, 2021

artemcm commented Jun 25, 2021

artemcm commented Jun 25, 2021

artemcm commented Jun 25, 2021

artemcm commented Jun 25, 2021

artemcm commented Jun 28, 2021

artemcm commented Jun 28, 2021

artemcm commented Jun 29, 2021

tomerd commented Jun 29, 2021

tomerd commented Jun 29, 2021

neonichu commented Jun 29, 2021 •

edited

artemcm commented Jun 30, 2021

ktoso commented Oct 6, 2021

artemcm commented Oct 26, 2021

ktoso commented Oct 27, 2021

artemcm commented Oct 27, 2021

artemcm commented Oct 27, 2021

artemcm commented Oct 27, 2021

artemcm commented Oct 27, 2021

ktoso commented Jul 5, 2022

SDGGiesbrecht commented Jul 5, 2022

artemcm commented Jul 6, 2022

ktoso commented Jul 7, 2022

MaxDesiatov Dec 11, 2023 •

edited

MaxDesiatov Dec 11, 2023

neonichu Dec 11, 2023

neonichu Dec 11, 2023

neonichu Dec 11, 2023

neonichu Dec 11, 2023

Verify that only targets that are manifest dependencies can be imported. #3562

Verify that only targets that are manifest dependencies can be imported. #3562

Conversation

artemcm commented Jun 16, 2021 • edited

artemcm commented Jun 25, 2021

tomerd commented Jun 25, 2021

neonichu left a comment

Choose a reason for hiding this comment

artemcm commented Jun 25, 2021

artemcm commented Jun 25, 2021

artemcm commented Jun 25, 2021

artemcm commented Jun 25, 2021

artemcm commented Jun 25, 2021

artemcm commented Jun 28, 2021

artemcm commented Jun 28, 2021

artemcm commented Jun 29, 2021

tomerd commented Jun 29, 2021

tomerd commented Jun 29, 2021

neonichu commented Jun 29, 2021 • edited

artemcm commented Jun 30, 2021

ktoso commented Oct 6, 2021

artemcm commented Oct 26, 2021

ktoso commented Oct 27, 2021

artemcm commented Oct 27, 2021

artemcm commented Oct 27, 2021

artemcm commented Oct 27, 2021

artemcm commented Oct 27, 2021

ktoso commented Jul 5, 2022

SDGGiesbrecht commented Jul 5, 2022

artemcm commented Jul 6, 2022

ktoso commented Jul 7, 2022

MaxDesiatov Dec 11, 2023 • edited

Choose a reason for hiding this comment

MaxDesiatov Dec 11, 2023

Choose a reason for hiding this comment

neonichu Dec 11, 2023

Choose a reason for hiding this comment

neonichu Dec 11, 2023

Choose a reason for hiding this comment

neonichu Dec 11, 2023

Choose a reason for hiding this comment

neonichu Dec 11, 2023

Choose a reason for hiding this comment

artemcm commented Jun 16, 2021 •

edited

neonichu commented Jun 29, 2021 •

edited

MaxDesiatov Dec 11, 2023 •

edited