-
Notifications
You must be signed in to change notification settings - Fork 137
[jnigen] filter out Kotlin DefaultConstructorMarker from Dart bindings #3048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
sagar-h007
wants to merge
5
commits into
dart-lang:main
Choose a base branch
from
sagar-h007:jnigen-remove-default-constructor-marker
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
64fb1ef
jnigen: filter out Kotlin DefaultConstructorMarker from Dart bindings
sagar-h007 e14ceb2
[jnigen] Regenerate kotlin_test bindings for DefaultConstructorMarker…
sagar-h007 f2b4cd9
jnigen: exclude Kotlin synthetic constructors with DefaultConstructor…
sagar-h007 c1ea8d4
refactor(excluder): use param.isKotlinSynthetic for ctor filtering
sagar-h007 b8db12d
fix(jnigen): prevent LateInitializationError for unrenamed classes in…
sagar-h007 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
isRenamedflag simply hides that bug.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
LateInitializationErrorwas triggered in_TypeGenerator.visitDeclaredTypewhen it accessedclassDecl.finalNamefor a class that had been removed by theExcluderbefore theRenamerran. SincefinalNameis 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 involvingDefaultConstructorMarkerweren’t marked early enough. They survived exclusion and reached codegen with aClassDeclthat never went through the renaming phase.The fix addresses both sides: reorder the stages so synthetic members are filtered correctly, and guard
_TypeGeneratorso any non-renamed class safely falls back toJObjectinstead of crashing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liamappelbe You’re right ,the root issue is that exclusion currently works at the declaration level, not the type-reference level.
The
Excluderremoves constructors that referenceDefaultConstructorMarker, but it doesn’t remove or rewrite the corresponding type references in the resolved AST. So DeclaredType nodes for classes that were never inClasses.declscan still show up in signatures the generator visits. Those classes were never seen by theRenamer, sofinalNamewas 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
isRenamedcheck 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 (likeJObject) during resolution, or by having the exclusion pass clean up dangling type references. I’m planning a follow-up change to address that properly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so after the follow up change, are you planning to remove
isRenamed?In the meantime, the flag should probably be called
isExcludedor something. The real thing you want to represent is this edge case where a node is excluded but not removed from the AST.