Fix numeric suffix rename - #1530

PR Health

// Copyright (c) 2024, 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.

coverage: 91.069% (-0.001%) from 91.07%
when pulling f67b95c on fix_numeric_suffix_rename
into cb2c8fa on main.

Oops, I misread this suggestion in the bug. I thought you were going to add a $ as a delimiter between the original method name and the deduping number. Like foo, foo, foo1, foo1 => foo, foo$1, foo1, foo1$1. I think that might be less confusing to users, since only the deduped methods actually get changed. WDYT?

Oops, I misread this suggestion in the bug.

No you correctly read it. It also was one of the options. I then made another comment: #571 (comment)

I thought you were going to add a $ as a delimiter between the original method name and the deduping number. Like foo, foo, foo1, foo1 => foo, foo$1, foo1, foo1$1. I think that might be less confusing to users, since only the deduped methods actually get changed. WDYT?

My argument was the fact that methods ending with a number are fewer than the methods that have overloads. This way users don't have to write a ton of $s.

I thought you were going to add a $ as a delimiter between the original method name and the deduping number. Like foo, foo, foo1, foo1 => foo, foo$1, foo1, foo1$1. I think that might be less confusing to users, since only the deduped methods actually get changed. WDYT?

My argument was the fact that methods ending with a number are fewer than the methods that have overloads. This way users don't have to write a ton of $s.

