Emit diagnostics/remarks out of wrapped ModularizationError #68525
Conversation
|
CC @compnerd |
|
@swift-ci please test |
| @@ -0,0 +1,9 @@ | |||
| // RUN: %empty-directory(%t) | |||
| // RUN: %target-swift-frontend -emit-module %S/Inputs/wrapped-modularization-error-remarks/A/A.swift -cxx-interoperability-mode=default -Xcc -fmodule-map-file=%S/Inputs/wrapped-modularization-error-remarks/CxxLib/include/module.modulemap -Xcc -I -Xcc %S/Inputs/wrapped-modularization-error-remarks/CxxLib/include -module-name A -o %t/A.swiftmodule | |||
| // RUN: not %target-swift-frontend -c -primary-file %S/Inputs/wrapped-modularization-error-remarks/B/B.swift -cxx-interoperability-mode=default -I %t -Xcc -fmodule-map-file=%S/Inputs/wrapped-modularization-error-remarks/CxxLib/include/module.modulemap -Xcc -I -Xcc %S/Inputs/wrapped-modularization-error-remarks/CxxLib/include -module-name B -o %t/B.swift.o -Rmodule-recovery 2>&1 | %FileCheck %s | |||
There was a problem hiding this comment.
What is the underlying problem here? I'm not very familiar with the C++ interior and can't tell what could be the context change between the two commands could lead to _GUID being lost.
I'm curious as I'm likely to screen issues when they are reported by serialization errors.
There was a problem hiding this comment.
I would also like to know if this issue is more of a compiler bug that could be fixed in the future and break the test. It's always a challenge to test the deserialization recovery logic, to find bugs we don't intend to fix to trigger the crash.
There was a problem hiding this comment.
The underlying problem is referenced in #68362 which is an issue we had on windows. I'm not sure if this is a compiler bug or a modulemap file issue. The issue may be fixed in the future and the test may break as the situation seems to be something that may be expected to work. But I'm not sure if I can predict what would happen in reality. This PR intends to emit diagnostics around the issue that help with better diagnosis.
| assert(context.LangOpts.EnableDeserializationRecovery); | ||
| auto handleModularizationError = [&](ModularizationError &error) | ||
| -> llvm::Error { | ||
| if (context.LangOpts.EnableModuleRecoveryRemarks) |
There was a problem hiding this comment.
I really like this as an expansion to EnableModuleRecoveryRemarks. Just so you know, we may turn it on by default in the future in some contexts. It currently reports both expected and unexpected errors, but when we start preventing the expected errors earlier we could start reporting only the unexpected errors.
There was a problem hiding this comment.
Good to know about the chance that the flag may be enabled by default.
The diagnostics/remarks out of the ModularizationError wrapped in a TypeError (eg. coming from resolveCrossReference) is otherwise just dropped but could help better understand C/C++ interop issues.
hjyamauchi
force-pushed
the
wrapped-modularization-error-remarks
branch
from
ba75256 to
c5dc68a
Compare
|
PTAL. Updated to limit the test to Windows as it seems that the underlying error happens only on Windows and the test doesn't fail on Mac/Linux. |
|
@swift-ci please test |
The diagnostics/remarks out of the ModularizationError wrapped in a TypeError (eg. coming from resolveCrossReference) is otherwise just dropped but could help better understand C/C++ interop issues.