[Serialization] Display contextual notes on deserialization errors and misconfigurations #66227

xymus · 2023-05-30T23:00:14Z

Since #65713 the Swift compiler raises a proper error instead of crashing when hitting a deserialization issue likely caused by broken modularization or project settings. This PR makes these diagnostics more helpful by providing contextual information:

Paths to the modules involved (up to 4 modules): the loaded swiftmodule with the broken outgoing reference, the path to the module where the decl was expected, the path to the underlying clang module, and the path to the module where the decl was found. This information should prevent us from having to ask for a different log with -Rmodule-loading.
Hint to delete the swiftmodule files when the module is library-evolution enabled.
Hint that clang settings affect clang modules involved in this scenario.
Pointing out when a decl moved between two modules with a similar name (where one name is a prefix of the other). This is a common issue when headers are shared between a clang framework's public and private modules.
Pointing out layering issues when an SDK module imports a local module.
Pointing out Swift language version mismatch which may lead to the compiler viewing the same clang decls differently when they are modified by APINotes files.

These notes are also display with -Rmodule-recovery (#66139) and when -experimental-force-workaround-broken-modules triggers and shows a warning (#66186).

There is room to improve those diagnostics further. We're considering bubbling up decls marked invalid instead of passing up errors in deserialization, this would allow for more complete synthesized decl when pointing to the final effect of the failure. We may also need to prioritize diagnosing these errors at a few sites where the compiler would still crash. We could also added services to the diagnostics engine to support one-line diagnostics pointing to binary files, without showing source code or the :1:1.

xymus · 2023-05-30T23:00:23Z

@swift-ci Please smoke test

xymus · 2023-05-30T23:02:35Z

test/Serialization/modularization-error.swift

+// CHECK-WORKAROUND: ^
+// CHECK-WORKAROUND: LibWithXRef.swiftmodule:1:1: note: attempting forced recovery enabled by -experimental-force-workaround-broken-modules
+// CHECK-WORKAROUND: <binary format>
+// CHECK-WORKAROUND: ^


@bnbarham I inverted the order back as we discussed. With the warning now showing the synthesized code I think this is more informative.

bnbarham · 2023-05-31T20:37:49Z

lib/Serialization/Deserialization.cpp

+  auto bufferID = SourceMgr.addMemBufferCopy("<binary format>", filename);
+  return SourceMgr.getLocForBufferStart(bufferID);


Won't this create a new buffer every time (same applies to all the other getSourceLoc's)? Having said that, see my comment below (re. whether we want location at all).

Yes. It's far from optimal but I don't think it's critical now since these buffers should only be allocated on failures or when remarks are enabled. We could reuse the buffers but I also want to investigate having diagnostics pointing to a file without a buffer at all, as not showing the <binary format> buffer content would be an improvement on its own.

If we do get a more optimized version, we could use it for existing services like extractNearestSourceLoc.

See my other comment. If you really want to keep, fair enough. It just seems like unknown makes more sense in these cases to me.

Though allocating one for every remark seems pretty bad to me 😅

bnbarham · 2023-05-31T20:40:07Z

lib/AST/Module.cpp

