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

Generator should bind types with unresolvable interfaces #371

Open
jonpryor opened this issue Sep 11, 2018 · 8 comments
Open

Generator should bind types with unresolvable interfaces #371

jonpryor opened this issue Sep 11, 2018 · 8 comments
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jonpryor
Copy link
Member

Context: Issue #358

If we have a //class which implements unresolvable //interface types, can we still bind that class?

For example, consider android.support.v7.widget.RecyclerView, which implements the interface android.support.v4.view.ScrollingView, which is either a package-private type or otherwise can't be resolved (different .jar file?).

Why can't generator bind this type?

Why shouldn't generator bind this type?

We should determine if this is actually a supportable/bindable scenario, and then improve generator so that such types are actually bound.

@erikpowa
Copy link

A.jar has public RecyclerView ||| B.jar has private ScrollingView

  1. { Binding A without referencing B } -> don't bind RecyclerView, since user doesn't provide B therefore nothing to do there
  2. (((Binding A without referencing B + $(ForceBindTypesAnyway) -> generate a dummy stripped RecyclerView)))
  3. { Binding A with reference B } -> don't bind RecyclerView + throw warning for invalid encapsulation at ScrollingViews
  4. { Binding A with reference B with user specified transform file } -> bind RecyclerView, where the transform file has visibility=public for ScrollingViews

1.,3.,4. theoretically would work for me if ACW won't cry for the visibility=public

As a user, If I don't provide everything for RecyclerView that means I don't want that type to be generated/binded in the first place, but when I provide everything (references + transforms) then I expect the type to be binded.

@jonpryor
Copy link
Member Author

This isn't a question around ScrollingView; we can't bind a type that we know nothing about, and a "dummy stripped ScrollingView" would do more harm than good.

The question is solely around RecyclerView: we have its definition, it's a public type, it "just happens" to implement an unresolvable interface type, meaning we cannot even determine if the interface type itself is public or not. Should the fact that the interface isn't resolvable prevent the binding of the RecyclerView type?

Your argument is that no, it should not be bound:

  1. { Binding A without referencing B } -> don't bind RecyclerView, since user doesn't provide B therefore nothing to do there

I disagree, because if you were using the .jar from Java, RecyclerView would be usable, even though ScrollingView may not be.

  1. (((Binding A without referencing B + $(ForceBindTypesAnyway) -> generate a dummy stripped RecyclerView)))

I don't know what a "dummy stripped RecyclerView" would be, but I'm imagine I'd still disagree: we should bind it as well as possible, because that would give an experience closer to Java.

  1. { Binding A with reference B } -> don't bind RecyclerView + throw warning for invalid encapsulation at ScrollingView

That is entirely contrary to how our current binding semantics work. If you have a Binding project "A" which binds A.jar which contains the type RecyclerView, and "A" references another assembly/library project "B" which binds a .jar which provides a public ScrollingView, then yes RecyclerView will be fully bound. That's how it's worked for years.

1.,3.,4. theoretically would work for me if ACW won't cry for the visibility=public

I don't understand what you mean by this.

If I try to guess, and:

  1. RecyclerView is a package-private type, and
  2. The previous hypothetical "B" library binds RecyclerView by using metadata so that visibility=public for ScrollingView

Then ACWs may not work. They will not work if ScrollingView is referenced, e.g.

public class MyView : Java.Lang.Object, IScrollingView {
    // ...
}

The above C# class would result in the Java Callable Wrapper:

public class MyView extends java.lang.Object implements package.of.ScrollingView {
    // ...
}

which will fail, because RecyclerView is a package-private type.

As such, the only way that we can sanely bind RecyclerView is by using metadata to remove SurfaceView:

<remove-node path="//class[@name='RecyclerView']/implements[@name='package.of. view.ScrollingView']" />

Then we can bind SurfaceView, and the binding won't mention RecyclerView.

Why should this manual transformation be required?

@erikpowa
Copy link

Then ACWs may not work. They will not work if ScrollingView is referenced, e.g.

The above C# class would result in the Java Callable Wrapper:

😄 yeah I know visibility=public is stupid but, not for implementation but for binding sake.

"A" references another assembly/library project "B" which binds a .jar which provides a public ScrollingView, then yes RecyclerView will be fully bound. That's how it's worked for years.

It's not in every scenario right? (I got 1 binding binding project where the types 100% resolvable, yet types are going missing during binding without XA warnings)

<remove-node path="//class[@name='RecyclerView']/implements[@name='package.of. view.ScrollingView']" />

Wait, would it work right now or isn't the RecyclerView become unbindable?

@jonpryor
Copy link
Member Author

It's not in every scenario right? (I got 1 binding binding project where the types 100% resolvable, yet types are going missing during binding without XA warnings)

I would like to see this project, as I do not currently understand how or why that would happen.

Wait, would it work right now or isn't the RecyclerView become unbindable?

That would work now, and would allow RecyclerView to be bound and used. The "problem" is that this is a manual intervention, and ideally we should require as few manual interventions as possible. (Not that we actually meet such an ideal, but ideally we'll be able to meet such an ideal...)

@erikpowa
Copy link

erikpowa commented Nov 4, 2018

@jonpryor I was planning to, as soon as I get to work with XA again.

@jonpryor
Copy link
Member Author

One "corner case" which just came to mind: we cannot bind abstract types which unresolvable interface methods automagically/by default.

Consider:

interface Fooable {
    void foo();
}

public abstract class X implements Fooable {
}

When this is compiled, both javap -v -p -c -s -constants X and class-parse.exe X.class, the X type does not declare a foo() method. Consequently, if the Fooable interface type cannot be found, then there is no way to know which abstract methods the bound X should provide, and if the binding for X doesn't bind/declare foo(), then X cannot be subclassed or instantiated.

@erikpowa
Copy link

erikpowa commented Nov 14, 2018

Just popped to my mind. I guess I realized where/what was my problem. The structure look like this (haven't created repro yet to test):

Binding Project A binds A.jar. Binding Project B binds B.jar. A ref B.
B.jar contains Foo. A.jar contains Bar.

@Deprecated
public interface Foo{ // B.jar
}

public interface Bar extends Foo { // A.jar
}

Binding Project B has transform:
<remove-node path="//*[@deprecated='deprecated']" />

Is Bar (can be) binded in Binding Project A? I assumed it will, but I guess it's not the case right now which would explain the unexpected behaviours for me and pretty similar to the first scenario where Foo isn't represent, yet the Bar still should be binded. Only differece is that in this scenario "we" have the (valid, public, non-abstract) metadata for Foo which lays in B.jar. (or it's a bug and should be binded by default)

Edit: and I guess the way to do it would be the same as earlier:
<remove-node path="//class[@name='Bar']/implements[@name='Foo']" />
So manual intervention is required here as well.

@mattleibow
Copy link
Member

@jonpryor Is this a similar issue: #359

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

No branches or pull requests

4 participants