Skip to content

[ffigen] Fix #2419 #2422

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 1 commit into from
Closed

[ffigen] Fix #2419 #2422

wants to merge 1 commit into from

Conversation

liamappelbe
Copy link
Contributor

When methods are renamed (to avoid duplicate names) in getDartMethodName they save the new name, and calling this method again just returns the cached name. This causes problems for the case where a method is copied from a super type to a sub type (eg in the case of constructors) if the subtype has a method with exactly the same name.

The fix is just to deep(ish) copy the method.

Fixes #2419

Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

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
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

Coverage Status

coverage: 87.146% (+4.6%) from 82.506%
when pulling 20bb76b on fix_2419
into ccbd88a on main.

@liamappelbe
Copy link
Contributor Author

Ah, now I remember why I didn't use this approach last time a similar bug came up. Methods instances need to be shared between layers in the inheritance hierarchy so that if one is renamed, they all are. Otherwise the method overriding relationships don't work. That mostly doesn't matter, because there's dynamic dispatch on the ObjC side anyway. But there's an edge case where mismatched renamings cause a collision where a subtype method has the same name as an unrelated supertype method, and arg/ret type mismatches between the methods can cause a Dart compile error.

But even if we share the method objects, there are even more subtle edge cases to worry about. Imaging Base has a method, ChildA inherits that method and has another method with the same name, and ChildB also inherits Base's method and has 2 other methods with the same name. Now ChildA and ChildB have to agree about how to rename all those methods. This could quickly get super complicated as the inheritance tree grows.

A better approach is probably to bypass this whole problem by shifting all instance methods to be extension methods. Extension methods only support static dispatch, but dynamic dispatch is handled on the ObjC side anyway, so it doesn't actually matter.

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
2 participants