+  auto &SourceMgr = getASTContext().Diags.SourceMgr;
+  for (auto F : getFiles()) {
+    if (auto SF = dyn_cast<SourceFile>(F)) {
+      auto bufferID = SF->getBufferID();


When would this path actually be taken, wouldn't they all be LoadedFile?

I think SourceFile is a FileUnit but not a LoadedFile. Still, that branch may be dead code at this point as no diagnostics should point to the currently building module, unless something went really wrong in the dependency order which I wouldn't fully rule out.

I was mostly referring to your latter point. But fair enough 👍

bnbarham · 2023-05-31T20:45:53Z

test/Serialization/Recovery/module-recovery-remarks.swift

@@ -15,6 +15,45 @@

 /// Main error downgraded to a remark.
 // CHECK-MOVED: LibWithXRef.swiftmodule:1:1: remark: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'A_related'
+
+/// Contextual notes about the modules involved.
+// CHECK-MOVED: A.swiftmodule:1:1: note: declaration was expected to be found in this module


Would just having no location be better and then a note of eg. declaration was expected to be found in 'A.swiftmodule'?

We don't see it in the test but the full path is the main point of interest here. If we don't have the path on the left side we get the <unknown>.

How about ../../A.swiftmodule:1:1: declaration was expected to be found in module 'A' at this path?

In order of preference:

<unknown>: declaration was expected to be found in module 'A' at '../../A.swiftmodule' ../../A.swiftmodule: declaration was expected to be found in module 'A' ../../A.swiftmodule:1:1 declaration was expected to be found in module 'A' ../../A.swiftmodule:1:1 declaration was expected to be found in module 'A' at this path

My reasoning here is mostly that given this is more of a list of notes, the location at the start makes it a little confusing. Where as if they were all "unknown" you'd just skip over the unknown and then read the note.

If we go for the first alternative, that's much simpler to support as I could get rid of the new ModuleDecl::getSourceModuleLoc logic:

<unknown>: declaration was expected to be found in module 'A' at '../../A.swiftmodule'

This next alternative is what I'm considering doing next in a future PR, just pointing to the path without a fake buffer content. I think this would be clean and reminiscent of clang's diagnostic style:

../../A.swiftmodule: declaration was expected to be found in module 'A'

The last one is close to the current behavior from this PR. I see this as a step in the right direction but if it's not actually better than the <unknown> variant and we're worried about creating too many buffers, I am considering just skipping that step:

../../A.swiftmodule:1:1 declaration was expected to be found in module 'A'

I'd be curious to hear more opinions about this.

Yeah, I'll do that. It will allow me to deprioritize working on the diagnostic style and instead put more time on the content and making sure it's actually raised when we want it to.

bnbarham · 2023-05-31T20:47:12Z

test/Serialization/Recovery/module-recovery-remarks.swift

+
+/// More notes depending on the context
+// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: note: this module was built with a Swift language version set to 5
+// CHECK-MOVED: while the current invocation uses 4


I'm surprised this works. Shouldn't this need a CHECK-MOVED-SAME?

It worked but -SAME will be more clear on the intent, I'll update it.

bnbarham · 2023-05-31T20:49:32Z

test/Serialization/Recovery/module-recovery-remarks.swift

+// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: note: this module was built with a Swift language version set to 5
+// CHECK-MOVED: while the current invocation uses 4
+
+// CHECK-MOVED: LibWithXRef.swiftmodule:1:1: note: the module 'LibWithXRef' enabled library-evolution; this file may need to be deleted if the SDK was modified


Are these all notes of the previous remark?

Yes they are. This is the worst case scenario, there won't be that many notes in most cases.

Preserve more information about the context of modularization errors by replacing the module names with pointers to ModuleDecl and ModuleFile objects.

This service provides a path to the loaded swiftmodule file and synthesizes pseudo-Swift code representing the reference.

xymus · 2023-06-07T23:05:33Z

@swift-ci Please smoke test

xymus · 2023-06-07T23:08:27Z

Updated the text of a few diagnostics and removed the SourceLoc from the notes. Hopefully this makes the messages easier to read and understand. We may want to bring back the version with SourceLoc if we notice that users are confused as to where the actual problem is located.

Here's how it looks now for the case triggering the most notes in the test for remarks:

/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/sdk/LibWithXRef.swiftmodule:1:1: remark: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'A_related'
A.MyType
^
<unknown>:0: note: the type was expected to be found in module 'A' at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/A.swiftmodule'
<unknown>:0: note: or expected to be found in the underlying module 'A' defined at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/module.modulemap'
<unknown>:0: note: the type was actually found in module 'A_related' at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/A_related.swiftmodule'
<unknown>:0: note: the module 'LibWithXRef' was built with a Swift language version set to 5.9 while the current invocation uses 4.1.50; APINotes may change how clang declarations are imported
<unknown>:0: note: the module 'LibWithXRef' has enabled library-evolution; the following file may need to be deleted if the SDK was modified: '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/sdk/LibWithXRef.swiftmodule'
<unknown>:0: note: declarations in the underlying clang module 'A' may be hidden by clang project settings
<unknown>:0: note: the distributed module 'LibWithXRef' refers to the local module 'A'; this may be caused by clang project settings
<unknown>:0: note: the type 'MyType' moved between related modules; clang project settings may affect headers shared between these modules
/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/sdk/LibWithXRef.swiftmodule:1:1: note: could not deserialize type for 'foo()'

xymus · 2023-06-07T23:17:36Z

Actually we do want the SourceLoc for the could not deserialize type for 'foo()' note as it may point to a different modules than the original errors. I'll update the previous message accordingly.

@swift-ci Please smoke test

This PR makes diagnostics on deserialization errors caused by project configuration more helpful by providing contextual information on the issue: - Path to the modules involved (up to 4 modules): the loaded swiftmodule with the broken outgoing reference, the path to the module where the decl was expected, the path to the underlying clang module, and the path to the module where the decl was found. This information should prevent us from having to ask for a different log with `-Rmodule-loading`. - Hint to delete the swiftmodule files when the module is library-evolution enabled. - Hint that clang settings affect clang modules involved in this scenario. - Pointing out when a decl moved between two modules with a similar name (where one name is a prefix of the other). This is a common issue when headers are shared between a clang framework's public and private modules. - Pointing out layering issues when an SDK module imports a local module. - Pointing out Swift language version mismatch which may lead to the compiler viewing the same clang decls differently when they are modified by APINotes files.

xymus · 2023-06-08T16:24:33Z

@swift-ci Please smoke test

bnbarham · 2023-06-08T18:16:01Z

<unknown>:0: note: declarations in the underlying clang module 'A' may be hidden by clang project settings
<unknown>:0: note: the distributed module 'LibWithXRef' refers to the local module 'A'; this may be caused by clang project settings
<unknown>:0: note: the type 'MyType' moved between related modules; clang project settings may affect headers shared between these modules

clang project settings is referenced a few times there, but I assume the issue is that the last two notes may not actually appear all the time?

Would it make sense for

/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/sdk/LibWithXRef.swiftmodule:1:1: note: could not deserialize type for 'foo()'

to be the first note?

xymus · 2023-06-08T19:04:52Z

These 3 notes would rarely show at the same time but 2 are likely to be shown together, as the first one would be very common. They could be more specific to avoid the general clang project settings; for diag 1 it would be -D, diag 2 Header Maps or search paths, and diag 3 -D again.

I like keeping the current error first as it's the problem to fix, whereas note: could not deserialize type for 'foo()' is a side-effect but not the root cause. So if there's a bug report to file it would be on the site of the first error.

xymus · 2023-06-08T19:09:57Z

In most cases the root cause and the side-effects would still be in the same module, this makes my argument about the order less relevant. I'll give a try to move the side effect before the root cause and see the result.

… failure

xymus · 2023-06-08T21:43:25Z

@swift-ci Please smoke test

xymus · 2023-06-08T21:47:52Z

@bnbarham The last commit inverts the order of the diagnostics, making the side-effect the main anchor of the errors/warnings/remarks. It looks like so:

.../LibWithXRef.swiftmodule:1:1: remark: could not deserialize type for 'foo()'
<binary format>
^
.../LibWithXRef.swiftmodule:1:1: note: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'A_related'
A.MyType
^
<unknown>:0: note: the type was expected to be found in module 'A' at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/A.swiftmodule'
<unknown>:0: note: or expected to be found in the underlying module 'A' defined at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/module.modulemap'
<unknown>:0: note: the type was actually found in module 'A_related' at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/A_related.swiftmodule'
<unknown>:0: note: the module 'LibWithXRef' was built with a Swift language version set to 5.9 while the current invocation uses 4.1.50; APINotes may change how clang declarations are imported
<unknown>:0: note: the module 'LibWithXRef' has enabled library-evolution; the following file may need to be deleted if the SDK was modified: '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/sdk/LibWithXRef.swiftmodule'
<unknown>:0: note: declarations in the underlying clang module 'A' may be hidden by clang preprocessor macros
<unknown>:0: note: the distributed module 'LibWithXRef' refers to the local module 'A'; this may be caused by header maps or search paths
<unknown>:0: note: the type 'MyType' moved between related modules; clang preprocessor macros may affect headers shared between these modules

bnbarham · 2023-06-08T22:13:10Z

I like keeping the current error first as it's the problem to fix, whereas note: could not deserialize type for 'foo()' is a side-effect but not the root cause. So if there's a bug report to file it would be on the site of the first error.

The last commit inverts the order of the diagnostics, making the side-effect the main anchor of the errors/warnings/remarks. It looks like so:

Oh no 😅. To be clear - I meant first note, ie. I was expecting:

.../LibWithXRef.swiftmodule:1:1: remark: reference to type 'MyType' broken by a context change; 'MyType' was expected to be in 'A', but now a candidate is found only in 'A_related'
A.MyType
^
.../LibWithXRef.swiftmodule:1:1: note: could not deserialize type for 'foo()'
<unknown>:0: note: the type was expected to be found in module 'A' at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/A.swiftmodule'
<unknown>:0: note: or expected to be found in the underlying module 'A' defined at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/module.modulemap'
<unknown>:0: note: the type was actually found in module 'A_related' at '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/A_related.swiftmodule'
<unknown>:0: note: the module 'LibWithXRef' was built with a Swift language version set to 5.9 while the current invocation uses 4.1.50; APINotes may change how clang declarations are imported
<unknown>:0: note: the module 'LibWithXRef' has enabled library-evolution; the following file may need to be deleted if the SDK was modified: '/Volumes/Long/swift-main/build/Ninja-RelWithDebInfoAssert/swift-macosx-arm64/test-macosx-arm64/Serialization/Recovery/Output/module-recovery-remarks.swift.tmp/sdk/LibWithXRef.swiftmodule'
<unknown>:0: note: declarations in the underlying clang module 'A' may be hidden by clang preprocessor macros
<unknown>:0: note: the distributed module 'LibWithXRef' refers to the local module 'A'; this may be caused by header maps or search paths
<unknown>:0: note: the type 'MyType' moved between related modules; clang preprocessor macros may affect headers shared between these modules

xymus · 2023-06-09T16:19:18Z

Ah. I'll drop the last commit then. I think it could be good but it doesn't show as well currently, it would need improvements.

I wouldn't move up that note because I think the first few notes (about the involved files) are important as direct additional information to the remark. They even duplicate some of the information so we could simplify the remark using them. And mixing the side effect with the details on the root cause may quickly become confusing. We may want to change it from a note to a remark/warning/error to make it stand out more if needed.

xymus · 2023-06-09T16:22:06Z

@swift-ci Please smoke test

xymus requested review from bnbarham, elsh, artemcm and tshortli May 30, 2023 23:00

xymus requested review from zoecarver, hyp, egorzhdan, hborla, slavapestov and xedin as code owners May 30, 2023 23:00

xymus commented May 30, 2023

View reviewed changes

bnbarham reviewed May 31, 2023

View reviewed changes

xymus added 3 commits June 7, 2023 13:01

[Serialization] ModularizationError keeps full objects

2f03c95

Preserve more information about the context of modularization errors by replacing the module names with pointers to ModuleDecl and ModuleFile objects.

[Serialization] Intro ModularizationError::getSourceLoc

1cd4f4a

This service provides a path to the loaded swiftmodule file and synthesizes pseudo-Swift code representing the reference.

[Serialization] Invert order of diagnostics in forced recovery mode

b1e0b89

xymus force-pushed the recovery-notes branch from 0b6f61f to a7e62f7 Compare June 7, 2023 23:05

xymus force-pushed the recovery-notes branch from a7e62f7 to d034eab Compare June 7, 2023 23:16

xymus force-pushed the recovery-notes branch from d034eab to 9804075 Compare June 8, 2023 16:24

[Serialization] Be more specific irt which clang settings may cause a…

af88a65

… failure

xymus force-pushed the recovery-notes branch from fbacdea to af88a65 Compare June 9, 2023 16:21

xymus merged commit b18e7f1 into swiftlang:main Jun 9, 2023

xymus deleted the recovery-notes branch June 9, 2023 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serialization] Display contextual notes on deserialization errors and misconfigurations #66227

[Serialization] Display contextual notes on deserialization errors and misconfigurations #66227

xymus commented May 30, 2023 •

edited

Loading

xymus commented May 30, 2023

xymus May 30, 2023

bnbarham May 31, 2023

xymus Jun 1, 2023

xymus Jun 1, 2023

bnbarham Jun 1, 2023 •

edited

Loading

bnbarham May 31, 2023

xymus Jun 1, 2023

bnbarham Jun 1, 2023

bnbarham May 31, 2023

xymus Jun 1, 2023

bnbarham Jun 1, 2023

xymus Jun 2, 2023 •

edited

Loading

xymus Jun 2, 2023

bnbarham May 31, 2023

xymus Jun 1, 2023

bnbarham May 31, 2023

xymus Jun 1, 2023

xymus commented Jun 7, 2023

xymus commented Jun 7, 2023 •

edited

Loading

xymus commented Jun 7, 2023

xymus commented Jun 8, 2023

bnbarham commented Jun 8, 2023

xymus commented Jun 8, 2023

xymus commented Jun 8, 2023

xymus commented Jun 8, 2023

xymus commented Jun 8, 2023

bnbarham commented Jun 8, 2023

xymus commented Jun 9, 2023

xymus commented Jun 9, 2023

		auto bufferID = SourceMgr.addMemBufferCopy("<binary format>", filename);
		return SourceMgr.getLocForBufferStart(bufferID);

[Serialization] Display contextual notes on deserialization errors and misconfigurations #66227

[Serialization] Display contextual notes on deserialization errors and misconfigurations #66227

Conversation

xymus commented May 30, 2023 • edited Loading

xymus commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnbarham Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xymus Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xymus commented Jun 7, 2023

xymus commented Jun 7, 2023 • edited Loading

xymus commented Jun 7, 2023

xymus commented Jun 8, 2023

bnbarham commented Jun 8, 2023

xymus commented Jun 8, 2023

xymus commented Jun 8, 2023

xymus commented Jun 8, 2023

xymus commented Jun 8, 2023

bnbarham commented Jun 8, 2023

xymus commented Jun 9, 2023

xymus commented Jun 9, 2023

xymus commented May 30, 2023 •

edited

Loading

bnbarham Jun 1, 2023 •

edited

Loading

xymus Jun 2, 2023 •

edited

Loading

xymus commented Jun 7, 2023 •

edited

Loading