[cxx-interop] Fix assertion failure from unavailable typedef + enum pattern #81625
Conversation
…nction
Also renames it to findOptionSetEnum() to make it a bit clearer at face
value that the returned ImportedType will contain a Swift enum.
Also refactors some nearby instances of
if (auto e = dyn_cast<ElaboratedType>(t))
t = e->desugar();
into a helper function, desugarIfElaborated().
…EnumInfo.cpp It's at home there alongside other ObjC enum-specific logic, rather than in the middle of ImportDecl.cpp (since it isn't directly or exclusively related to importing decls).
|
Draft PR with only NFC commits for now, to run under CI |
|
@swift-ci please test |
|
macOS tests are still hanging but Linux + Windows have passed, so it seems I didn't accidentally break anything with the [NFC] commits. Pushing actual patch + opening the PR now |
j-hui
force-pushed
the
fix-unavailable-enum
branch
from
bea20a2 to
75e4f04
Compare
j-hui
requested review from
zoecarver,
egorzhdan,
beccadax,
ian-twilightcoder,
Xazax-hun,
fahadnayyar and
susmonteiro
as code owners
|
@egorzhdan for what it's worth, I tried explicitly checking for For now, 75e4f04 keeps the behavior consistent with what it was before, and simply adjusts the assertion to accommodate |
|
@swift-ci please test |
|
I seem to have just missed #81636. Restarting tests |
|
@swift-ci please test |
j-hui
force-pushed
the
fix-unavailable-enum
branch
from
75e4f04 to
a9b744e
Compare
|
@swift-ci please test |
|
@egorzhdan thanks for the suggestions, I took both of them. Do you have thoughts on checking for expansion of the |
|
@swift-ci please test |
The NS_OPTIONS macro sometimes uses a pattern where it loosely associates a typedef with an anonymous enum via a shared underlying integer type (emphasis on "loosely"). The typedef is marked as unavailable in Swift so as to not cause name ambiguity when we associate the anonymous enum with said typedef. We use unavailability as a heuristic during the import process, but that conflates NS_OPTIONS with NS_ENUMs that can be marked as unavailable for entirely unrelated reasons. That in and of itself is fine, because the import logic is general enough to handle both cases, but we have an assertion that seems to be unaware of this scenario, and trips on unavailable NS_ENUMs. (In those cases, the typedef points to the enum rather than the underlying integer type.) This patch fixes the assertion to be resilient against such cases by looking through the enum a typedef refers to. rdar://150399978
j-hui
force-pushed
the
fix-unavailable-enum
branch
from
a9b744e to
fa5c4f2
Compare
|
@swift-ci please test |
importer::findOptionSetEnum() uses some fragile heuristics to determine
whether a typedef is involved in the construction of a CF_OPTIONS or
NS_OPTIONS type. This patch adds an explicit check that the typedef is
expanded from either of those macros, to prevent, e.g., an unavailable
NS_ENUM, from being mistakenly recognized as an NS_OPTIONS.
Note that doing this is still kind of fragile, and prevents users from
building {NS,CF}_OPTIONS with their own macros. The right thing to do is
probably specifically look for the flag_enum attribute, but that is not
currently what we're doing for reasons whose discovery is left as
an exercise to the future git archaeologist.
This patch also removes (part of) a test case that builds
a CF_OPTIONS-like type with the "SOME_OPTIONS" macro, which is no longer
supported as of this patch.
rdar://150399978
|
@swift-ci please test |
|
@swift-ci please test windows platform |
…-enum" This reverts commit 828876f.
The
NS_OPTIONSmacro sometimes uses a pattern where it looselyassociates a typedef with an anonymous enum via a shared underlying
integer type (emphasis on "loosely"). The typedef is marked as
unavailable in Swift so as to not cause name ambiguity when we associate
the anonymous enum with said typedef. We use unavailability as
a heuristic during the import process, but that conflates
NS_OPTIONSwith
NS_ENUMs that can be marked as unavailable for entirely unrelatedreasons.
That in and of itself is fine, because the import logic is general
enough to handle both cases, but we have an assertion that seems to be
unaware of this scenario, and trips on unavailable
NS_ENUMs. (In thosecases, the typedef points to the enum rather than the underlying integer
type.) This patch fixes the assertion to be resilient against such cases
by looking through the enum a typedef refers to.
This PR also includes some NFC commits which consolidate usage of the
findOptionSetEnum()helper function. This pattern (and the failing assertion) was previously duplicated across six distinct locations acrossImportDecl.cppandImportType.cpp. The interface and behavior of this function is based onfindOptionSetType(), which was previously defined inImportDecl.cppbut also used inImportType.cppvia an ad hoc function declaration.Those NFC commits make way for 80db054, where the actual fix is contained to a fairly small change.
rdar://150399978
Resolves #79636