Warn if a package binary module is loaded from SDK #64600

elsh · 2023-03-24T02:15:09Z

Warns if a module loaded is a package module but not locally built.
Update a related diagnostic
Resolves rdar://107074380

elsh · 2023-03-24T02:17:57Z

xymus · 2023-03-24T16:11:02Z

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -1960,9 +1960,23 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
    // diagnostics.
    (void)ID->getDecls();

+    auto target = ID->getModule();
+    if (target->isNonUserModule() && // target module is in distributed SDK
+        getASTContext().LangOpts.PackageName == target->getPackageName().str()) {


Should we check that the package name is not empty to avoid this warning between two modules without a package name?

good point, will update

xymus · 2023-03-24T16:13:40Z

test/Sema/import_package_module_from_sdk.swift

+
+// RUN: %target-swift-frontend -typecheck %t/Client1.swift -package-name libPkg -sdk %t/SDK -I %t -I %t/SDK/Frameworks/LibInSDK.framework/Modules 2> %t/result.output
+// RUN: %FileCheck %s < %t/result.output
+// CHECK: warning: module 'LibInSDK' is in package 'libPkg' but was loaded from SDK; modules of the same package are expected to be loaded from local build directory: {{.*}}/SDK/Frameworks/LibInSDK.framework/Modules/LibInSDK.swiftmodule/{{.*}}.swiftmodule


Since that should be on the import itself you could use -verify mode to check the diagnostic.

xymus · 2023-03-24T16:15:21Z

cc @bnbarham You may like this, we're using the package knowledge and isNonUserModule to report if a module from the same project is loaded from a distributed source instead of a local build.

bnbarham · 2023-03-24T16:44:08Z

test/Sema/import_package_module_from_sdk.swift

+
+// RUN: %target-swift-frontend -typecheck %t/Client1.swift -package-name libPkg -sdk %t/SDK -I %t -I %t/SDK/Frameworks/LibInSDK.framework/Modules 2> %t/result.output
+// RUN: %FileCheck %s < %t/result.output
+// CHECK: warning: module 'LibInSDK' is in package 'libPkg' but was loaded from SDK; modules of the same package are expected to be loaded from local build directory: {{.*}}/SDK/Frameworks/LibInSDK.framework/Modules/LibInSDK.swiftmodule/{{.*}}.swiftmodule


modules of the same package are expected to be loaded from local build directory

Adds confusion IMO. It's not strictly true, since I could have various build directories and that would still be fine. It also begs the question as to what is the "local build directory" 😅.

I think just the prior warning is good enough (ie. module 'mod' is in package 'package' but was loaded from the SDK). Or alternatively SDK module 'mod' should not be included in user-defined package 'package', which IIUC is more what the warning is about?

updated the confusing part in the original message as "locally built from source only"

bnbarham · 2023-03-24T16:52:07Z

test/Serialization/load_package_module.swift

@@ -9,7 +9,7 @@

 // RUN: not %target-swift-frontend -module-name ClientInSamePkg %t/ClientLoadInterfaceModule.swift -emit-module -emit-module-path %t/ClientInSamePkg.swiftmodule -package-name mypkg -I %t 2> %t/resultA.output
 // RUN: %FileCheck %s -check-prefix CHECK-A < %t/resultA.output
-// CHECK-A: error: module 'LibFromInterface' is in package 'mypkg' but was built from interface '{{.*}}LibFromInterface.swiftinterface'; modules of the same package can only be loaded if built from source
+// CHECK-A: error: module 'LibFromInterface' is in package 'mypkg' but was built from interface; modules of the same package can only be loaded if built from source: {{.*}}LibFromInterface.swiftinterface


I feel similar about this error message as the one above, but I'm less sure of another way to word it. As a user I'm just not sure this would mean much to me. Really I want to know what I've done wrong. Maybe this is fine though since IIUC it would be fairly uncommon for a regular user to hit this.

bnbarham · 2023-03-24T16:55:12Z

cc @bnbarham You may like this, we're using the package knowledge and isNonUserModule to report if a module from the same project is loaded from a distributed source instead of a local build.

Nice :)

nkcsgexi · 2023-03-24T17:45:42Z

include/swift/AST/DiagnosticsSema.def

-ERROR(in_package_module_not_compiled_from_source,Fatal,
-      "module %0 is in package '%1' but was built from interface '%2'; "
-      "modules of the same package can only be loaded if built from source",
+ERROR(in_package_module_not_compiled_from_source,none,


Do we use this error somewhere?

Yes, it was already added to SerializationLoader::loadAST in main; its message is just updated in this PR.

nkcsgexi

Issuing a warning for this case makes sense to me👍

elsh · 2023-03-24T23:09:53Z

@swift-ci smoke test

elsh · 2023-03-25T00:13:38Z

@swift-ci clean smoke test linux

elsh · 2023-03-25T08:43:00Z

@swift-ci smoke test

Resolves rdar://107074380

elsh · 2023-03-26T00:10:24Z

@swift-ci smoke test

elsh · 2023-03-26T08:10:25Z

@swift-ci test

elsh requested review from hborla, slavapestov and xedin as code owners March 24, 2023 02:15

elsh requested review from xymus and nkcsgexi March 24, 2023 02:15

elsh force-pushed the es-warn branch 2 times, most recently from 5fe187f to 3bcd726 Compare March 24, 2023 02:17

xymus reviewed Mar 24, 2023

View reviewed changes

xymus approved these changes Mar 24, 2023

View reviewed changes

bnbarham reviewed Mar 24, 2023

View reviewed changes

nkcsgexi reviewed Mar 24, 2023

View reviewed changes

Warn if a package binary module is loaded from SDK

f87ba95

Resolves rdar://107074380

elsh force-pushed the es-warn branch from fe2ca4c to f87ba95 Compare March 26, 2023 00:09

elsh merged commit d900706 into main Mar 26, 2023

elsh deleted the es-warn branch March 26, 2023 22:31

Warn if a package binary module is loaded from SDK #64600

Warn if a package binary module is loaded from SDK #64600

Uh oh!

Conversation

elsh commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elsh commented Mar 24, 2023

Uh oh!

xymus Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

elsh Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

xymus Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

xymus commented Mar 24, 2023

Uh oh!

bnbarham Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

elsh Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnbarham Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

bnbarham commented Mar 24, 2023

Uh oh!

nkcsgexi Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

elsh Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkcsgexi left a comment

Choose a reason for hiding this comment

Uh oh!

elsh commented Mar 24, 2023

Uh oh!

elsh commented Mar 25, 2023

Uh oh!

elsh commented Mar 25, 2023

Uh oh!

elsh commented Mar 26, 2023

Uh oh!

elsh commented Mar 26, 2023

Uh oh!

Uh oh!

elsh commented Mar 24, 2023 •

edited

Loading

elsh Mar 24, 2023 •

edited

Loading

elsh Mar 24, 2023 •

edited

Loading