fix(metadata-generator): strip nullability wrappers before structural type checks#389
Conversation
… type checks Since #357/#363, _Nullable/_Nonnull annotations are represented as NullableType/NonNullableType wrapper nodes in the type tree, breaking every consumer that pattern-matches on type kind without unwrapping: - MethodHasErrorOutParameter: `NSError * _Nullable * _Nullable` no longer matched Pointer(Interface NSError). #367 added a single-level unwrap, but wrappers can nest (e.g. BNNSFilterCreateLayerPadding, whose typedef carries its own nullability), so it could still miss. - CF Create Rule: functions returning `CFXxxRef _Nullable` skipped the FunctionOwnsReturnedCocoaObject/FunctionReturnsUnmanaged flags, breaking CF memory management for ~200 SDK functions. - Utils::areTypesEqual: wrapper kinds fell into `default: return true`, so any two Nullable types compared equal (and Nullable(A) != A), affecting RemoveDuplicateMembersFilter. - .d.ts output: hasClosedGenerics/getClosedGenericsIfAny missed wrapped interface returns, dropping generic type parameters (e.g. dictionaryWithContentsOfFile lost <KeyType, ObjectType>). - YAML output: Nullable/NonNullable had no serialization cases and were silently dropped. Fix: add Type::stripNullability(), which unwraps arbitrarily nested nullability wrappers, and use it at every structural inspection site. createFromAttributedType now also collapses nesting at creation (the outermost annotation wins), so wrappers can no longer stack. The binary serializer remains intentionally transparent to these wrappers, so the runtime metadata shape is unchanged. Adds a TNSApi fixture with a nullability-carrying typedef error parameter (`typedef NSError* _Nullable TNSNullableError`) plus a matching marshalling test to cover the nested-annotation case. Supersedes the runtime fallback proposed in #382 by fixing the flag at the source; #382's diagnostics improvements remain worth cherry-picking separately. Fixes regression introduced in #357/#363, completes #367.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds Meta::Type::stripNullability() and applies it across the metadata generator, type equality, TypeScript emission, and YAML traits; it also adds a typedef-based nullable NSError fixture method and a test exercising its out-parameter behavior. ChangesNullability typedef support in metadata generator
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metadata-generator/src/Yaml/MetaYamlTraits.h`:
- Around line 217-344: The switch in MappingTraits<Meta::Type*> misses a handler
for Meta::TypeType::TypeExtVector so emitted ExtVector info is dropped; add a
new case for TypeExtVector that casts the type to Meta::ExtVectorType (e.g.,
Meta::ExtVectorType& concreteType = type->as<Meta::ExtVectorType>();) and call
io.mapRequired on its fields (at minimum map the inner type and the vector size,
e.g., io.mapRequired("InnerType", concreteType.innerType) and
io.mapRequired("Size", concreteType.size)) following the style used in
TypeConstantArray/TypeIncompleteArray so ExtVector serialized data is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b9851c5-354f-4217-8884-ac9fd2147d1c
📒 Files selected for processing (9)
TestFixtures/Api/TNSApi.hTestFixtures/Api/TNSApi.mTestRunner/app/tests/Marshalling/ObjCTypesTests.jsmetadata-generator/src/Meta/MetaFactory.cppmetadata-generator/src/Meta/TypeEntities.hmetadata-generator/src/Meta/TypeFactory.cppmetadata-generator/src/Meta/Utils.cppmetadata-generator/src/TypeScript/DefinitionWriter.cppmetadata-generator/src/Yaml/MetaYamlTraits.h
The previous commit added ExtVector to the TypeType enum mapping but not to the structural switch in MappingTraits<Meta::Type*>, so ext-vector types emitted only 'Type: ExtVector' with their element type and size dropped. Mirror the TypeConstantArray style (ArrayType + Size). Addresses PR review feedback on #389.
Since #357/#363, _Nullable/_Nonnull annotations are represented as NullableType/NonNullableType wrapper nodes in the type tree, breaking every consumer that pattern-matches on type kind without unwrapping:
NSError * _Nullable * _Nullableno longer matched Pointer(Interface NSError). fix: enhance nullable type handling in method parameter validation #367 added a single-level unwrap, but wrappers can nest (e.g. BNNSFilterCreateLayerPadding, whose typedef carries its own nullability), so it could still miss.CFXxxRef _Nullableskipped the FunctionOwnsReturnedCocoaObject/FunctionReturnsUnmanaged flags, breaking CF memory management for ~200 SDK functions.default: return true, so any two Nullable types compared equal (and Nullable(A) != A), affecting RemoveDuplicateMembersFilter.Fix: add Type::stripNullability(), which unwraps arbitrarily nested nullability wrappers, and use it at every structural inspection site. createFromAttributedType now also collapses nesting at creation (the outermost annotation wins), so wrappers can no longer stack. The binary serializer remains intentionally transparent to these wrappers, so the runtime metadata shape is unchanged.
Adds a TNSApi fixture with a nullability-carrying typedef error parameter (
typedef NSError* _Nullable TNSNullableError) plus a matching marshalling test to cover the nested-annotation case.Supersedes the runtime fallback proposed in #382 by fixing the flag at the source; #382's diagnostics improvements remain worth cherry-picking separately.
Fixes regression introduced in #357/#363, completes #367.
Summary by CodeRabbit
New Features
Tests