Skip to content

fix(metadata-generator): strip nullability wrappers before structural type checks#389

Merged
NathanWalker merged 2 commits into
mainfrom
fix/nullability-error-out
Jun 12, 2026
Merged

fix(metadata-generator): strip nullability wrappers before structural type checks#389
NathanWalker merged 2 commits into
mainfrom
fix/nullability-error-out

Conversation

@edusperoni

@edusperoni edusperoni commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

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). 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.
  • 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.

Summary by CodeRabbit

  • New Features

    • Support for nullability-carrying typedefs in NSError-style out-parameters, improving nullable error handling.
    • Improved nullability handling across metadata and type generation, yielding more accurate type and TypeScript signature output.
  • Tests

    • Added test coverage validating the new nullable-typedef error out-parameter behavior.

… 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.
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7cd8e1b2-3e9d-454b-b819-067fc590dc3b

📥 Commits

Reviewing files that changed from the base of the PR and between d1e1fed and 885797c.

📒 Files selected for processing (1)
  • metadata-generator/src/Yaml/MetaYamlTraits.h

📝 Walkthrough

Walkthrough

This 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.

Changes

Nullability typedef support in metadata generator

Layer / File(s) Summary
stripNullability() API
metadata-generator/src/Meta/TypeEntities.h
Add stripNullability() and const overload to Meta::Type that iteratively unwraps nested TypeNullable and TypeNonNullable wrapper nodes.
Type creation respects outer attributed nullability
metadata-generator/src/Meta/TypeFactory.cpp
Unwrap inner type nullability via stripNullability() before applying clang::AttributedType nullability so the outer annotation wins.
Type equality ignores nullability wrappers
metadata-generator/src/Meta/Utils.cpp
Utils::areTypesEqual now strips nullability from both inputs before structural comparison; other helpers unchanged.
Metadata pattern detection updates
metadata-generator/src/Meta/MetaFactory.cpp
Use stripNullability() when detecting bridged interfaces in function signatures and when recognizing NSError out-parameter patterns.
TypeScript definition generation
metadata-generator/src/TypeScript/DefinitionWriter.cpp
Use stripNullability() for instancetype detection, closed generics discovery, pointer-void checks, nullable-pointer recursion, and return-type computation.
YAML serialization for nullable type variants
metadata-generator/src/Yaml/MetaYamlTraits.h
Extend Meta::TypeType enum with TypeProtocol, TypeExtVector, TypeNullable, TypeNonNullable and map InnerType for nullable/non-nullable types; reformat trait implementations.
Test fixture with nullable typedef
TestFixtures/Api/TNSApi.h, TestFixtures/Api/TNSApi.m
Add typedef NSError* _Nullable TNSNullableError; and - (BOOL)methodTypedefNullable:error: using the typedef for the out-parameter and following standard error-assignment logic.
Test case for typedef-based nullability
TestRunner/app/tests/Marshalling/ObjCTypesTests.js
Add NSErrorOutParameterWithNullabilityCarryingTypedef test covering zero/non-zero error codes and verifying interop.Reference becomes an NSError.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🐰 A typedef hops in with nullability wrapped tight,
We strip it away to keep types right,
From patterns to YAML, the changes align,
With tests that confirm all is fine!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.63% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding stripNullability() functionality and using it throughout the metadata-generator to properly handle nullability wrappers in type structural checks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ad920c6 and d1e1fed.

📒 Files selected for processing (9)
  • TestFixtures/Api/TNSApi.h
  • TestFixtures/Api/TNSApi.m
  • TestRunner/app/tests/Marshalling/ObjCTypesTests.js
  • metadata-generator/src/Meta/MetaFactory.cpp
  • metadata-generator/src/Meta/TypeEntities.h
  • metadata-generator/src/Meta/TypeFactory.cpp
  • metadata-generator/src/Meta/Utils.cpp
  • metadata-generator/src/TypeScript/DefinitionWriter.cpp
  • metadata-generator/src/Yaml/MetaYamlTraits.h

Comment thread metadata-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.
@NathanWalker NathanWalker merged commit 05224d3 into main Jun 12, 2026
10 of 11 checks passed
@NathanWalker NathanWalker deleted the fix/nullability-error-out branch June 12, 2026 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants