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

Collisions between field and nested type names cause compile errors #1185

Open
jonpryor opened this issue Feb 7, 2024 · 1 comment
Open
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jonpryor
Copy link
Member

jonpryor commented Feb 7, 2024

Context: https://developercommunity.visualstudio.com/t/I-am-using-the-Maui-class-library-to-imp/10579664

When the binding of a method collides with a nested type, we prefix the method name with Invoke:

https://github.com/xamarin/java.interop/blob/7f08b77f3464f2b276ec5edd2e4836b1915f86dd/tools/generator/Java.Interop.Tools.Generator.ObjectModel/GenBase.cs#L353

We don't appear to have any equivalent behavior when a field name collides with a nested type or property. This can result in CS0102 errors.

For example, consider the wechat-sdk-android-6.8.26.aar library. When we try to bind this, it fails:

obj/Debug/net8.0-android/generated/src/Com.Tencent.MM.Opensdk.Modelbase.BaseResp.cs(24,14): error CS0102: The type 'BaseResp' already contains a definition for 'ErrCode'
obj/Debug/net8.0-android/generated/src/Com.Tencent.MM.Opensdk.Modelmsg.WXMediaMessage.cs(54,79): error CS0102: The type 'WXMediaMessage' already contains a definition for 'MediaObject'

Relevant context:

abstract partial class BaseRep {
    // Metadata.xml XPath field reference: path="/api/package[@name='com.tencent.mm.opensdk.modelbase']/class[@name='BaseResp']/field[@name='errCode']"
    [Register ("errCode")]
    // line 24 follows
    public int ErrCode {}

    public abstract partial class ErrCode : Java.Lang.Object {
    }
}

partial class WXMediaMessage {
    // Metadata.xml XPath field reference: path="/api/package[@name='com.tencent.mm.opensdk.modelmsg']/class[@name='WXMediaMessage']/field[@name='mediaObject']"
    [Register ("mediaObject")]
    // line 54 follows
    public IMediaObject? MediaObject {}

    partial class MediaObject : Java.Lang.Object {
    }
}

If we had automagic renaming of fields names so that they wouldn't collide with nested types, as we do with methods, this library would have been bindable without Metadata.

Metadata which allows the library to build:

<metadata>
  <attr
      path="/api/package[@name='com.tencent.mm.opensdk.modelbase']/class[@name='BaseResp']/field[@name='errCode']"
      name="managedName"
  >ErrorCode</attr>

  <attr
      path="/api/package[@name='com.tencent.mm.opensdk.modelmsg']/class[@name='WXMediaMessage']/field[@name='mediaObject']"
      name="managedName"
  >MediaObjectField</attr>

</metadata>
@jpobst
Copy link
Contributor

jpobst commented Feb 7, 2024

Capturing some discussion on Discord about this:

[10:31 AM] jpobst: weird, i would have thought we would just not bind the fields
[10:31 AM] Jonathan Pryor: for that lib, they were bound and caused build errors. 🙂
[10:31 AM] jpobst: the problem we have with "automagic" renaming is that it can break existing API, so we've never implemented it before 
[10:32 AM] Jonathan Pryor: in this case, it would prevent a build failure, so it couldn't possibly break existing API. 🙂
[10:32 AM] jpobst: ie:
            - API 1.0 only has the field, so we call it "Foo"
            - API 2.0 adds the nested type, so we rename the field to "FooField"
[10:32 AM] Jonathan Pryor: ugh.
[10:33 AM] Jonathan Pryor: then we'd need apiSince support
[10:34 AM] jpobst: gustavo did a lot of work in this area, which we eventually decided we didn't want: https://github.com/xamarin/java.interop/pull/504
[10:35 AM] jpobst: i guess "not binding the conflicting field" is also a potential API break 
[10:38 AM] Jonathan Pryor: i fear we're hitting a "perfect is the enemy of the good (or default)" scenario
[10:39 AM] Jonathan Pryor: i'd prefer to have a "works reasonable for default purposes".  Mono.Android.dll has separate higher-level API compat checks.
[10:39 AM] jpobst: yeah, i think if we were starting over (generator2?), we should probably choose better automatic bindings over API compatibility
[10:39 AM] jpobst: i suspect we're nearly the only ones that care about that level of API compat
[10:39 AM] Jonathan Pryor: a new binding util does make things like that easier
[10:40 AM] Jonathan Pryor: in that if we ship both, people can always opt for the previous generator if necessary
[10:40 AM] jpobst: and of course we have tools in place to prevent API breaks, and the knowledge to fix them
[10:45 AM] jpobst: i guess we could implement it (and related features) in generator behind a $(AndroidGeneratorStrictAPICompatibility)=false flag or something 

@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Feb 7, 2024
@jpobst jpobst changed the title Field+Nested Type Collision? Collisions between field and nested type names cause compile errors Feb 7, 2024
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

2 participants