From 86704e35e1b3f76965c8c83e1c9e0edd789a1d54 Mon Sep 17 00:00:00 2001 From: Becca Royal-Gordon Date: Fri, 4 Apr 2025 18:54:36 -0700 Subject: [PATCH] [ClangImporter] Fix import of aliased enum cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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. --- lib/ClangImporter/ImportDecl.cpp | 50 ++++++++++++++++++++++++++++---- test/ClangImporter/enum.swift | 6 ++-- 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/lib/ClangImporter/ImportDecl.cpp b/lib/ClangImporter/ImportDecl.cpp index 6bd63bda9938f..94c34719d0f0a 100644 --- a/lib/ClangImporter/ImportDecl.cpp +++ b/lib/ClangImporter/ImportDecl.cpp @@ -1864,10 +1864,28 @@ namespace { break; } + /// A table mapping each raw value used in this enum to the clang or + /// Swift decl for the "canonical" constant corresponding to that raw + /// value. The clang decls represent cases that haven't yet been imported; + /// the Swift decls represent cases that have been imported before. + /// + /// The problem we are trying to solve here is that C allows several + /// constants in the same enum to have the same raw value, but Swift does + /// not. We must therefore resolve collisions by selecting one case to be + /// the "canonical" one that will be imported as an \c EnumElementDecl + /// and importing the others as static \c VarDecl aliases of it. This + /// map knows which constants are canonical and can map a constant's raw + /// value to its corresponding canonical constant. + /// + /// Note that unavailable constants don't get inserted into this table, + /// so if an unavailable constant has no available alias, it simply won't + /// be present here. (Potential raw value conflicts doesn't really matter + /// for them because they will be imported as unavailable anyway.) llvm::SmallDenseMap, 8> canonicalEnumConstants; + // Fill in `canonicalEnumConstants` if it will be used. if (enumKind == EnumKind::NonFrozenEnum || enumKind == EnumKind::FrozenEnum) { for (auto constant : decl->enumerators()) { @@ -1942,24 +1960,32 @@ namespace { SwiftDeclConverter(Impl, getActiveSwiftVersion()) .importEnumCase(constant, decl, cast(result)); } else { - const clang::EnumConstantDecl *unimported = + // Will initially be nullptr if `canonicalCaseIter` points to a + // memoized result. + const clang::EnumConstantDecl *canonConstant = canonicalCaseIter-> second.dyn_cast(); - // Import the canonical enumerator for this case first. - if (unimported) { + // First, either import the canonical constant for this case, + // or extract the memoized result of a previous import (and use it + // to populate `canonConstant`). + if (canonConstant) { enumeratorDecl = SwiftDeclConverter(Impl, getActiveSwiftVersion()) - .importEnumCase(unimported, decl, cast(result)); + .importEnumCase(canonConstant, decl, cast(result)); if (enumeratorDecl) { + // Memoize so we end up in the `else` branch next time. canonicalCaseIter->getSecond() = cast(enumeratorDecl); } } else { enumeratorDecl = canonicalCaseIter->second.get(); + canonConstant = + cast(enumeratorDecl->getClangDecl()); } - if (unimported != constant && enumeratorDecl) { + // If `constant` wasn't the `canonConstant`, import it as an alias. + if (canonConstant != constant && enumeratorDecl) { ImportedName importedName = Impl.importFullName(constant, getActiveSwiftVersion()); Identifier name = importedName.getBaseIdentifier(Impl.SwiftContext); @@ -1975,6 +2001,7 @@ namespace { } } + // Now import each of the constant's alternate names. Impl.forEachDistinctName(constant, [&](ImportedName newName, ImportNameVersion nameVersion) -> bool { @@ -2025,6 +2052,19 @@ namespace { } } + // We don't always add an imported canonical constant to the enum's + // members right away, but we should have by the time we leave the loop. + // Verify that they are all in the enum's member list. (rdar://148213237) + if (CONDITIONAL_ASSERT_enabled()) { + for (const auto &entry : canonicalEnumConstants) { + auto importedCase = entry.second.dyn_cast(); + if (!importedCase) + continue; + + ASSERT(llvm::is_contained(result->getCurrentMembers(), importedCase)); + } + } + return result; } diff --git a/test/ClangImporter/enum.swift b/test/ClangImporter/enum.swift index 3a9d2077d5392..4184e2391f75c 100644 --- a/test/ClangImporter/enum.swift +++ b/test/ClangImporter/enum.swift @@ -1,7 +1,7 @@ -// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck %s -verify -// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck %s 2>&1 | %FileCheck %s +// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck %s -verify -compiler-assertions +// RUN: not %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck %s -compiler-assertions 2>&1 | %FileCheck %s // -- Check that we can successfully round-trip. -// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -D IRGEN -emit-ir -primary-file %s | %FileCheck -check-prefix=CHECK-IR %s +// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -D IRGEN -emit-ir -primary-file %s -compiler-assertions | %FileCheck -check-prefix=CHECK-IR %s // REQUIRES: objc_interop