Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/skills/android-reviewer/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ Review the CI results. **Never post ✅ LGTM if any required CI check is failing

### 5. Load review rules

Based on the file types identified in step 2, read the appropriate rule files from this skill's `references/` directory.
Based on the changed files and diff content identified in step 2, read the appropriate rule files from this skill's `references/` directory.

**Always load:**
- `references/repo-conventions.md` — Formatting, style, and patterns specific to this repository.
Expand All @@ -72,7 +72,7 @@ Based on the file types identified in step 2, read the appropriate rule files fr
- `references/csharp-rules.md` — When any `.cs` files changed. Covers nullable, async, error handling, performance, and code organization.
- `references/msbuild-rules.md` — When `.targets`, `.props`, `.projitems`, or `.csproj` files changed, or when MSBuild task C# files changed (e.g., files under `src/Xamarin.Android.Build.Tasks/` or `external/xamarin-android-tools/src/Microsoft.Android.Build.BaseTasks/`).
- `references/native-rules.md` — When `.c`, `.cpp`, `.h`, or `.hpp` files changed. Covers memory management, C++ best practices, symbol visibility, and platform-specific code.
- `references/interop-rules.md` — When both C# and native files changed, when the diff contains P/Invoke or JNI interop code (e.g., `DllImport`, `[Register]` attribute changes, `JNIEnv` calls, `[MarshalAs]`, `[StructLayout]`, `JniObjectReference`, `JniPeerMembers`, `JniTransition`), or when files under `external/Java.Interop/`, `src/Mono.Android/`, or `src/native/` changed.
- `references/interop-rules.md` — When interop markers or Java/JNI name conversions appear, or files under `external/Java.Interop/`, `src/Mono.Android/`, `src/native/`, or `src/Microsoft.Android.Sdk.TrimmableTypeMap/` change.
Comment thread
jonathanpeppers marked this conversation as resolved.
- `references/testing-rules.md` — When test files changed (e.g., files under `tests/`, `**/Tests/`, or test project directories).
- `references/security-rules.md` — When any code files changed (C#, C/C++, or MSBuild).

Expand Down
24 changes: 20 additions & 4 deletions .github/skills/android-reviewer/references/interop-rules.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
# Managed ↔ Native Interop Review Rules

Rules for the boundary between C# and C/C++ code — P/Invoke declarations, JNI
bindings, and shared structs. Load when both managed and native files change, or
when the diff contains interop markers (`JniObjectReference`, `JniPeerMembers`,
`DllImport`, `[Register]`, `JNIEnv`, `[MarshalAs]`, `[StructLayout]`).
Rules for managed/native boundaries and Java/JNI name handling. Load for interop
markers, Java/JNI name conversions, or changes under Java.Interop, Mono.Android,
native runtime, or TrimmableTypeMap paths.

---

Expand All @@ -21,6 +20,23 @@ when the diff contains interop markers (`JniObjectReference`, `JniPeerMembers`,

---

## Java Type Names

Determine the required representation at each changed call site:
Comment thread
jonathanpeppers marked this conversation as resolved.

| Representation | Nested-class example | Typical consumers |
|----------------|----------------------|-------------------|
| **JNI internal name** | `android/view/View$OnClickListener` | JNI registration, descriptors after removing `L`/`;`, runtime JNI lookup |
| **Java binary name** | `android.view.View$OnClickListener` | `Class.forName`, Android manifests, ACW maps, ProGuard/R8 class rules |
| **Java source name** | `android.view.View.OnClickListener` | Generated `.java` type references such as `extends`, `implements`, parameters, and return types |
| **Managed nested type name** | `Android.Views.View+IOnClickListener` | Managed metadata and assembly-qualified names |

- Read conversion helpers instead of trusting their names, and compare their full behavior with any inline logic they replace.
- Trace serialized names to the final consumer; correctness is per call site.
- Preserve `$` for binary-name consumers. Replace it with `.` only for Java source names.

---

## P/Invoke & Marshaling Checks

| Check | What to look for |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ Guidance for test code. The repo-specific conventions (e.g., `BaseTest`,
| **Generator tests must include Invoker types** | Tests for generated binding code (under `external/Java.Interop/tools/generator/` and related test projects) should verify both the interface/class output and the `*Invoker` type behavior. Invoker codegen has historically had subtle bugs with default interface methods and virtual dispatch. |
| **JVM-dependent tests** | Tests that require a running JVM must be in projects that configure the JVM environment (e.g., `Java.Interop-Tests`). Verify that test classes requiring a JVM are not placed in unit-test-only projects, where they will silently skip or fail with obscure errors. |
| **Expected codegen output tests** | Generator tests that compare against expected output files should be updated when the expected format changes. Stale expected-output files cause spurious test failures that mask real regressions. |
| **Test nested Java/JNI names at boundaries** | Name-conversion tests must include a `$` nested type and assert the final manifest, map, rule, or generated-source output—not only the helper. |
Loading