s/LookupResult/std::optional<IR::Item>/ (errors => UnsupportedItem). This CL replaces and deletes the Importer::LookupResult class with std::optional<IR::Item>. This CL translates the old LookupResult values as follows: 1. LookupResult() => std::nullopt 2. LookupResult(IR::Item) => std::optional(IR::Item) 3. LookupResult(std::string error) => std::optional(IR::UnsupportedItem(...)) This refactoring has been done when working on b/226211490, but it is not strictly required to address this bug. PiperOrigin-RevId: 436725428
diff --git a/rs_bindings_from_cc/importer.cc b/rs_bindings_from_cc/importer.cc index 5096f26..0de6f11 100644 --- a/rs_bindings_from_cc/importer.cc +++ b/rs_bindings_from_cc/importer.cc
@@ -13,6 +13,7 @@ #include <string> #include <tuple> #include <utility> +#include <variant> #include <vector> #include "third_party/absl/container/flat_hash_map.h" @@ -280,8 +281,8 @@ } // ... and then we remove those that "conflict" with an IR item. - for (const auto& [decl, result] : lookup_cache_) { - if (result.item()) { + for (const auto& [decl, result] : import_cache_) { + if (result.has_value()) { // Remove doc comments of imported items. if (auto raw_comment = ctx_.getRawCommentForDeclNoCache(decl)) { ordered_comments.erase(raw_comment->getBeginLoc()); @@ -382,26 +383,16 @@ // We emit IR items in the order of the decls they were generated for. // For decls that emit multiple items we use a stable, but arbitrary order. std::vector<OrderedItem> items; - for (const auto& [decl, result] : lookup_cache_) { - auto item = result.item(); - if (item) { + for (const auto& [decl, item] : import_cache_) { + if (item.has_value()) { + if (std::holds_alternative<UnsupportedItem>(*item) && + !IsFromCurrentTarget(decl)) { + continue; + } + items.push_back( std::make_tuple(decl->getSourceRange(), GetDeclOrder(decl), *item)); } - if (IsFromCurrentTarget(decl)) { - std::string name = "unnamed"; - if (const auto* named_decl = clang::dyn_cast<clang::NamedDecl>(decl)) { - name = named_decl->getQualifiedNameAsString(); - } - SourceLoc source_loc = ConvertSourceLocation(decl->getBeginLoc()); - if (!result.errors().empty()) { - auto error = absl::StrJoin(result.errors(), "\n\n"); - items.push_back(std::make_tuple( - decl->getSourceRange(), GetDeclOrder(decl), - UnsupportedItem{ - .name = name, .message = error, .source_loc = source_loc})); - } - } } for (auto comment : ImportFreeComments()) { @@ -439,21 +430,21 @@ void Importer::ImportDeclsFromDeclContext( const clang::DeclContext* decl_context) { for (auto decl : decl_context->decls()) { - LookupDecl(decl->getCanonicalDecl()); + ImportDeclIfNeeded(decl->getCanonicalDecl()); } } -Importer::LookupResult Importer::LookupDecl(clang::Decl* decl) { - if (!lookup_cache_.contains(decl)) { - lookup_cache_.insert({decl, ImportDecl(decl)}); - } +void Importer::ImportDeclIfNeeded(clang::Decl* decl) { + // TODO: Move `decl->getCanonicalDecl()` from callers into here. - return lookup_cache_[decl]; + if (!import_cache_.contains(decl)) { + import_cache_.insert({decl, ImportDecl(decl)}); + } } -Importer::LookupResult Importer::ImportDecl(clang::Decl* decl) { +std::optional<IR::Item> Importer::ImportDecl(clang::Decl* decl) { if (auto* namespace_decl = clang::dyn_cast<clang::NamespaceDecl>(decl)) { - return LookupResult("Namespaces are not supported yet"); + return ImportUnsupportedItem(decl, "Namespaces are not supported yet"); } else if (auto* function_decl = clang::dyn_cast<clang::FunctionDecl>(decl)) { return ImportFunction(function_decl); } else if (auto* function_template_decl = @@ -472,18 +463,19 @@ clang::dyn_cast<clang::TypedefNameDecl>(decl)) { return ImportTypedefName(typedef_name_decl); } else if (clang::isa<clang::ClassTemplateDecl>(decl)) { - return LookupResult("Class templates are not supported yet"); + return ImportUnsupportedItem(decl, "Class templates are not supported yet"); } else { - return LookupResult(); + return std::nullopt; } } -Importer::LookupResult Importer::ImportFunction( +std::optional<IR::Item> Importer::ImportFunction( clang::FunctionDecl* function_decl) { - if (!IsFromCurrentTarget(function_decl)) return LookupResult(); - if (function_decl->isDeleted()) return LookupResult(); + if (!IsFromCurrentTarget(function_decl)) return std::nullopt; + if (function_decl->isDeleted()) return std::nullopt; if (function_decl->isTemplated()) { - return LookupResult("Function templates are not supported yet"); + return ImportUnsupportedItem(function_decl, + "Function templates are not supported yet"); } devtools_rust::LifetimeSymbolTable lifetime_symbol_table; @@ -503,7 +495,7 @@ clang::dyn_cast<clang::CXXMethodDecl>(function_decl)) { if (!known_type_decls_.contains( method_decl->getParent()->getCanonicalDecl())) { - return LookupResult("Couldn't import the parent"); + return ImportUnsupportedItem(function_decl, "Couldn't import the parent"); } // non-static member functions receive an implicit `this` parameter. @@ -615,7 +607,7 @@ case clang::AS_none: // No need for IR to include Func representing private methods. // TODO(lukasza): Revisit this for protected methods. - return LookupResult(); + return std::nullopt; } llvm::Optional<MemberFuncMetadata::InstanceMethodMetadata> instance_metadata; @@ -650,7 +642,7 @@ } if (!errors.empty()) { - return LookupResult(errors); + return ImportUnsupportedItem(function_decl, errors); } bool has_c_calling_convention = @@ -664,7 +656,7 @@ CRUBIT_CHECK(return_type.ok()); if (translated_name.has_value()) { - return LookupResult(Func{ + return Func{ .name = *translated_name, .owning_target = GetOwningTarget(function_decl), .doc_comment = GetComment(function_decl), @@ -676,9 +668,9 @@ .member_func_metadata = std::move(member_func_metadata), .has_c_calling_convention = has_c_calling_convention, .source_loc = ConvertSourceLocation(function_decl->getBeginLoc()), - }); + }; } - return LookupResult(); + return std::nullopt; } BlazeLabel Importer::GetOwningTarget(const clang::Decl* decl) const { @@ -716,34 +708,36 @@ return invocation_.target_ == GetOwningTarget(decl); } -Importer::LookupResult Importer::ImportRecord( +std::optional<IR::Item> Importer::ImportRecord( clang::CXXRecordDecl* record_decl) { const clang::DeclContext* decl_context = record_decl->getDeclContext(); if (decl_context->isFunctionOrMethod()) { - return LookupResult(); + return std::nullopt; } if (record_decl->isInjectedClassName()) { - return LookupResult(); + return std::nullopt; } if (decl_context->isRecord()) { - return LookupResult("Nested classes are not supported yet"); + return ImportUnsupportedItem(record_decl, + "Nested classes are not supported yet"); } if (record_decl->isUnion()) { - return LookupResult("Unions are not supported yet"); + return ImportUnsupportedItem(record_decl, "Unions are not supported yet"); } // Make sure the record has a definition that we'll be able to call // ASTContext::getASTRecordLayout() on. record_decl = record_decl->getDefinition(); if (!record_decl || record_decl->isInvalidDecl() || !record_decl->isCompleteDefinition()) { - return LookupResult(); + return std::nullopt; } // To compute the memory layout of the record, it needs to be a concrete type, // not a template. if (record_decl->getDescribedClassTemplate() || clang::isa<clang::ClassTemplateSpecializationDecl>(record_decl)) { - return LookupResult("Class templates are not supported yet"); + return ImportUnsupportedItem(record_decl, + "Class templates are not supported yet"); } sema_.ForceDeclarationOfImplicitMembers(record_decl); @@ -767,7 +761,7 @@ std::optional<Identifier> record_name = GetTranslatedIdentifier(record_decl); if (!record_name.has_value()) { - return LookupResult(); + return std::nullopt; } // Provisionally assume that we know this RecordDecl so that we'll be able // to import fields whose type contains the record itself. @@ -777,7 +771,7 @@ // Importing a field failed, so note that we didn't import this RecordDecl // after all. known_type_decls_.erase(record_decl); - return LookupResult(fields.status().ToString()); + return ImportUnsupportedItem(record_decl, fields.status().ToString()); } for (const Field& field : *fields) { @@ -787,7 +781,7 @@ } } - return LookupResult(Record{ + return Record{ .rs_name = std::string(record_name->Ident()), .cc_name = std::string(record_name->Ident()), .id = GenerateDeclId(record_decl), @@ -803,17 +797,18 @@ .move_constructor = GetMoveCtorSpecialMemberFunc(*record_decl), .destructor = GetDestructorSpecialMemberFunc(*record_decl), .is_trivial_abi = record_decl->canPassInRegisters(), - .is_final = record_decl->isEffectivelyFinal()}); + .is_final = record_decl->isEffectivelyFinal()}; } -Importer::LookupResult Importer::ImportEnum(clang::EnumDecl* enum_decl) { +std::optional<IR::Item> Importer::ImportEnum(clang::EnumDecl* enum_decl) { std::optional<Identifier> enum_name = GetTranslatedIdentifier(enum_decl); if (!enum_name.has_value()) { // TODO(b/208945197): This corresponds to an unnamed enum declaration like // `enum { kFoo = 1 }`, which only exists to provide constants into the // surrounding scope and doesn't actually introduce an enum namespace. It // seems like it should probably be handled with other constants. - return LookupResult("Unnamed enums are not supported yet"); + return ImportUnsupportedItem(enum_decl, + "Unnamed enums are not supported yet"); } clang::QualType cc_type = enum_decl->getIntegerType(); @@ -823,13 +818,14 @@ // with no fixed underlying type." The same page implies that this can't // occur in C++ nor in standard C, but clang supports enums like this // in C "as an extension". - return LookupResult( + return ImportUnsupportedItem( + enum_decl, "Forward declared enums without type specifiers are not supported"); } std::optional<devtools_rust::TypeLifetimes> no_lifetimes; absl::StatusOr<MappedType> type = ConvertQualType(cc_type, no_lifetimes); if (!type.ok()) { - return LookupResult(type.status().ToString()); + return ImportUnsupportedItem(enum_decl, type.status().ToString()); } std::vector<Enumerator> enumerators; @@ -840,7 +836,8 @@ GetTranslatedIdentifier(enumerator); if (!enumerator_name.has_value()) { // It's not clear that this case is possible - return LookupResult("importing enum failed: missing enumerator name"); + return ImportUnsupportedItem( + enum_decl, "importing enum failed: missing enumerator name"); } enumerators.push_back(Enumerator{ @@ -849,31 +846,49 @@ }); } - return LookupResult(Enum{ + return Enum{ .identifier = *enum_name, .id = GenerateDeclId(enum_decl), .owning_target = GetOwningTarget(enum_decl), .underlying_type = *std::move(type), .enumerators = enumerators, - }); + }; } -Importer::LookupResult Importer::ImportTypedefName( +IR::Item Importer::ImportUnsupportedItem(const clang::Decl* decl, + std::string error) { + std::string name = "unnamed"; + if (const auto* named_decl = clang::dyn_cast<clang::NamedDecl>(decl)) { + name = named_decl->getQualifiedNameAsString(); + } + SourceLoc source_loc = ConvertSourceLocation(decl->getBeginLoc()); + return UnsupportedItem{ + .name = name, .message = error, .source_loc = source_loc}; +} + +IR::Item Importer::ImportUnsupportedItem(const clang::Decl* decl, + std::set<std::string> errors) { + return ImportUnsupportedItem(decl, absl::StrJoin(errors, "\n\n")); +} + +std::optional<IR::Item> Importer::ImportTypedefName( clang::TypedefNameDecl* typedef_name_decl) { const clang::DeclContext* decl_context = typedef_name_decl->getDeclContext(); if (decl_context) { if (decl_context->isFunctionOrMethod()) { - return LookupResult(); + return std::nullopt; } if (decl_context->isRecord()) { - return LookupResult("Typedefs nested in classes are not supported yet"); + return ImportUnsupportedItem( + typedef_name_decl, + "Typedefs nested in classes are not supported yet"); } } clang::QualType type = typedef_name_decl->getASTContext().getTypedefType(typedef_name_decl); if (MapKnownCcTypeToRsType(type.getAsString()).has_value()) { - return LookupResult(); + return std::nullopt; } std::optional<Identifier> identifier = @@ -884,14 +899,14 @@ ConvertQualType(typedef_name_decl->getUnderlyingType(), no_lifetimes); if (underlying_type.ok()) { known_type_decls_.insert(typedef_name_decl); - return LookupResult( - TypeAlias{.identifier = *identifier, - .id = GenerateDeclId(typedef_name_decl), - .owning_target = GetOwningTarget(typedef_name_decl), - .doc_comment = GetComment(typedef_name_decl), - .underlying_type = *underlying_type}); + return TypeAlias{.identifier = *identifier, + .id = GenerateDeclId(typedef_name_decl), + .owning_target = GetOwningTarget(typedef_name_decl), + .doc_comment = GetComment(typedef_name_decl), + .underlying_type = *underlying_type}; } else { - return LookupResult(std::string(underlying_type.status().message())); + return ImportUnsupportedItem( + typedef_name_decl, std::string(underlying_type.status().message())); } }
diff --git a/rs_bindings_from_cc/importer.h b/rs_bindings_from_cc/importer.h index 4642c17..54cd6b2 100644 --- a/rs_bindings_from_cc/importer.h +++ b/rs_bindings_from_cc/importer.h
@@ -93,40 +93,27 @@ void Import(clang::TranslationUnitDecl* decl); private: - // The result of looking up a decl. This may either contain an item that was - // imported or a vector of errors that occurred. Both are empty for decls that - // don't get imported on purpose. - class LookupResult { - std::optional<IR::Item> item_; - std::set<std::string> errors_; - - public: - LookupResult() {} - explicit LookupResult(IR::Item item) : item_(item) {} - explicit LookupResult(std::string error) : errors_({error}) {} - explicit LookupResult(std::set<std::string> errors) : errors_(errors) {} - - const std::optional<IR::Item>& item() const { return item_; } - const std::set<std::string>& errors() const { return errors_; } - }; - // Imports all decls contained in a `DeclContext`. void ImportDeclsFromDeclContext(const clang::DeclContext* decl_context); - // Looks up a decl, either from the cache, or by importing it and updating the - // cache. - LookupResult LookupDecl(clang::Decl* decl); + // Imports a decl, but only if it hasn't been already imported earlier. + void ImportDeclIfNeeded(clang::Decl* decl); // Imports a decl and creates an IR item (or error messages). // Does not use or update the cache. - LookupResult ImportDecl(clang::Decl* decl); + std::optional<IR::Item> ImportDecl(clang::Decl* decl); // These functions import specific `Decl` subtypes. They use `LookupDecl` to // lookup dependencies. They don't use or update the cache themselves. - LookupResult ImportFunction(clang::FunctionDecl* function_decl); - LookupResult ImportRecord(clang::CXXRecordDecl* record_decl); - LookupResult ImportTypedefName(clang::TypedefNameDecl* typedef_name_decl); - LookupResult ImportEnum(clang::EnumDecl* enum_decl); + std::optional<IR::Item> ImportFunction(clang::FunctionDecl* function_decl); + std::optional<IR::Item> ImportRecord(clang::CXXRecordDecl* record_decl); + std::optional<IR::Item> ImportTypedefName( + clang::TypedefNameDecl* typedef_name_decl); + std::optional<IR::Item> ImportEnum(clang::EnumDecl* enum_decl); + + IR::Item ImportUnsupportedItem(const clang::Decl* decl, std::string error); + IR::Item ImportUnsupportedItem(const clang::Decl* decl, + std::set<std::string> errors); absl::StatusOr<std::vector<Field>> ImportFields( clang::CXXRecordDecl* record_decl); @@ -191,7 +178,8 @@ clang::Sema& sema_; std::unique_ptr<clang::MangleContext> mangler_; - absl::flat_hash_map<const clang::Decl*, LookupResult> lookup_cache_; + absl::flat_hash_map<const clang::Decl*, std::optional<IR::Item>> + import_cache_; absl::flat_hash_set<const clang::TypeDecl*> known_type_decls_; }; // class Importer
diff --git a/rs_bindings_from_cc/test/golden/escaping_keywords_rs_api.rs b/rs_bindings_from_cc/test/golden/escaping_keywords_rs_api.rs index 9ca301d..d32c8cf 100644 --- a/rs_bindings_from_cc/test/golden/escaping_keywords_rs_api.rs +++ b/rs_bindings_from_cc/test/golden/escaping_keywords_rs_api.rs
@@ -57,7 +57,7 @@ // Error while generating bindings for item 'await': // Class templates are not supported yet -// rs_bindings_from_cc/test/golden/escaping_keywords.h;l=21 +// rs_bindings_from_cc/test/golden/escaping_keywords.h;l=22 // Error while generating bindings for item 'become': // Function templates are not supported yet