I might align more with @liamappelbe here. Appending $1 for deduped things means that the method name before the $ is unchanged, which is less confusing. I don't expect users to type the $ that often, they'd likely use autocomplete in their IDE. (But I don't work with the generated APIs, so I'm not the one to ask! Happy with any solution that's usable for users.)

(Side note: If possible with transformers, we should implement some heuristic renamers which for example check if there's a single argument difference, and if yes use $argument as postfix instead of a number.)

I might align more with @liamappelbe here. Appending $1 for deduped things means that the method name before the $ is unchanged, which is less confusing. I don't expect users to type the $ that often, they'd likely use autocomplete in their IDE.

I agree, it may be subjective but aesthetically the code is going to look cryptic as overloadings are quite common. @lrhn @mkustermann wdyt?

@dcharkes @liamappelbe I changed the renaming strategy to use $K suffices for overloads. PTAL.

@dcharkes @liamappelbe I changed the renaming strategy to use $K suffices for overloads. PTAL.

Both isEnabled, isEnabled1, isEnabled2 and isEnabled, isEnabled$1, isEnabled$2 are exceedingly ugly! 😄 I see your argument for making overloading nicer and methods with numbers at the end more ugly. But really, we should have those transformers and some default ones that do a better job. isEnabledJsonParserFeature, isEnabledJsonFactoryFeature, and isEnabledStreamReadFeature would be better, still ugly, but better.

@dcharkes @liamappelbe I changed the renaming strategy to use $K suffices for overloads. PTAL.

Both isEnabled, isEnabled1, isEnabled2 and isEnabled, isEnabled$1, isEnabled$2 are exceedingly ugly! 😄 I see your argument for making overloading nicer and methods with numbers at the end more ugly. But really, we should have those transformers and some default ones that do a better job. isEnabledJsonParserFeature, isEnabledJsonFactoryFeature, and isEnabledStreamReadFeature would be better, still ugly, but better.

We already discussed the numeric vs non numeric in another issue: #585

Maybe the ugliness is a feature, not a bug! Having $ in the code signals the fact that there is something that could be improved if the user takes the time to provide exact renaming.

Also a lot of method overloading in Java happens because Java does not have default parameter values like Dart has. The user could transform such family of functions to use default parameters in Dart instead. Here's an example of such case: https://github.com/dart-lang/native/blob/main/pkgs/jni/lib/src/nio/jbyte_buffer.dart#L136-L155

I agree, it may be subjective but aesthetically the code is going to look cryptic as overloadings are quite common. @lrhn @mkustermann wdyt?

IMHO it makes sense to have the first part of the name be the same, then some separator making it clear to users what comes afterwards is to select the overloaded method. Separator could be a $, _, ...

The last part being a number is probably fine as users have code-completion and see signatures in their completion window. Otherwise one could combine the argument type names - which is more verbose (especially if one wants guarantee to avoid collisions). One could think of making this pluggable - so there's an option for users to manually name them.

@HosseinYousefi Is there a strategy to avoid breaking APIs when new overloads are added (and therefore cause change in numbering)? e.g. the generator could not just output bindings but also the naming choice it used. When running the generator again in future (with possibly updated jars), the generator could initialize it's chosen names from that naming choice and new overloads will get seperate names. That is also beneficial for code across packages: If one uses JNIgen to generate package:base and then package:special that imports package:base and extends classes there - it would want to use the same naming scheme as package:special.

Is there a strategy to avoid breaking APIs when new overloads are added (and therefore cause change in numbering)?

We so far have not tried to prevent renumbering (see the discussion in #585 (comment)). In previous discussions we preferred conciseness over the risk of renumbering (which can be avoided by appending a hash of the types/params or the types themselves).

@HosseinYousefi Is there a strategy to avoid breaking APIs when new overloads are added (and therefore cause change in numbering)?

👍 I opened an issue for this a few days ago: #1529

(The mechanism already exists for interop between generated code.)

We so far have not tried to prevent renumbering (see the discussion in #585 (comment)). In previous discussions we preferred conciseness over the risk of renumbering (which can be avoided by appending a hash of the types/params or the types themselves).

In a world where bindings are not single-file, single-package but across packages that import each other - or where bindings are part of the public API (e.g. us publishing packages with bindings like package:android_bluetooth) - then consistent naming becomes much more important.

Yes, using a hash is the default and simplest solution to this problem (also utilized by e.g. kotlin to java compiler) - but that would require adding the hash to all names, even if there's only one method - which makes it not very ergonomic.

An alternative would be to make the generator "remember" the naming choices it made before and bootstrap from that naming choice when generating dependent code or code for updated JARs - making the choice sticky and therefore much less breaking.

I also worry about renumbering. If there are overloadings, it's not unreasonable that there will be more.
Or if it's very common name that many things may want to have a function for, like WhatNot add(WhatNot, WhatNot), which can exist for numbers, lists, strings, points, vectors.

I do prefer $ as separator. It screams "not part of original name, inserted by automatic code generation" to me.
If the original native name contains a $, then ... maybe double it. Or just add a $0 at the end, even if not overloaded, so that the last $ is always introduced by this tool.

We could give users a way to control the naming. It could be the same functionality we use for avoiding renumbering.

Say ffigen/jnigen reads a file before doing the renaming, a file which contains the result of prior renamings, so that it always gives the same name to a declaration that it has already seen once, and it writes the file back when it has seen any changes to declarations.
(That file may need to contain all declarations, even those that are not overloaded, so it knows which one was the original when the first overloading declaration is added. It can drop members that don't exist any more.)

Then users can check the file into source control to preserve the naming, and they can hand edit the file to give a specific different naming to a function.
If I want Point add(Point p1, Point p2) to be named addPoints instead of add$7, I can change the entry to, fx, "add(Point,Point):Point": "addPoints" (or however the receiver signature is represented in a unique way).

Using a hash of the signature makes for long names (or higher risk of conflict if we truncate to fewer bits).
Could use a hash of the signature in the renaming file, so it maps the name foo and signature with hash 0xA512B9F0 to foo$1.

(Do we have a way to choose a name today when using xxigen, or is that something you're expected to do at the Dart end, with a Point addPoints(Point p1, Point p2) => prefix.add$7(p1, p2); forwarder?)

Do we have a way to choose a name today when using xxigen

ffigen has it. jnigen will have it soon.