Repository navigation
[jnigen] Don't break method names #1529
Description
Let's say that we have two methods void foo(float a) and void foo(double a). JNIgen generates these as void foo(double a) and void foo1(double a), the user can go ahead and use these methods as intended – using the first one for floats and the second one for doubles.
Now say an update of this code, swaps the order of the methods so now foo needs to be run on doubles and foo1 on floats. There will be no compilation error and the code fails in mysterious ways.
Once #746 lands, we can use the same generated yaml/json to see what the mappings signatures were before the regeneration and warn if the naming changes.
@liamappelbe Something to keep in mind for ffigen as well.
Activity
dcharkes commentedon Sep 10, 2024
We could possibly use the existing "imports" mechanism for this?
HosseinYousefi commentedon Sep 10, 2024
Yes. That's what I referred to in the description:
s/warn if the/use the same/!johnmccutchan commentedon Oct 11, 2024
Warning developers is not an acceptable solution. We cannot break users of generated code just because a method was added to an existing class. Full stop.
It sounds like when we generate the bindings, we need to generate a snapshot (json or whatever) of the mapping and when generating bindings jnigen will open that snapshot and use it ensure stable mappings.
HosseinYousefi commentedon Oct 11, 2024
Agreed.
[-]Warn if regeneration renames method overloads[/-][+][jnigen] Don't break method names[/+]dcharkes commentedon Dec 5, 2024
Some notes from a discussion:
previousRunOracle[uniqueResourceIdentifier]?['dart_name'] ?? uniqueRenamer(name). This should work with all kinds of decisions: renames, name conflicts, transforming getters into methods and vice versa.HosseinYousefi commentedon Dec 9, 2024
Interop between the generated bindings means that if
package:bimports the bindings from package:a, then we must use the correct renamings when classes inbinherit from classes ina. This is going to be in form of a json file generated alongside the dart files with all the symbols after renamings.On the other hand, due to the lack of method overloading and other syntactic differences between java and dart, we can have methods like
foo(int)andfoo(String)that will turn into foo andfoo$1in dart. If a new version of the Java library addsfoo(int, int)or if one of the other methods get removed, we want the bindings to remain stable, so any code that is using them won't change. So we need a json again to keep these mappings on regeneration runs.We could generate the same json for both of these use cases, there is also a question of how to run transformations? Do we run them first and then do another pass to de-duplicate names using
$Ksuffices? Or do we first de-dup and then run the transformers? In the latter case, we can have a json that is generated after de-duping for the stability purposes and another one after transformation for importing purposes and just put the responsibility on the user to avoid changes.HosseinYousefi commentedon Dec 9, 2024
Another idea is to have a boolean
is_renamedfor each transformed member in the json and ignore this element for the stability as it had been transformed anyways. The responsibility is now on the user to respect semver when renaming.Any iteration on the transformer is treated the same way: if we rename a member that was previously automatically named, we know what we're doing, so the json is changed to use the new name and
is_renamedis nowtrue.dcharkes commentedon Dec 9, 2024
I like this. For helping with semver we should encourage users to use https://pub.dev/packages/dart_apitool, instead of coming up with some JNIgen-specific solution.
Maybe
is_transformed(to cover getters<->functions etc.) oruser-defined,intended,manual.