Skip to content
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

New Adventures in Obj-C Lightweight Generics... #2096

Open
mungler opened this issue May 19, 2023 · 15 comments
Open

New Adventures in Obj-C Lightweight Generics... #2096

mungler opened this issue May 19, 2023 · 15 comments
Assignees

Comments

@mungler
Copy link

mungler commented May 19, 2023

Following on from #1947

I can now see that the converted source headers have generic information where applicable, e.g. a

private Monkey[] monkeys

field in the java source becomes:

IOSObjectArray<ABCMonkey *> monkeys

which is great!

The problem is that non-Collection classes are also being translated with generics... or perhaps the problem is that their generics are not being stripped?

I'm seeing two types of error when compiling our Obj-C library project with the --objc-generics flag enabled:

Type arguments cannot be applied to non-parameterized class 'Foo'

For example, we have a 'Resource' class, defined in Java as:

public class Resource<T>

with a constructor which takes a Class<T> argument:

public Resource(String key, Request request, Class<T> target) {

In the generated source, i'm seeing errors around this, e.g.:

Type arguments cannot be applied to non-parameterized class 'IOSClass' from this line:

- (instancetype __nonnull)initWithASWRequest:(ASWRequest *)request withIOSClass:(IOSClass<T> *)target;

Another issue shows up in a method on a class that uses the Resource class, defined in the java source as:

public void putResource(final String key, final Resource<?> resource) {
   resourcesMap.put(key, resource);
}

the translation looks like:

- (void)putResourceWithNSString:(NSString *)key
      withASWPresentersResource:(ASWPresentersResource<id> *)resource;

which gives:

Type arguments cannot be applied to non-parameterized class 'ASWPresentersResource'

I'm not totally sure what is wrong here, I think some things are being mistranslated, and the <T> references are being copied as-is, maybe?

As I understand it, in Obj-C, you're free to parameterise any class, whereas Swift will ignore any lightweight generics other than those on collection classes. Which is fine, but the project in question is Obj-C, not Swift (our app is Swift, but uses the translated code in the form of a static library).

I hope that makes some sort of sense. Please let me know if i can be of any assistance. I could possibly try to put together an example project that shows the problem.

@mungler
Copy link
Author

mungler commented May 19, 2023

@tomball sorry it always takes me months to follow up... i'm scraping time when I can :)

@mungler
Copy link
Author

mungler commented May 19, 2023

I believe this example class shows the problem, when converted with latest j2objc and --objc-generics flag:

public class Scratch<T> {

    private String key;
    private Class<T> target;

    public Scratch(String key, Class<T> target) {
        this.key = key;
        this.target = target;
    }

    public String inspect() {
        return "Key: " + key + " target: " + target.getName();
    }

    public static void main(String[] args) {
        System.out.println(new Scratch("foo", Long.class).inspect());
        System.out.println(new Scratch("bar", Float.class).inspect());
    }
}

@mungler
Copy link
Author

mungler commented May 19, 2023

Attached a zip containing Scratch.java and the j2objc-generated Scratch.h and Scratch.m files.

For reference, in the header file, the constructor is translated as:

- (instancetype __nonnull)initWithNSString:(NSString *)key
                              withIOSClass:(IOSClass<T> *)target;

which results in the error:

Type arguments cannot be applied to non-parameterized class 'IOSClass'

And in the .m file:

J2OBJC_FIELD_SETTER(Scratch, target_, IOSClass<T> *)

which results in:

No type or protocol named 'T'

Example.zip

@mungler
Copy link
Author

mungler commented May 19, 2023

Another example, in case its useful.

Example2.zip

@tomball
Copy link
Collaborator

tomball commented May 19, 2023

Thanks for the feedback here, as it sounds like we have a way to go before this feature is usable. @litstrong is now the responsible engineer for this feature, so I'm sure you'll be hearing from him soon.

@mungler
Copy link
Author

mungler commented May 19, 2023

Thanks @tomball - again apologies for the glacial pace of progress on my side!

@litstrong FYI i think there are two distinct problems here, one of which is shown in each of the two example zip files. Firstly it seems that IOSClass should perhaps become generic itself, or else references to Class in java should be translated without the generic argument. The other problem is handling of user-written generic classes - <T> seems to be being mapped directly, and <?> is being written as <id>, with undesirable results.

Please let me know if i can be of assistance, including testing updates :)

@litstrong
Copy link
Collaborator

Thanks @mungler for your valuable feedback and sorry for my late response.

The j2objc generics support (--objc-generics) is not fully implemented yet. So, looks like we have two issues in this thread:

  1. We want to update IOSClass to retain type info to support Class<T> translation.
  2. Foo<?> should be translated to ObjCFoo * instead of ObjCFoo<id> *.

Please let me know if my understanding is correct.

@mungler
Copy link
Author

mungler commented May 31, 2023

Hi @litstrong i believe so, yes. Happy to test it out on a different branch or whatever, just let me know when you have something testable 👌

@mungler
Copy link
Author

mungler commented Jul 10, 2023

Hi @litstrong / @tomball just checking in to see if I can be of any assistance with testing things for this? Much appreciated.

@litstrong
Copy link
Collaborator

Hi Rory, apologize for slow response on this thread. I think we have good test examples from you to start, but was not able to prioritize it yet because the team is under-staffed at the moment and I'm busy with many things in parallel. But it is definitely on my plate, I'll try to spend time on it once I address some tasks with higher priority in my list.

@mungler
Copy link
Author

mungler commented Jul 10, 2023 via email

@mungler
Copy link
Author

mungler commented Sep 20, 2023

Hi @litstrong I was wondering if i might be able to tackle this, emboldened by having my first PR accepted and merged recently... :)

How would you feel about me pinging you random questions and things when I get stuck? Do you have any pointers to where to look first? I can see that IOSClass.h in the jre_emul project will need updated to specify <__covariant ObjectType> similar to IOSArray, but I guess the main bulk of the effort will be in the translator project itself. I can see the hooks where the option kicks in, but am unfamiliar with the project as it stands, so will need some time and/or guidance on finding my way around.

Let me know what you think!

@litstrong
Copy link
Collaborator

litstrong commented Sep 27, 2023

Hi Rory, defintely welcome to contribute! It is worth to mention new features for j2objc is lower priority to us given that we are expermenting Kotlin multiplatform. The future of j2objc needs to be evaluted based on the experiment results.

I had one change here by supporting the annotation @GenerateObjectiveCGenerics, so that we only enable Generics conversion for the annotated types. I think that could be a good example to trace down to see how it works.

@mungler
Copy link
Author

mungler commented Sep 27, 2023

@litstrong thats sad news! Can I ask what the migration path looks like for Google? Kotlin multiplatform or similar?

@litstrong
Copy link
Collaborator

Hi Rory, sorry, I think my statement was not accurate, just updated my previous comment.

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

No branches or pull requests

3 participants