[ClangImporter] Fix IUO ordering bug #70106
Conversation
The IUO-ness of imported declarations is not actually computed by IsImplicitlyUnwrappedOptionalRequest. Instead, ClangImporter manually sets the bit to `true` after the declaration’s type is imported and expects IsImplicitlyUnwrappedOptionalRequest to always set it to `false` for all other imported declarations. Normally, declaration types are imported greedily as soon as the declaration is created. However, a ClangImporter refactoring in swiftlang#61026 deferred the import of a VarDecl’s type, and therefore the setting of its IUO bit, until the first time InterfaceTypeRequest is evaluated. It turns out that there is nothing to guarantee that InterfaceTypeRequest will be evaluated before IsImplicitlyUnwrappedOptionalRequest, so if isImplicitlyUnwrappedOptional() was fetched before getInterfaceType() was called, it would return an incorrect result. The only known client that accesses the information in this order is the API digester, but in theory any part of the compiler could fall into this trap. Force the evaluation of InterfaceTypeRequest during IsImplicitlyUnwrappedOptionalRequest when necessary to compute the IUO bit for an imported VarDecl, and add a test to prove that this fixes the observed bug in the API digester.
|
@zoecarver If you can think of a better fix, suggestions welcome. |
|
@swift-ci please test |
It sounds like we were a bit sloppy with the original request conversion here. Would it be too difficult to fix this the right way? I know it's not your fault it's like this.
This is true of all requests, which is why the evaluation function should not have observable side effects like this. |
True. ClangImporter has a bad—well, I guess really it's just old—habit of manually computing and pre-populating values that are normally computed by a request that only works for Swift code; that's usually acceptable because it sets the values right after it creates the AST nodes, but here some of that work was deferred, so it became possible to execute other code before the field was populated.
The tricky thing is that, because of how I don't love this, though, so if anyone has a better idea, I'd love to hear it. |
There was a problem hiding this comment.
The tricky thing is that, because of how importType() works, we can't compute the IUO-ness of a decl without also computing enough information to determine its interface type.
Looking at usages of OTK_ImplicitlyUnwrappedOptional in ImportType.cpp, it might be theoretically possible to untangle them with some minimal amount of duplication. After all, ImplicitlyUnwrappedOptionalRequest also does this to Swift AST in some sense, because it looks at TypeReprs just like InterfaceTypeRequest ultimately looks at TypeReprs.
It might even make sense to just always have InterfaceTypeRequest return a type/IUO pair.
But both of those are much larger changes than what I was imagining, so your narrow fix to force the request for its side effect is fine. Hopefully we can sort it out eventually!
|
... ok, one last idea: since the importType() lazyness only applies to VarDecls, it might not be too bad to figure out how to extract just that part? |
|
I've looked at the IUO logic in ImportType.cpp and it seems difficult to disentangle from the I think I'm going to go with what I have for now. |
|
@swift-ci please test and merge |
|
@swift-ci smoke test |
|
@swift-ci smoke test and merge |
|
@swift-ci smoke test and merge |
1 similar comment
|
@swift-ci smoke test and merge |
|
One more try. @swift-ci smoke test and merge |
|
@swift-ci please clean smoke test macOS platform |
|
@swift-ci please clean smoke test Linux platform |
|
@swift-ci smoke test |
The IUO-ness of imported declarations is not actually computed by IsImplicitlyUnwrappedOptionalRequest. Instead, ClangImporter manually sets the bit to
trueafter the declaration’s type is imported and expects IsImplicitlyUnwrappedOptionalRequest to always set it tofalsefor all other imported declarations.Normally, declaration types are imported greedily as soon as the declaration is created. However, a ClangImporter refactoring in #61026 deferred the import of a VarDecl’s type, and therefore the setting of its IUO bit, until the first time InterfaceTypeRequest is evaluated.
It turns out that there is nothing to guarantee that InterfaceTypeRequest will be evaluated before IsImplicitlyUnwrappedOptionalRequest, so if isImplicitlyUnwrappedOptional() was fetched before getInterfaceType() was called, it would return an incorrect result. The only known client that accesses the information in this order is the API digester, but in theory any part of the compiler could fall into this trap.
Force the evaluation of InterfaceTypeRequest during IsImplicitlyUnwrappedOptionalRequest when necessary to compute the IUO bit for an imported VarDecl, and add a test to prove that this fixes the observed bug in the API digester.
Cherry-picked out of #69928.