[jnigen] filter out Kotlin DefaultConstructorMarker from Dart bindings#3048
Open
sagar-h007 wants to merge 5 commits intodart-lang:mainfrom
Open
[jnigen] filter out Kotlin DefaultConstructorMarker from Dart bindings#3048sagar-h007 wants to merge 5 commits intodart-lang:mainfrom
sagar-h007 wants to merge 5 commits intodart-lang:mainfrom
Conversation
sagar-h007
changed the title
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
liamappelbe
reviewed
Feb 4, 2026
Contributor
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
This looks reasonable, but it needs a test. You've added a test DefaultParams.kt file, but you're not doing anything with it. You'll need to regenerate the kotlin_test bindings using test/kotlin_test/generate.dart, and then verify the behavior of the bindings by adding some tests to test/kotlin_test/runtime_test_registrant.dart
sagar-h007
force-pushed
the
jnigen-remove-default-constructor-marker
branch
from
d59ab09 to
2d8609e
Compare
[jnigen] Add runtime tests for Kotlin DefaultConstructorMarker filtering Revert kotlin.dart to previous state
sagar-h007
force-pushed
the
jnigen-remove-default-constructor-marker
branch
from
885e19c to
64fb1ef
Compare
Contributor
|
Please don't force push unless you absolutely need to (I think I've had to force push once in the last 5 years). It deletes the commit history which makes your PR way harder to review. |
Contributor
|
You still haven't updated the test's generated code, which is probably why the analyzer bot is failing |
Author
|
Hi @liamappelbe — thanks for the clarification, and sorry about the force-push earlier. That’s on me, I’ll avoid doing that going forward. I’ve now regenerated the Let me know if there’s anything else you’d like me to adjust or verify. |
liamappelbe
reviewed
Feb 9, 2026
Contributor
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
Also, take a look at the analysis errors: https://github.com/dart-lang/native/actions/runs/21744051314/job/63028164140?pr=3048
Author
|
Hi @liamappelbe , Is there anything that needs to be changed? |
liamappelbe
reviewed
Feb 15, 2026
Contributor
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
Oops. Forgot to press submit on my review. Looks good, just one more small thing.
liamappelbe
reviewed
Feb 17, 2026
Contributor
liamappelbe
left a comment
liamappelbe
left a comment
There was a problem hiding this comment.
Looks good, just needs a changelog entry. Also take a look at the failing test.
btw I'm going on vacation for a week, so no more code reviews until next week.
… Dart generator ,ran generate.dart and add changelog
liamappelbe
reviewed
Feb 24, 2026
| late String finalName; | ||
|
|
||
| @JsonKey(includeFromJson: false) | ||
| bool isRenamed = false; |
Contributor
There was a problem hiding this comment.
This seems like a bit of a hack, to work around the late initialization error. Usually late initialization errors are due to a bug, such as making assumptions about the initialization order that turn out to be false. Could you explain what was causing the late initialization error, and why this is necessary to fix it?
Author
There was a problem hiding this comment.
This wasn’t a hack but a fix for an initialization ordering bug.
finalName was declared late and only assigned inside the renaming path. For elements that didn’t need renaming, that path never ran, so finalName stayed uninitialized and later access caused a LateInitializationError.
The original logic assumed the renamer always ran before finalName was used, which wasn’t true for all elements. That made the design rely on an implicit ordering invariant that wasn’t actually guaranteed.
The isRenamed flag fixes this by making the state explicit. Instead of guessing whether renaming happened based on a late field, we track it directly and only read finalName when it’s valid. This removes the ordering dependency and makes the generator’s behavior deterministic and easier to reason about.
Contributor
There was a problem hiding this comment.
The original logic assumed the renamer always ran before finalName was used, which wasn’t true for all elements. That made the design rely on an implicit ordering invariant that wasn’t actually guaranteed.
This doesn't really explain why the bug is happening, it's just restating the symptoms of the bug. Why are there elements that are being code-genned, but aren't being renamed? You should figure out why that's happening and fix that. This isRenamed flag simply hides that bug.
Author
There was a problem hiding this comment.
The LateInitializationError was triggered in _TypeGenerator.visitDeclaredType when it accessed classDecl.finalName for a class that had been removed by the Excluder before the Renamer ran. Since finalName is only set during renaming, it was never initialized for that class.
The underlying issue was stage ordering. The old pipeline ran Excluder before KotlinProcessor, so synthetic constructors involving DefaultConstructorMarker weren’t marked early enough. They survived exclusion and reached codegen with a ClassDecl that never went through the renaming phase.
The fix addresses both sides: reorder the stages so synthetic members are filtered correctly, and guard _TypeGenerator so any non-renamed class safely falls back to JObject instead of crashing.
Contributor
There was a problem hiding this comment.
So excluded elements are not being renamed, but are being code genned? That seems like the true bug here. Why are these excluded elements still being referenced during code generation? Shouldn't they be fully excluded?
Author
There was a problem hiding this comment.
@liamappelbe You’re right ,the root issue is that exclusion currently works at the declaration level, not the type-reference level.
The Excluder removes constructors that reference DefaultConstructorMarker, but it doesn’t remove or rewrite the corresponding type references in the resolved AST. So DeclaredType nodes for classes that were never in Classes.decls can still show up in signatures the generator visits. Those classes were never seen by the Renamer, so finalName was never initialized, which caused the crash.
So it’s not that excluded elements are being generated,it’s that references to non-generated classes can still exist in the type graph, and the generator wasn’t handling them as external types.
The isRenamed check is just a safety guard so codegen behaves deterministically. The real fix should happen earlier in the pipeline, either by normalizing such types to a fallback (like JObject) during resolution, or by having the exclusion pass clean up dangling type references. I’m planning a follow-up change to address that properly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does:
This updates jnigen to handle Kotlin default constructors properly.
Kotlin generates a synthetic DefaultConstructorMarker parameter for constructors with default values. That parameter shouldn’t show up in the generated Dart API. This change hides it from Dart, and always passes null to the JNI constructor instead, which is what Kotlin expects anyway.
What changed
This only applies to Kotlin-generated synthetic constructors and does not affect normal Java constructors.
Fixes: #1986
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.