cross-language-cpp / djinni-generator

Command-line tool that generates gluecode from a djinni-IDL file
https://djinni.xlcpp.dev/djinni-generator/setup
Apache License 2.0
94 stars 19 forks source link

Objective-C++ deprecation issues #166

Open sirnacnud opened 6 months ago

sirnacnud commented 6 months ago

I am running in to an issue with the new deprecation support. I have an interface that is marked as deprecated, and I am generating Objective-C++ files, but looks like that generator wasn't changed in the deprecation support. When I try to compile the Objective-C++ files, it triggers a deprecation warning (or compile error if warnings are errors), as it is using the deprecated C++ type.

The problem is the ::MyInterface C++ type used in the class is deprecated. Does anyone have thoughts on how to resolve this?

idl:

# interface comment
#
# @deprecated Use someother interface
my_interface = interface +o {
  # @deprecated Use someother method
  method_a(value:i32);
}

my_interface.hpp:

// AUTOGENERATED FILE - DO NOT MODIFY!
// This file was generated by Djinni from test.djinni

#pragma once

#include <cstdint>

/**
 * interface comment
 *
 * @deprecated Use someother interface
 */
class [[deprecated("Use someother interface")]] MyInterface {
public:
    virtual ~MyInterface() {}

    /** @deprecated Use someother method */
    [[deprecated("Use someother method")]]
    virtual void method_a(int32_t value) = 0;
};

MyInterface+Private.h:

// AUTOGENERATED FILE - DO NOT MODIFY!
// This file was generated by Djinni from test.djinni

#include "my_interface.hpp"
#include <memory>

static_assert(__has_feature(objc_arc), "Djinni requires ARC to be enabled for this file");

@protocol MyInterface;

namespace djinni_generated {

class MyInterface
{
public:
    using CppType = std::shared_ptr<::MyInterface>;
    using CppOptType = std::shared_ptr<::MyInterface>;
    using ObjcType = id<MyInterface>;

    using Boxed = MyInterface;

    static CppType toCpp(ObjcType objc);
    static ObjcType fromCppOpt(const CppOptType& cpp);
    static ObjcType fromCpp(const CppType& cpp) { return fromCppOpt(cpp); }

private:
    class ObjcProxy;
};

}  // namespace djinni_generated
a4z commented 6 months ago

Hi, I am not sure if I understand the problem correctly. Is it that the __deprecated_msg("...") in the Objective-C part is missing?

Mabye @eakoli can help more than I, since the deprecation feature comes from him.

sirnacnud commented 6 months ago

The problematic code is this in the Objective-C++ generated conversion code. The ::MyInterface is deprecated in C++, so this will trigger a deprecation warning when compiling the above header MyInterface+Private.h file.

class MyInterface
{
public:
    // Theses are deprecated in C++
    using CppType = std::shared_ptr<::MyInterface>;
    using CppOptType = std::shared_ptr<::MyInterface>;
    ...
};

As a work around we can mark this C++ class as deprecated and that does eliminate the warning. I just wasn't for sure if this is the correct fix.

class [[deprecated("Use someother interface")]] MyInterface 
{
public:
    using CppType = std::shared_ptr<::MyInterface>;
    using CppOptType = std::shared_ptr<::MyInterface>;
    ...
};
jothepro commented 6 months ago

In pydjinni I've worked around the issue by disabling deprecation warnings in the affected code, e.g. like this:

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
class MyInterface
{
public:
    // Theses are deprecated in C++
    using CppType = std::shared_ptr<::MyInterface>;
    using CppOptType = std::shared_ptr<::MyInterface>;
    ...
};
#pragma clang diagnostic pop

Not sure if that's the smartest approach, though.

If marking the generated translator class as deprecated does work as you describe, that sounds like a really nice solution to me!

eakoli commented 6 months ago

Yea, unfortunately that is one of the issue with marking code as deprecated, as sometimes those methods / types still need to be used in the implementation. So you end up with code that will generate warnings.

The only real solution is to disable the warnings, which would need to be done in all the generated code and implementations of the exposed interfaces. This could be problematic if it's done automatically as its possible that it hides other uses of deprecated items, that you actually DO want to know about.

My specific reason for adding this in was more to communicate to our android/iOS developers what APIs that they should move off of, and we dont have deprecation as an error, only a warning.

sirnacnud commented 6 months ago

For the project I am using Djinni with, we have warnings defined as errors, so the Djinni generated code that has warnings can't be compiled. I understand not all projects choose this option.

From an API standpoint, I would think compiling the generated API shouldn't create any warnings. Only when the client code tries to use the deprecated Djinni APIs should it create a warning. I think from a conceptual standpoint it makes sense to declare this C++ proxy as deprecated as well, as all the types it handles converting are already deprecated. This does solve the warnings as I mentioned above. I don't think there is any risk in hiding other unrelated deprecation warnings, as the proxy only has a few methods for converting from/to C++/Objective-C. Is there any objection to this? I can push up a PR for it.

a4z commented 6 months ago

Maybe @eakoli is the best person to answer that question since he introduced the feature.

For me, it's hard to estimate what this would break on existing code. I kind of agree that compiling the generated API should probably not create warnings, and what Duncan suggests sounds reasonable, but personally, I have no use cases in my current projects affected by that, so I do not feel I am able to give a qualified statement on that topic.