[ClangImporter] Fix import of aliased enum cases #80557
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a C enum has multiple constants with the same value, ClangImporter selects one of them to import as an `EnumElementDecl` and imports the others as `VarDecl` “aliases” of that one. This helps preserve the invariant that each case of an enum has a unique raw value and is distinct for exhaustiveness checking. However, a bug in that logic could sometimes cause the canonical case to be imported *twice*—once as an `EnumElementDecl` and again as a `VarDecl` alias. In this situation, the `EnumElementDecl` was not added to the enum’s member list, resulting in a malformed AST. This bug has apparently been present since early 2017 (!), but it seems to have been harmless until recently, when the `ComputeSideEffects` SIL pass recently became sensitive to it (probably because of either swiftlang#79872 or swiftlang#80263). Correct this problem by modifying the memoization logic to retrieve the canonical case’s clang-side constant so the subsequent check will succeed. Additionally change a variable name and add comments to help clarify this code for future maintainers. In lieu of adding new unit tests, this commit adds a (slightly expensive) conditional assertion to catch this kind of AST malformation. There are actually about twenty tests that will fail with just the assertion and not the fix, but I’ve updated one of them to enable the assertion even in release mode. Fixes rdar://148213237. Followup to swiftlang#80487, which added related assertions to the SIL layer.
beccadax
requested review from
zoecarver,
egorzhdan,
ian-twilightcoder,
Xazax-hun,
j-hui,
fahadnayyar and
susmonteiro
as code owners
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
(Testing source compatibility to check this change against more surface area.) |
nkcsgexi
approved these changes
Apr 5, 2025
Xazax-hun
approved these changes
Apr 6, 2025
|
@swift-ci please test Linux platform |
This was referenced Apr 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a C enum has multiple constants with the same value, ClangImporter selects one of them to import as an
EnumElementDecland imports the others asVarDecl“aliases” of that one. This helps preserve the invariant that each case of an enum has a unique raw value and is distinct for exhaustiveness checking.However, a bug in that logic could sometimes cause the canonical case to be imported twice—once as an
EnumElementDecland again as aVarDeclalias. In this situation, theEnumElementDeclwas not added to the enum’s member list, resulting in a malformed AST. This bug has apparently been present since early 2017 (!), but it seems to have been harmless until recently, when theComputeSideEffectsSIL pass recently became sensitive to it (probably because of either #79872 or #80263).Correct this problem by modifying the memoization logic to retrieve the canonical case’s clang-side constant so the subsequent check will succeed. Additionally change a variable name and add comments to help clarify this code for future maintainers.
In lieu of adding new unit tests, this commit adds a (slightly expensive) conditional assertion to catch this kind of AST malformation. There are actually about twenty tests that will fail with just the assertion and not the fix, but I’ve updated one of them to enable the assertion even in release mode.
Fixes rdar://148213237. Followup to #80487, which added related assertions to the SIL layer.