When adding an imported C++ name, make sure that its clang::Decl is mapped if import failed
#5769
Conversation
… mapped if import failed When mapping parameter types, we assume that if the `clang::Decl` isn't mapped, the name wasn't added, so this fixes a bug that triggers a crash otherwise.
toolchain/check/import_cpp.cpp
Outdated
| if (inst_id.has_value()) { | ||
| context.name_scopes().AddRequiredName(scope_id, name_id, inst_id); | ||
| if (inst_id == SemIR::ErrorInst::InstId) { | ||
| context.sem_ir().clang_decls().Add( |
There was a problem hiding this comment.
I am looking at the other calls to clang_decls().Add() and I don't see any case where we early out when we see ErrorInst, so I would expect any inst_id to already be added through the normal way if the inst_id is ErrorInst - can you help me understand why that would not be happening?
There was a problem hiding this comment.
Sure.
MapRecordType() -> ImportCXXRecordDecl() -?> BuildClassDefinition() -> clang_decls().Add().
MapRecordType() -> AddNameToScope()
Before this change, we could call AddNameToScope() and not add it to the decl. This happens in MapRecordType() where the name is added after calling ImportCXXRecordDecl(). ImportCXXRecordDecl() has a couple of use cases where it won't call BuildClassDefinition(), which indeed always adds the declaration to clang_decls.
FWIW, I've made sure decl isn't in clang_decls in this case, and is otherwise, and I can add CARBON_CHECK is you think it's useful.
There was a problem hiding this comment.
Thanks. So then I wonder if we need this mapping from the decl to errorinst when ImportCXXRecordDecl() returns ErrorInst, could it do that instead of doing it more lazily in MapRecordType? That way ImportCXXRecordDecl() could establish an invariant (and we can document it) that the decl given to it will always be added to the clang_decls?
There was a problem hiding this comment.
Yes, we can definitely do that.
However, it means doing more complex work in ImportCXXRecordDecl() and in any future declaration type we will import that might fail on import, and I thought we can reduce duplication of that logic by doing it here.
I do see the benefit of making it clear that we always add it to clang_decls in ImportCXXRecordDecl(), but lean towards avoid logic duplication.
For example, ImportNamespaceDecl() currently always does it because it never fails, but if we add a failure case we will have to add logic that adds it to clang_decls, and extrapolating to many other decls we will support, this might make things more complex than necessary.
Given that every name added should also have a decl mapped I think the logic makes sense.
I don't feel strongly about this so up to you.
WDYT?
There was a problem hiding this comment.
Thanks for the extra info. I think that combined with the idea that we're going to be reducing the number of ErrorInsts that can occur over time (by handing more/all C++ cases), I am less worried about making the handling of ErrorInst do more work, and would prefer to limit the amount of layers of code that have to think about ErrorInst instead. So I would prefer to do this in ImportCXXRecordDecl then.
| struct S; | ||
|
|
||
| auto foo1(S) -> void; | ||
| auto foo2(S) -> void; |
There was a problem hiding this comment.
Nothing about these tests seems to be intentionally causing an ErrorInst to be imported - perhaps they cause one now due to lack of support, but they won't indefinitely right? Could we produce a test that will always intentionally produce an ErrorInst on import? Or is that impossible because it would have to produce a clang error and then we wouldn't import it?
There was a problem hiding this comment.
Yes, not sure, and yes.
While we currently lack any support for declared but not defined structs, I'm not sure whether we plan to support calling a C++ function that takes declared but not defined structs by value.
It's possible the code would change to work differently by that time.
Given that these are integration tests and not unit tests, I'm not sure we should focus on how we import ErrorInst, as this is tied to the implementation, but rather make sure we cover the use cases to make sure behavior is as expected. These use cases are the most concise use cases to I found to trigger those crashes, to demonstrate the fix and make sure we won't have regressions for these use cases.
There was a problem hiding this comment.
Given that these are integration tests and not unit tests, I'm not sure we should focus on how we import ErrorInst, as this is tied to the implementation
FWIW we do test that ErrorInst is imported and handled correctly from Carbon imports. For example this test imports an ErrorInst and demonstrates it's imported okay by not crashing and showing the ErrorInst in the resulting semir.
I am thinking if we should try to ensure we have something like that for C++ imports too. But it sounds like we won't have a durable way to produce ErrorInst from C++. So I guess we can't then.
There was a problem hiding this comment.
Given that these are integration tests and not unit tests, I'm not sure we should focus on how we import ErrorInst, as this is tied to the implementation
FWIW we do test that ErrorInst is imported and handled correctly from Carbon imports. For example this test imports an ErrorInst and demonstrates it's imported okay by not crashing and showing the ErrorInst in the resulting semir.
Sure, use case wise we can test something similar, but we know, it won't catch that case because it will either fail in the C++ compilation part or in the Carbon part, but not in the interop part.
I am thinking if we should try to ensure we have something like that for C++ imports too. But it sounds like we won't have a durable way to produce ErrorInst from C++. So I guess we can't then.
I'm not aware of a way, but I'm open to ideas.
…he import fails instead of doing that in `AddNameToScope()`.
When mapping parameter types, we assume that if the
clang::Declisn't mapped, the name wasn't added, so this fixes a bug that triggers a crash otherwise.Part of #5533.