Skip to content

[ffigen] Clone methods when copying them between classes #2436

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

Closed
wants to merge 2 commits into from

Conversation

liamappelbe
Copy link
Contributor

Relanding #2422. Method cloning is still necessary to fully fix #2419, even after #2434. Now that methods are generated on extensions, we need to clone methods when they're copied from a super type to a subtype, so that renames on one member of the type hierarchy doesn't affect the other members.

Fixes #2419

Copy link

github-actions bot commented Jul 23, 2025

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
objective_c Breaking 8.1.0 9.0.0-wip 9.0.0 ✔️
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.

Package Leaked API symbols
objective_c _FinalizablePointer
_Version

This check can be disabled by tagging the PR with skip-leaking-check.

License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/native_doc_dartifier/lib/native_doc_dartifier.dart
pkgs/native_doc_dartifier/lib/src/native_doc_dartifier_base.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@coveralls
Copy link

coveralls commented Jul 23, 2025

Coverage Status

coverage: 84.732% (+0.1%) from 84.62%
when pulling 0b68514 on fix_2419
into 3ff7b44 on main.

Copy link
Member

@HosseinYousefi HosseinYousefi left a comment

Choose a reason for hiding this comment

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

LGTM but I'm designing the JNIgen transformers in a way where if you rename a method in a class, you are also renaming it in all of the supertypes and subtypes. Because even with extensions, you see the methods of your supertype:

extension on Foo {
  void method() {
    print('hello');
  }
}
  
class Foo {}

class Bar extends Foo {}


void main() {
  Bar().method(); // prints hello
}

Of course shadowing works:

extension on Foo {
  void method() {
    print('hello');
  }
}
  
class Foo {}

extension on Bar {
  String method() {
    return 'bye';
  }
}

class Bar extends Foo {}


void main() {
  Bar().method(); // now returns 'bye'.
}

So after renaming the same method could exist with many different names for a type, while maybe some other methods are not. This might be somewhat confusing to the users.

In general, as a user I'd expect that if Foo().method$3() is exactly the same as Bar().method$3() if Bar extends Foo even if internally we're using extensions to reduce the number of polymorphic dispatches from two language to one.

cc @dcharkes, @goderbauer, what's your opinion?

@liamappelbe
Copy link
Contributor Author

liamappelbe commented Jul 23, 2025

LGTM but I'm designing the JNIgen transformers in a way where if you rename a method in a class, you are also renaming it in all of the supertypes and subtypes. Because even with extensions, you see the methods of your supertype:

My concern with that approach was that if you have a complicated inheritance tree with multiple branches, and many methods with the same name scattered throughout the tree, then all the children have to agree about how the methods will be renamed, to avoid collisions.

In the previous PR I got to that point and decided to give up and go with the extension method approach. Maybe if I thought about it more deeply there's an algorithm that avoids this problem (eg a specific iteration order of the tree that makes the renaming straightforward). If there is such an algorithm, I should do that instead of this PR.

EDIT: Actually I think I just have to make sure the iteration hits the super type before the sub types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ffigen generates duplicate methods for objective-c initializers
3 participants