lauromoura pushed a commit to branch master. http://git.enlightenment.org/core/efl.git/commit/?id=dfb856158c8ea37d9caa170a6794447ec18ecdd9
commit dfb856158c8ea37d9caa170a6794447ec18ecdd9 Author: Lauro Moura <[email protected]> Date: Tue Sep 10 19:30:46 2019 -0300 csharp: Skip non-public members from interfaces Summary: Eolian allows non-public members in interfaces and mixins (usually @protected). As both kinds are converted to C# interfaces, this causes problem as non-public members are forbidden in C# interfaces. This commit changes eolian_mono by removing those members from the C# interfaces. If a generated class implements the interface, the method is generated as if it were a protected member of the class directly. For mixed properties like `Efl.Io.Reader.CanRead { get; set @protected; }`, the interface has only the public getter and the the implementing class has both the public getter and the protected setter. With this, C# devs won't be able to directly implement protected Eo methods from interfaces. (But this really does not make sense from the C# point of view). ref T7494 Reviewers: segfaultxavi, felipealmeida, YOhoho Reviewed By: YOhoho Subscribers: cedric, brunobelo, Jaehyun_Cho, #reviewers, woohyun, #committers Tags: #efl Maniphest Tasks: T7494 Differential Revision: https://phab.enlightenment.org/D9800 --- .../eolian/mono/async_function_definition.hh | 3 + src/bin/eolian_mono/eolian/mono/blacklist.hh | 12 ++++ src/bin/eolian_mono/eolian/mono/documentation.hh | 21 ++++++ .../eolian/mono/function_declaration.hh | 4 ++ .../eolian_mono/eolian/mono/function_definition.hh | 42 +++++++----- .../eolian_mono/eolian/mono/function_helpers.hh | 7 +- .../eolian/mono/function_registration.hh | 5 ++ src/bin/eolian_mono/eolian/mono/helpers.hh | 33 ++++++++++ src/bin/eolian_mono/eolian/mono/klass.hh | 20 ++---- src/bindings/mono/efl_mono/Bind.cs | 10 ++- src/lib/eolian_cxx/grammar/klass_def.hpp | 20 ++++++ src/tests/efl_mono/Eo.cs | 74 ++++++++++++++++++++++ src/tests/efl_mono/dummy_interfaces.c | 2 + src/tests/efl_mono/dummy_test_iface.eo | 31 +++++++++ src/tests/efl_mono/dummy_test_object.c | 12 ++++ src/tests/efl_mono/dummy_test_object.eo | 2 + 16 files changed, 262 insertions(+), 36 deletions(-) diff --git a/src/bin/eolian_mono/eolian/mono/async_function_definition.hh b/src/bin/eolian_mono/eolian/mono/async_function_definition.hh index b3ab6d4102..a6907a2a95 100644 --- a/src/bin/eolian_mono/eolian/mono/async_function_definition.hh +++ b/src/bin/eolian_mono/eolian/mono/async_function_definition.hh @@ -52,6 +52,9 @@ struct async_function_declaration_generator return true; if (!f.return_type.original_type.visit(is_future{})) return true; + // C# interfaces can't have non-public members + if(f.scope != attributes::member_scope::scope_public) + return true; if (!as_generator( scope_tab << "/// <summary>Async wrapper for <see cref=\"" << name_helpers::managed_method_name(f) << "\" />.</summary>\n" diff --git a/src/bin/eolian_mono/eolian/mono/blacklist.hh b/src/bin/eolian_mono/eolian/mono/blacklist.hh index 5099013396..c46f2f4de7 100644 --- a/src/bin/eolian_mono/eolian/mono/blacklist.hh +++ b/src/bin/eolian_mono/eolian/mono/blacklist.hh @@ -64,6 +64,18 @@ inline bool is_function_blacklisted(attributes::function_def const& func, Contex return is_function_blacklisted(c_name); } +inline bool is_non_public_interface_member(attributes::function_def const& func, attributes::klass_def const¤t_klass) +{ + if (current_klass.type == attributes::class_type::interface_ + || current_klass.type == attributes::class_type::mixin) + { + if (func.scope != attributes::member_scope::scope_public) + return true; + } + + return false; +} + // Blacklist structs that require some kind of manual binding. inline bool is_struct_blacklisted(std::string const& full_name) diff --git a/src/bin/eolian_mono/eolian/mono/documentation.hh b/src/bin/eolian_mono/eolian/mono/documentation.hh index 588d0b68fa..6cf57fd342 100644 --- a/src/bin/eolian_mono/eolian/mono/documentation.hh +++ b/src/bin/eolian_mono/eolian/mono/documentation.hh @@ -73,6 +73,25 @@ struct documentation_generator // Klass is needed to check the property naming rulles attributes::klass_def klass_d((const ::Eolian_Class *)klass, eolian_object_unit_get(klass)); + // Comment the block below to enable @see reference conversion for non-public interface members. + // As they are not generated, this causes a doc warning that fails the build, but can be useful to track + // public methods referencing protected stuff. + if (ftype != EOLIAN_PROPERTY) + { + bool is_func_public = ::eolian_function_scope_get(function, ftype) == EOLIAN_SCOPE_PUBLIC; + + if (helpers::is_managed_interface(klass_d) && !is_func_public) + return ""; + } + else + { + bool is_get_public = ::eolian_function_scope_get(function, EOLIAN_PROP_GET) == EOLIAN_SCOPE_PUBLIC; + bool is_set_public = ::eolian_function_scope_get(function, EOLIAN_PROP_SET) == EOLIAN_SCOPE_PUBLIC; + + if (helpers::is_managed_interface(klass_d) && !(is_get_public || is_set_public)) + return ""; + } + switch(ftype) { case ::EOLIAN_METHOD: @@ -112,6 +131,8 @@ struct documentation_generator static std::string function_conversion(attributes::function_def const& func) { + // This function is called only from the constructor reference conversion, so it does not + // need to check whether this function non-public in a interface returning an empty reference (yet). std::string name = name_helpers::klass_full_concrete_or_interface_name(func.klass); switch (func.type) { diff --git a/src/bin/eolian_mono/eolian/mono/function_declaration.hh b/src/bin/eolian_mono/eolian/mono/function_declaration.hh index baad0bb33d..4532746be2 100644 --- a/src/bin/eolian_mono/eolian/mono/function_declaration.hh +++ b/src/bin/eolian_mono/eolian/mono/function_declaration.hh @@ -23,6 +23,10 @@ struct function_declaration_generator if(blacklist::is_function_blacklisted(f, context) || f.is_static) return true; + // C# interfaces can't have non-public members + if(f.scope != attributes::member_scope::scope_public) + return true; + if(!as_generator(documentation).generate(sink, f, context)) return false; diff --git a/src/bin/eolian_mono/eolian/mono/function_definition.hh b/src/bin/eolian_mono/eolian/mono/function_definition.hh index 81e2ad81ef..51da954d44 100644 --- a/src/bin/eolian_mono/eolian/mono/function_definition.hh +++ b/src/bin/eolian_mono/eolian/mono/function_definition.hh @@ -81,6 +81,10 @@ struct native_function_definition_generator .generate(sink, std::make_tuple(f.c_name, f.c_name, f.c_name, f.c_name), context)) return false; + // We do not generate the wrapper to be called from C for non public interface member directly. + if (blacklist::is_non_public_interface_member(f, *klass)) + return true; + // Actual method implementation to be called from C. std::string return_type; if(!as_generator(eolian_mono::type(true)).generate(std::back_inserter(return_type), f.return_type, context)) @@ -303,12 +307,12 @@ struct property_wrapper_definition_generator if (blacklist::is_property_blacklisted(property, *implementing_klass, context)) return true; - bool interface = context_find_tag<class_context>(context).current_wrapper_kind == class_context::interface; + bool is_interface = context_find_tag<class_context>(context).current_wrapper_kind == class_context::interface; bool is_static = (property.getter.is_engaged() && property.getter->is_static) || (property.setter.is_engaged() && property.setter->is_static); - if (interface && is_static) + if (is_interface && is_static) return true; auto get_params = property.getter.is_engaged() ? property.getter->parameters.size() : 0; @@ -389,19 +393,21 @@ struct property_wrapper_definition_generator std::string scope = "public "; std::string get_scope = property.getter.is_engaged() ? eolian_mono::function_scope_get(*property.getter) : ""; + bool is_get_public = get_scope == "public "; std::string set_scope = property.setter.is_engaged() ? eolian_mono::function_scope_get(*property.setter) : ""; - if (interface) + bool is_set_public = set_scope == "public "; + + // No need to generate this wrapper as no accessor is public. + if (is_interface && (!is_get_public && !is_set_public)) + return true; + + // C# interface members are declared automatically as public + if (is_interface) { scope = ""; get_scope = ""; set_scope = ""; } - else if ((property.klass.type == attributes::class_type::mixin) || - (property.klass.type == attributes::class_type::interface_)) - { - get_scope = ""; - set_scope = ""; - } else if ((get_scope != "") && (get_scope == set_scope)) { scope = get_scope; @@ -439,11 +445,12 @@ struct property_wrapper_definition_generator return false; } - if (property.getter.is_engaged() && interface) + if (property.getter.is_engaged() && is_interface) { - if (!as_generator(scope_tab << scope_tab << set_scope << "get;\n" - ).generate(sink, attributes::unused, context)) - return false; + if (is_get_public) + if (!as_generator(scope_tab << scope_tab << set_scope << "get;\n" + ).generate(sink, attributes::unused, context)) + return false; } else if (property.getter.is_engaged() && get_params == 0/*parameters.size() == 1 && property.getter.is_engaged()*/) { @@ -483,11 +490,12 @@ struct property_wrapper_definition_generator // return false; // } - if (property.setter.is_engaged() && interface) + if (property.setter.is_engaged() && is_interface) { - if (!as_generator(scope_tab << scope_tab << set_scope << "set;\n" - ).generate(sink, attributes::unused, context)) - return false; + if (is_set_public) + if (!as_generator(scope_tab << scope_tab << set_scope << "set;\n" + ).generate(sink, attributes::unused, context)) + return false; } else if (parameters.size() == 1 && property.setter.is_engaged()) { diff --git a/src/bin/eolian_mono/eolian/mono/function_helpers.hh b/src/bin/eolian_mono/eolian/mono/function_helpers.hh index 0ea25da536..5f7cf67877 100644 --- a/src/bin/eolian_mono/eolian/mono/function_helpers.hh +++ b/src/bin/eolian_mono/eolian/mono/function_helpers.hh @@ -165,10 +165,6 @@ function_definition_epilogue_generator const as_generator(function_definition_ep inline std::string function_scope_get(attributes::function_def const& f) { - if ((f.klass.type == attributes::class_type::mixin) || - (f.klass.type == attributes::class_type::interface_)) - return "public "; - switch (f.scope) { case attributes::member_scope::scope_public: @@ -178,7 +174,8 @@ inline std::string function_scope_get(attributes::function_def const& f) case attributes::member_scope::scope_protected: return "protected "; case attributes::member_scope::scope_unknown: - return " "; + // This should trigger a compilation error + return "unkown_scope "; } return " "; } diff --git a/src/bin/eolian_mono/eolian/mono/function_registration.hh b/src/bin/eolian_mono/eolian/mono/function_registration.hh index b490808b9a..e258ce7cff 100644 --- a/src/bin/eolian_mono/eolian/mono/function_registration.hh +++ b/src/bin/eolian_mono/eolian/mono/function_registration.hh @@ -35,6 +35,11 @@ struct function_registration_generator if(blacklist::is_function_blacklisted(f, context) || f.is_static) // Static methods aren't overrideable return true; + // We do not generate registration wrappers for non public interface/mixin members in their concrete classes. + // They go in the first concrete/abstract implementation. + if(blacklist::is_non_public_interface_member(f, *klass)) + return true; + if(!as_generator( indent << "if (" << f.c_name << "_static_delegate == null)\n" << indent << "{\n" diff --git a/src/bin/eolian_mono/eolian/mono/helpers.hh b/src/bin/eolian_mono/eolian/mono/helpers.hh index a7c27c4809..291e5234ff 100644 --- a/src/bin/eolian_mono/eolian/mono/helpers.hh +++ b/src/bin/eolian_mono/eolian/mono/helpers.hh @@ -233,6 +233,39 @@ std::vector<attributes::function_def> get_all_implementable_methods(attributes:: return ret; } +template<typename Klass> +inline bool is_managed_interface(Klass const& klass) +{ + return klass.type == attributes::class_type::interface_ + || klass.type == attributes::class_type::mixin; +} + + +/* + * Gets all methods that this class should register (i.e. that comes from it and non-public interface methods + * that this class is the first one implementing) + */ +template<typename Context> +std::vector<attributes::function_def> get_all_registerable_methods(attributes::klass_def const& cls, Context const& context) +{ + std::vector<attributes::function_def> ret; + + auto implementable_methods = get_all_implementable_methods(cls, context); + + std::copy_if(implementable_methods.cbegin(), implementable_methods.cend(), std::back_inserter(ret) + , [&cls](attributes::function_def const & func) { + + if (cls == func.klass) + return true; + + if (!is_managed_interface(func.klass) || func.scope != attributes::member_scope::scope_public) + return true; + return false; + }); + + return ret; +} + /* * Checks whether the given is unique going up the inheritance tree from leaf_klass */ diff --git a/src/bin/eolian_mono/eolian/mono/klass.hh b/src/bin/eolian_mono/eolian/mono/klass.hh index 81d1070aab..80ac37c141 100644 --- a/src/bin/eolian_mono/eolian/mono/klass.hh +++ b/src/bin/eolian_mono/eolian/mono/klass.hh @@ -31,18 +31,6 @@ namespace eolian_mono { -/* Get the actual number of functions of a class, checking for blacklisted ones */ -template<typename Context> -static std::size_t -get_implementable_function_count(grammar::attributes::klass_def const& cls, Context context) -{ - auto methods = helpers::get_all_implementable_methods(cls, context); - return std::count_if(methods.cbegin(), methods.cend(), [&context](grammar::attributes::function_def const& func) - { - return !blacklist::is_function_blacklisted(func, context) && !func.is_static; - }); -} - template<typename Context> static bool is_inherit_context(Context const& context) @@ -239,6 +227,9 @@ struct klass if (!generate_events(sink, cls, concrete_cxt)) return false; + if (!as_generator(lit("#pragma warning disable CS0628\n")).generate(sink, attributes::unused, concrete_cxt)) + return false; + // Parts if(!as_generator(*(part_definition)) .generate(sink, cls.parts, concrete_cxt)) return false; @@ -263,6 +254,9 @@ struct klass return false; } + if (!as_generator(lit("#pragma warning restore CS0628\n")).generate(sink, attributes::unused, concrete_cxt)) + return false; + // Copied from nativeinherit class, used when setting up providers. if(!as_generator( scope_tab << "private static IntPtr GetEflClassStatic()\n" @@ -398,7 +392,7 @@ struct klass context); auto native_inherit_name = name_helpers::klass_native_inherit_name(cls); auto inherit_name = name_helpers::klass_inherit_name(cls); - auto implementable_methods = cls.functions; + auto implementable_methods = helpers::get_all_registerable_methods(cls, context); bool root = !helpers::has_regular_ancestor(cls); auto const& indent = current_indentation(inative_cxt); diff --git a/src/bindings/mono/efl_mono/Bind.cs b/src/bindings/mono/efl_mono/Bind.cs index d82d0c1f49..1d29c25d42 100644 --- a/src/bindings/mono/efl_mono/Bind.cs +++ b/src/bindings/mono/efl_mono/Bind.cs @@ -3,6 +3,7 @@ using System; using System.Runtime.InteropServices; using System.Collections.Generic; +using System.Reflection; using System.Linq; using System.ComponentModel; @@ -47,8 +48,15 @@ public class BindableProperty<T> throw new InvalidOperationException($"Failed to cast binder {binder} to IPart"); } - var partBinder = partHolder.GetPart(this.partName) as Efl.Ui.IPropertyBind; + // We rely on reflection as GetPart is protected and not generated in IPart. + var partMethod = partHolder.GetType().GetMethod("GetPart", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); + if (partMethod == null) + { + throw new InvalidOperationException($"Failed to get 'GetPart' method on property binder"); + } + + var partBinder = partMethod.Invoke(partHolder, new System.Object[] { this.partName }) as Efl.Ui.IPropertyBind; if (partBinder != null) { return partBinder.PropertyBind(this.propertyName, modelProperty); diff --git a/src/lib/eolian_cxx/grammar/klass_def.hpp b/src/lib/eolian_cxx/grammar/klass_def.hpp index 108be02f79..683bb000a4 100644 --- a/src/lib/eolian_cxx/grammar/klass_def.hpp +++ b/src/lib/eolian_cxx/grammar/klass_def.hpp @@ -1277,6 +1277,26 @@ struct klass_def || lhs.parts < rhs.parts; } + friend inline bool operator==(klass_def const& lhs, klass_name const& rhs) + { + return lhs.namespaces == rhs.namespaces + && lhs.eolian_name == rhs.eolian_name + && lhs.type == rhs.type; + } + friend inline bool operator==(klass_name const& lhs, klass_def const& rhs) + { + return rhs == lhs; + } + friend inline bool operator!=(klass_def const& lhs, klass_name const& rhs) + { + return !(lhs == rhs); + } + friend inline bool operator!=(klass_name const& lhs, klass_def const& rhs) + { + return !(rhs == lhs); + } + + klass_def(std::string eolian_name, std::string cxx_name, std::string filename , documentation_def documentation , std::vector<std::string> namespaces diff --git a/src/tests/efl_mono/Eo.cs b/src/tests/efl_mono/Eo.cs index 56813b3f87..efb1faa306 100644 --- a/src/tests/efl_mono/Eo.cs +++ b/src/tests/efl_mono/Eo.cs @@ -1,6 +1,7 @@ using System; using System.Linq; using System.Collections.Generic; +using System.Reflection; namespace TestSuite { @@ -533,4 +534,77 @@ class TestObjectDeletion } } +class TestProtectedInterfaceMembers +{ + + private class MyObject : Dummy.TestObject + { + public MyObject(Efl.Object parent = null) : base(parent) + { + } + + protected override int MethodProtected(int x) + { + return x * x; + } + } + + public static void test_protected_interface_in_generated_class_called_from_c() + { + var obj = new Dummy.TestObject(); + Test.AssertEquals(obj.CallMethodProtected(42), -42); + } + + public static void test_protected_interface_in_inherited_class_called_from_c() + { + var obj = new MyObject(); + Test.AssertEquals(obj.CallMethodProtected(42), 42 * 42); + } + + public static void test_interface_skipped_protected() + { + var type = typeof(Dummy.ITestIface); + var methods = type.GetMethods(BindingFlags.Public | BindingFlags.Instance); + + // Fully protected property + Test.AssertNull(methods.SingleOrDefault(m => m.Name == "GetProtectedProp")); + Test.AssertNull(methods.SingleOrDefault(m => m.Name == "SetProtectedProp")); + + // Partially protected property + Test.AssertNotNull(methods.SingleOrDefault(m => m.Name == "GetPublicGetterPrivateSetter")); + Test.AssertNull(methods.SingleOrDefault(m => m.Name == "SetPublicGetterPrivateSetter")); + + var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance); + Test.AssertNull(properties.SingleOrDefault(m => m.Name == "ProtectedProp")); + Test.AssertNotNull(properties.SingleOrDefault(m => m.Name == "PublicGetterPrivateSetter")); + } + + public static void test_interface_skipped_protected_in_implementation() + { + var type = typeof(Dummy.TestObject); + + // Fully protected property + var protected_methods = type.GetMethods(BindingFlags.NonPublic | BindingFlags.Instance).Where(m => m.IsFamily); + Test.AssertNotNull(protected_methods.SingleOrDefault(m => m.Name == "GetProtectedProp")); + Test.AssertNotNull(protected_methods.SingleOrDefault(m => m.Name == "SetProtectedProp")); + + // Partially protected property + var public_methods = type.GetMethods(BindingFlags.Public | BindingFlags.Instance); + Test.AssertNotNull(public_methods.SingleOrDefault(m => m.Name == "GetPublicGetterPrivateSetter")); + Test.AssertNotNull(protected_methods.SingleOrDefault(m => m.Name == "SetPublicGetterPrivateSetter")); + + var protected_properties = type.GetProperties(BindingFlags.NonPublic | BindingFlags.Instance); + var prop = protected_properties.SingleOrDefault(m => m.Name == "ProtectedProp"); + Test.AssertNotNull(prop); + Test.Assert(prop.GetMethod.IsFamily); + Test.Assert(prop.SetMethod.IsFamily); + + var public_properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance); + prop = public_properties.SingleOrDefault(m => m.Name == "PublicGetterPrivateSetter"); + Test.AssertNotNull(prop); + Test.Assert(prop.GetMethod.IsPublic); + Test.Assert(prop.SetMethod.IsFamily); + } +} + } diff --git a/src/tests/efl_mono/dummy_interfaces.c b/src/tests/efl_mono/dummy_interfaces.c index f4b3ca5948..c60ec59f0a 100644 --- a/src/tests/efl_mono/dummy_interfaces.c +++ b/src/tests/efl_mono/dummy_interfaces.c @@ -1,4 +1,6 @@ // Include file for interfaces .eo.c files +#define DUMMY_TEST_IFACE_PROTECTED + #include "libefl_mono_native_test.h" #include "dummy_test_iface.eo.c" diff --git a/src/tests/efl_mono/dummy_test_iface.eo b/src/tests/efl_mono/dummy_test_iface.eo index 3ab37f5407..068b1352bd 100644 --- a/src/tests/efl_mono/dummy_test_iface.eo +++ b/src/tests/efl_mono/dummy_test_iface.eo @@ -10,6 +10,37 @@ interface Dummy.Test_Iface data: int; } } + + method_protected @protected @const { + params { + @in x: int; + } + return: int; + } + + call_method_protected @const { + params { + @in x: int; + } + return: int; + } + + @property protected_prop @protected { + get {} + set {} + values { + data: int; + } + } + + @property public_getter_private_setter { + get {} + set @protected {} + values { + data: int; + } + } + } events { nonconflicted: void; diff --git a/src/tests/efl_mono/dummy_test_object.c b/src/tests/efl_mono/dummy_test_object.c index 6c32f80ca1..a880dc40c6 100644 --- a/src/tests/efl_mono/dummy_test_object.c +++ b/src/tests/efl_mono/dummy_test_object.c @@ -1,4 +1,6 @@ +#define DUMMY_TEST_IFACE_PROTECTED + #include "libefl_mono_native_test.h" typedef struct Dummy_Test_Object_Data @@ -4704,6 +4706,16 @@ const Eina_Value_Type *_dummy_test_object_mirror_value_type(EINA_UNUSED const Eo return type; } +int _dummy_test_object_dummy_test_iface_method_protected(EINA_UNUSED const Eo *obj, EINA_UNUSED Dummy_Test_Object_Data *pd, int x) +{ + return -x; +} + +int _dummy_test_object_dummy_test_iface_call_method_protected(const Eo *obj, EINA_UNUSED Dummy_Test_Object_Data *pd, int x) +{ + return dummy_test_iface_method_protected(obj, x); +} + // Inherit int _dummy_inherit_helper_receive_dummy_and_call_int_out(Dummy_Test_Object *x) { diff --git a/src/tests/efl_mono/dummy_test_object.eo b/src/tests/efl_mono/dummy_test_object.eo index 2fa2c32c55..676b68a421 100644 --- a/src/tests/efl_mono/dummy_test_object.eo +++ b/src/tests/efl_mono/dummy_test_object.eo @@ -1653,6 +1653,8 @@ class Dummy.Test_Object extends Efl.Object implements Dummy.Test_Iface { Efl.Object.provider_find; Dummy.Test_Iface.emit_nonconflicted; Dummy.Test_Iface.iface_prop { get; set; } + Dummy.Test_Iface.method_protected; + Dummy.Test_Iface.call_method_protected; } events { evt,with,string @hot: const(string); --
