fix(ios): NSError** runtime fallback and enriched arg-count diagnostics#382
fix(ios): NSError** runtime fallback and enriched arg-count diagnostics#382NathanWalker wants to merge 2 commits into
Conversation
Adds defensive coverage and improved error visibility around NSError** out-parameter handling on modern iOS SDKs. Runtime: - Metadata.h: hasErrorOutParameter() now falls back to inspecting the last binary type encoding when the MethodHasErrorOutParameter flag is clear. Mirrors what ArgConverter::IsErrorOutParameter already does on the callback path. Lets apps shipping older/buggy metadata recover correct auto-NSError** handling without regenerating metadata. Diagnostics: - ArgConverter.mm: argument-count error message had its Actual/Expected labels swapped and no call-site context. Fixed the swap and added method kind, JS name, class name, full Objective-C selector, and a conditional hint when the method has an NSError** out-parameter that callers may omit. - ObjectManager.mm: ReleaseNativeCounterpartCallback error message now names the function it is about (__releaseNativeCounterpart). Documentation: - MetaFactory.cpp: explanatory comment block documenting why the existing NullableType unwrap (already on main via #367) is needed for "NSError * _Nullable * _Nullable" selectors on modern SDKs.
📝 WalkthroughWalkthroughThis PR enhances NSError out-parameter detection and improves argument-mismatch error messages across the NativeScript runtime. Metadata detection now infers NSError parameters from serialized type encodings when metadata flags are unset, and error messages in argument validation include method context, selector information, and conditional accounting for hidden NSError parameters. ChangesNSError Detection and Error Messaging
🎯 3 (Moderate) | ⏱️ ~20 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
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)metadata-generator/src/Meta/MetaFactory.cppIn file included from metadata-generator/src/Meta/MetaFactory.cpp:1: ... [truncated 2200 characters] ... 10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@NativeScript/runtime/ArgConverter.mm`:
- Around line 113-124: The error message always formats the Objective-C selector
as an instance selector "-[Class selector]" even when reporting a static method;
update the string construction around errorStream (where methodKindStr is
computed from instanceMethod) to choose "+[" when instanceMethod is false so the
selector is printed as "+[Class selector]" for class methods; modify the logic
that appends " (selector: -[" << classNameStr << " " << selectorStr << "])" to
switch the prefix based on instanceMethod (use methodKindStr or a conditional to
select "+" vs "-") so jsNameStr, classNameStr and selectorStr remain unchanged
but the selector punctuation correctly reflects instance vs static methods.
🪄 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: e5c8c3a2-db60-41c4-b69e-6563f1bbd9a5
📒 Files selected for processing (4)
NativeScript/runtime/ArgConverter.mmNativeScript/runtime/Metadata.hNativeScript/runtime/ObjectManager.mmmetadata-generator/src/Meta/MetaFactory.cpp
| const char* methodKindStr = instanceMethod ? "instance" : "static"; | ||
|
|
||
| std::ostringstream errorStream; | ||
| errorStream << "Actual arguments count: \"" << argsCount << ". Expected: \"" << args.Length() | ||
| << "\"."; | ||
| errorStream << "Actual arguments count: \"" << args.Length() << "\". Expected: \"" << argsCount | ||
| << "\""; | ||
| if (meta && meta->hasErrorOutParameter()) { | ||
| errorStream << " (or \"" << (argsCount - 1) | ||
| << "\" if omitting the trailing NSError** out-parameter)"; | ||
| } | ||
| errorStream << " for " << methodKindStr << " method \"" << jsNameStr << "\" on class \"" | ||
| << classNameStr << "\" (selector: -[" << classNameStr << " " << selectorStr | ||
| << "])."; |
There was a problem hiding this comment.
Use +[ when formatting static-method selectors.
Line 123 always renders the Objective-C selector as -[Class selector]. For class methods, the message now says static method but still prints an instance selector, which makes the new call-site context misleading.
Proposed fix
const char* classNameStr = klass ? class_getName(klass) : "<unknown>";
const char* methodKindStr = instanceMethod ? "instance" : "static";
+ const char selectorKind = instanceMethod ? '-' : '+';
std::ostringstream errorStream;
errorStream << "Actual arguments count: \"" << args.Length() << "\". Expected: \"" << argsCount
<< "\"";
if (meta && meta->hasErrorOutParameter()) {
@@
}
errorStream << " for " << methodKindStr << " method \"" << jsNameStr << "\" on class \""
- << classNameStr << "\" (selector: -[" << classNameStr << " " << selectorStr
+ << classNameStr << "\" (selector: " << selectorKind << "[" << classNameStr << " " << selectorStr
<< "]).";🤖 Prompt for 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.
In `@NativeScript/runtime/ArgConverter.mm` around lines 113 - 124, The error
message always formats the Objective-C selector as an instance selector "-[Class
selector]" even when reporting a static method; update the string construction
around errorStream (where methodKindStr is computed from instanceMethod) to
choose "+[" when instanceMethod is false so the selector is printed as "+[Class
selector]" for class methods; modify the logic that appends " (selector: -[" <<
classNameStr << " " << selectorStr << "])" to switch the prefix based on
instanceMethod (use methodKindStr or a conditional to select "+" vs "-") so
jsNameStr, classNameStr and selectorStr remain unchanged but the selector
punctuation correctly reflects instance vs static methods.
1 participant
Runtime:
Diagnostics:
Summary by CodeRabbit