From 5e3e6d11dfa68b4acdd90b57b05577427fa4e741 Mon Sep 17 00:00:00 2001 From: Jochen Hoenle <173445474+hoe-jo@users.noreply.github.com> Date: Tue, 21 Jul 2026 10:18:52 +0200 Subject: [PATCH] [clang parser] update logging strategy currently clang parser uses no logging / eprintln, use logging instead --- cpp/libclang/src/visitor/BUILD | 2 + .../src/visitor/src/class_parser_helper.rs | 12 ++- cpp/libclang/src/visitor/src/class_visitor.rs | 94 ++++++++++++++----- .../src/visitor/src/class_visitor_test.rs | 42 +++++++++ cpp/libclang/src/visitor/src/enum_visitor.rs | 9 +- .../src/visitor/src/function_visitor.rs | 21 ++++- 6 files changed, 153 insertions(+), 27 deletions(-) diff --git a/cpp/libclang/src/visitor/BUILD b/cpp/libclang/src/visitor/BUILD index 8ca39d08..06724c58 100644 --- a/cpp/libclang/src/visitor/BUILD +++ b/cpp/libclang/src/visitor/BUILD @@ -28,6 +28,7 @@ rust_library( "//tools/metamodel/class:class_diagram", "//tools/metamodel/sequence:sequence_diagram", "@crates//:clang", + "@crates//:log", "@crates//:serde", ], ) @@ -40,6 +41,7 @@ rust_test( ], deps = [ "@crates//:clang", + "@crates//:log", "@crates//:serde", ], ) diff --git a/cpp/libclang/src/visitor/src/class_parser_helper.rs b/cpp/libclang/src/visitor/src/class_parser_helper.rs index 37c18ac9..a714b907 100644 --- a/cpp/libclang/src/visitor/src/class_parser_helper.rs +++ b/cpp/libclang/src/visitor/src/class_parser_helper.rs @@ -547,10 +547,18 @@ fn resolve_named_type(original: &Type, canonical: &Type) -> ResolvedType { } if is_dependent_expression_type(original) { - return ResolvedType::Dependent(resolve_unknown_name(original, canonical)); + let name = resolve_unknown_name(original, canonical); + log::debug!( + "type '{}' is structurally unresolvable before template instantiation \ + (dependent/decltype expression)", + name + ); + return ResolvedType::Dependent(name); } - ResolvedType::Unknown(resolve_unknown_name(original, canonical)) + let name = resolve_unknown_name(original, canonical); + log::debug!("could not resolve type '{}' to a concrete entity id", name); + ResolvedType::Unknown(name) } /// Detects types libclang exposes as `Unexposed` because their meaning depends on diff --git a/cpp/libclang/src/visitor/src/class_visitor.rs b/cpp/libclang/src/visitor/src/class_visitor.rs index cb4d010b..cfca7f66 100644 --- a/cpp/libclang/src/visitor/src/class_visitor.rs +++ b/cpp/libclang/src/visitor/src/class_visitor.rs @@ -59,7 +59,10 @@ impl ClassVisitor { entity: &Entity, namespace: Option<&str>, ) -> Option<(ParsedClassInfo, SimpleEntity)> { - let name = entity.get_name()?; + let Some(name) = entity.get_name() else { + log::debug!("skipping class/struct: anonymous type has no name"); + return None; + }; let id = match namespace { Some(ns) if !ns.is_empty() => format!("{ns}::{name}"), @@ -154,8 +157,17 @@ pub(crate) fn parse_source_location(entity: &Entity) -> SourceLocation { } fn collect_variable_type(entity: &Entity) -> Option { - let name = entity.get_name()?; - let field_type = entity.get_type()?; + let Some(name) = entity.get_name() else { + log::debug!("skipping field/variable: entity has no name"); + return None; + }; + let Some(field_type) = entity.get_type() else { + log::debug!( + "skipping field/variable '{}': could not determine its type", + name + ); + return None; + }; Some(ParsedVariableType { name, @@ -204,11 +216,21 @@ fn method_arguments<'tu>(entity: &Entity<'tu>) -> Vec> { } fn parse_type_alias(entity: &Entity) -> Option { - let alias = entity.get_name()?; + let Some(alias) = entity.get_name() else { + log::debug!("skipping type alias: entity has no name"); + return None; + }; - let original_type = entity + let Some(original_type) = entity .get_typedef_underlying_type() - .map(|t| render_type_for_display(&t, &resolve_type(&t)))?; + .map(|t| render_type_for_display(&t, &resolve_type(&t))) + else { + log::debug!( + "skipping type alias '{}': could not determine underlying type", + alias + ); + return None; + }; Some(TypeAlias { alias, @@ -407,23 +429,41 @@ fn infer_entity_type_from_members(kind: EntityKind, class: &SimpleEntity) -> Ent fn build_relationships_for_class(ctx: &mut VisitContext, builder: &ParsedClassInfo) { for base in &builder.base_classes { let Some(resolved_base) = base.resolved_type.referenced_entity_id() else { - eprintln!( - "Warning: unable to resolve base type '{}' for '{}'; \ - skipping inheritance relationship (dependent/decltype expression?)", - base.resolved_type.render_for_display(), - builder.id - ); + if matches!(base.resolved_type, ResolvedType::Dependent(_)) { + // Expected, permanent limitation of AST-only analysis (e.g. a + // `decltype`/SFINAE base class that cannot be resolved without + // template instantiation) — never abort, not even in debug/test + // builds. + log::debug!( + "unable to resolve base type '{}' for '{}'; \ + skipping inheritance relationship (dependent/decltype expression)", + base.resolved_type.render_for_display(), + builder.id + ); + } else { + // Unexpected: a base class resolving to `Unknown`/`Builtin` likely + // indicates a gap in the resolver rather than a known limitation. + // When in doubt, warn and continue rather than abort — a single + // unanticipated input must never crash the parser. + log::warn!( + "unable to resolve base type '{}' for '{}'; \ + skipping inheritance relationship (unexpected unresolved type)", + base.resolved_type.render_for_display(), + builder.id + ); + } continue; }; let Some(target_class) = ctx.types.get(resolved_base) else { // Base type is not in the type map — it is likely an external dependency - // that was filtered out during the visit phase. Skip the relationship - // rather than panicking so analysis of workspace classes can proceed. - eprintln!( - "Warning: base type '{}' not found in type map for '{}'; \ - skipping inheritance relationship (external dependency?)", - resolved_base, builder.id + // that was filtered out during the visit phase. This is expected and + // common, so skip the relationship without ever aborting. + log::debug!( + "base type '{}' not found in type map for '{}'; \ + skipping inheritance relationship (external dependency)", + resolved_base, + builder.id ); continue; }; @@ -434,10 +474,20 @@ fn build_relationships_for_class(ctx: &mut VisitContext, builder: &ParsedClassIn RelationType::Inheritance }; - let class = ctx - .types - .get_mut(&builder.id) - .expect("Source class must exist before building relationships"); + let Some(class) = ctx.types.get_mut(&builder.id) else { + // Internal invariant: `builder.id` is derived from `ctx.types` during + // the visit phase, so it should always still be present here. If it + // isn't, that's a bug in the visitor pipeline rather than an expected + // input condition. When in doubt, warn and skip rather than abort — + // a single unanticipated input must never crash the parser. + log::warn!( + "source class '{}' unexpectedly missing from type map; \ + skipping inheritance relationship to '{}'", + builder.id, + resolved_base + ); + continue; + }; add_relationship( class, diff --git a/cpp/libclang/src/visitor/src/class_visitor_test.rs b/cpp/libclang/src/visitor/src/class_visitor_test.rs index b26f6b48..0eee96d4 100644 --- a/cpp/libclang/src/visitor/src/class_visitor_test.rs +++ b/cpp/libclang/src/visitor/src/class_visitor_test.rs @@ -181,3 +181,45 @@ fn resolve_relationships_skips_dependent_base_class_without_panicking() { SourceLocation::new(source_file, 5) ); } + +/// An unresolved base type that is *not* `ResolvedType::Dependent` (e.g. +/// `Unknown`) is unexpected and gets a `log::warn!`, but must still never +/// abort the parser — when in doubt, warn and skip rather than crash. +#[test] +fn resolve_relationships_warns_and_skips_unexpected_unresolved_base() { + let source_file = "unit_source.cpp"; + + let mut ctx = VisitContext::default(); + ctx.types.insert( + "Derived".to_string(), + SimpleEntity { + id: "Derived".to_string(), + name: "Derived".to_string(), + source_location: SourceLocation::new(source_file, 1), + ..Default::default() + }, + ); + + ctx.parsed_class_info.push(ParsedClassInfo { + id: "Derived".to_string(), + base_classes: vec![ParsedBaseClass { + // Not `Dependent`: an unexpected, unresolvable base type. + resolved_type: ResolvedType::Unknown("SomeWeirdType".to_string()), + source_location: SourceLocation::new(source_file, 1), + }], + variable_types: vec![], + method_types: vec![], + }); + + // Must not panic. + ClassVisitor::resolve_relationships(&mut ctx); + + let derived = ctx + .types + .get("Derived") + .expect("Derived must still exist after relationship resolution"); + assert!( + derived.relationships.is_empty(), + "unresolvable base class must not produce a relationship" + ); +} diff --git a/cpp/libclang/src/visitor/src/enum_visitor.rs b/cpp/libclang/src/visitor/src/enum_visitor.rs index 4cc7e209..10e7d702 100644 --- a/cpp/libclang/src/visitor/src/enum_visitor.rs +++ b/cpp/libclang/src/visitor/src/enum_visitor.rs @@ -28,7 +28,10 @@ impl AstVisitor for EnumVisitor { impl EnumVisitor { fn visit_enum(entity: Entity) -> Option { - let name = entity.get_name()?; + let Some(name) = entity.get_name() else { + log::debug!("skipping enum: anonymous enum has no name"); + return None; + }; let namespace_id = Self::get_namespace_id(&entity); let full_qualified_id = if let Some(namespace_id) = &namespace_id { format!("{}::{}", namespace_id, name) @@ -79,6 +82,10 @@ fn get_literals(entity: Entity) -> Vec { Some(constant_val_tuple.0 as i128) } } else { + log::debug!( + "enum constant '{}' has no value; skipping literal", + c.get_name().unwrap_or_default() + ); return None; }; diff --git a/cpp/libclang/src/visitor/src/function_visitor.rs b/cpp/libclang/src/visitor/src/function_visitor.rs index 61f1abb6..a9fc2e92 100644 --- a/cpp/libclang/src/visitor/src/function_visitor.rs +++ b/cpp/libclang/src/visitor/src/function_visitor.rs @@ -34,23 +34,40 @@ impl FunctionVisitor { // ── Top-level extraction ────────────────────────────────────────────────── fn extract_function_def(entity: Entity) -> Option { - let body_node = Self::get_method_body(entity)?; + let Some(body_node) = Self::get_method_body(entity) else { + log::debug!( + "skipping method '{}': no compound statement body (declaration-only?)", + entity.get_name().unwrap_or_default() + ); + return None; + }; if !entity .get_location() .map(|loc| loc.is_in_main_file()) .unwrap_or(false) { + log::debug!( + "skipping method '{}': not located in the main file", + entity.get_name().unwrap_or_default() + ); return None; } - let method_name = entity.get_name()?; + let Some(method_name) = entity.get_name() else { + log::debug!("skipping method: entity has no name"); + return None; + }; let class_name = entity .get_semantic_parent() .and_then(|p| p.get_name()) .unwrap_or_default(); if class_name.is_empty() { + log::debug!( + "skipping method '{}': owning class/struct has no name", + method_name + ); return None; }