From 3f6bf4e9094a3aad60ecb104d33724bf9bf20bcc Mon Sep 17 00:00:00 2001 From: Frank Dellaert Date: Sun, 18 Oct 2026 13:59:00 -0400 Subject: [PATCH] Fix MATLAB interface boundaries and enum value wrapping --- gtwrap/matlab_wrapper/mixins.py | 45 +++++---- gtwrap/matlab_wrapper/wrapper.py | 130 ++++++++++++++----------- tests/expected/matlab/enum_wrapper.cpp | 4 +- tests/fixtures/matlab_enum_arguments.i | 29 ++++++ tests/test_matlab_wrapper.py | 102 +++++++++++++++++++ 5 files changed, 232 insertions(+), 78 deletions(-) create mode 100644 tests/fixtures/matlab_enum_arguments.i diff --git a/gtwrap/matlab_wrapper/mixins.py b/gtwrap/matlab_wrapper/mixins.py index 854a60a..6a76003 100644 --- a/gtwrap/matlab_wrapper/mixins.py +++ b/gtwrap/matlab_wrapper/mixins.py @@ -155,30 +155,35 @@ def is_pair(arg_type: parser.Type) -> bool: and arg_type.typename.qualified_name() in ('pair', 'std::pair') and len(arg_type.template_params) == 2) - def is_class_enum(self, arg_type: parser.Type, class_: parser.Class): - """Check if arg_type is an enum in the class `class_`.""" + def resolve_enum(self, arg_type: parser.Type, class_=None): + """Return the declared enum's C++ and MATLAB names, or None.""" + name = arg_type.typename.to_cpp() + scope = [] if class_: - class_enums = [enum.name for enum in class_.enums] - return arg_type.typename.name in class_enums - else: - return False - - def is_global_enum(self, arg_type: parser.Type, class_: parser.Class): - """Check if arg_type is a global enum.""" - if class_: - # Get the enums in the class' namespace - global_enums = [ - member.name for member in class_.parent.content - if isinstance(member, parser.Enum) - ] - return arg_type.typename.name in global_enums - else: - return False + scope = class_.namespaces()[1:] + [class_.name] + # Preserve this instantiation's MATLAB alias for its own enums. + owners = (class_.name, class_.original.name, + class_.cpp_typename().templated_name()) + namespaces = arg_type.typename.namespaces + is_local = (not namespaces or any( + namespaces == prefix + [owner] + for prefix in ([], class_.namespaces()[1:]) + for owner in owners)) + if is_local and any(enum.name == arg_type.typename.name + for enum in class_.enums): + return (f'{class_.to_cpp()}::{arg_type.typename.name}', + '.'.join(scope + [arg_type.typename.name])) + + # Try the local class/namespace scopes, then the fully qualified name. + for length in range(len(scope), -1, -1): + qualified = '::'.join(scope[:length] + [name]) + if qualified in self.enum_names: + return self.enum_names[qualified] + return None def is_enum(self, arg_type: parser.Type, class_: parser.Class): """Check if `arg_type` is an enum.""" - return self.is_class_enum(arg_type, class_) or self.is_global_enum( - arg_type, class_) + return self.resolve_enum(arg_type, class_) is not None class FormatMixin: diff --git a/gtwrap/matlab_wrapper/wrapper.py b/gtwrap/matlab_wrapper/wrapper.py index c5dd26b..301886f 100755 --- a/gtwrap/matlab_wrapper/wrapper.py +++ b/gtwrap/matlab_wrapper/wrapper.py @@ -88,6 +88,8 @@ def __init__(self, self.classes_elems: Dict[Union[parser.Class, instantiator.InstantiatedClass], int] = {} + # Qualified C++ enum names map to their C++ and MATLAB value types. + self.enum_names: Dict[str, tuple[str, str]] = {} # Id for ordering global functions in the wrapper self.global_function_id = 0 # Files and their content @@ -104,6 +106,23 @@ def add_class(self, instantiated_class): self.classes_elems[instantiated_class] = 0 self.classes.append(instantiated_class) + def _register_enums(self, namespace): + """Collect enum owners before generating references to their types.""" + for element in namespace.content: + if isinstance(element, parser.Namespace): + self._register_enums(element) + elif isinstance(element, parser.Enum): + names = namespace.full_namespaces()[1:] + [element.name] + cpp_name = '::'.join(names) + self.enum_names[cpp_name] = (cpp_name, '.'.join(names)) + elif isinstance(element, instantiator.InstantiatedClass): + names = element.namespaces()[1:] + [element.name] + for enum in element.enums: + cpp_name = f'{element.to_cpp()}::{enum.name}' + value = (cpp_name, '.'.join(names + [enum.name])) + self.enum_names[cpp_name] = value + self.enum_names['::'.join(names + [enum.name])] = value + def _update_wrapper_id(self, collector_function=None, id_diff=0, @@ -231,16 +250,22 @@ def _wrap_args(self, args): def _matlab_type_check(self, ctype, variable, - wrap_datatypes=True): + wrap_datatypes=True, + instantiated_class=None): """Return the MATLAB predicate used to dispatch one argument.""" if self.is_optional(ctype): value_check = self._matlab_type_check( - self.optional_value_type(ctype), variable, wrap_datatypes) + self.optional_value_type(ctype), variable, wrap_datatypes, + instantiated_class) if not value_check: return '' return '({empty} || {value_check})'.format( empty=f'isempty({variable})', value_check=value_check) + enum = self.resolve_enum(ctype, instantiated_class) + if enum: + return f"isa({variable},'{enum[1]}')" + name = ctype.typename.name if name in self.not_check_type: return '' @@ -275,7 +300,8 @@ def _matlab_type_check(self, return ' && '.join(checks) - def _wrap_variable_arguments(self, args, wrap_datatypes=True): + def _wrap_variable_arguments(self, args, wrap_datatypes=True, + instantiated_class=None): """ Wrap an interface_parser.ArgumentList into a statement of argument checks. @@ -289,7 +315,7 @@ def _wrap_variable_arguments(self, args, wrap_datatypes=True): for i, arg in enumerate(args.list(), 1): variable = f'varargin{{{i}}}' check = self._matlab_type_check(arg.ctype, variable, - wrap_datatypes) + wrap_datatypes, instantiated_class) if check: var_arg_wrap += f' && {check}' @@ -316,7 +342,8 @@ def _wrap_list_variable_arguments(self, args): return var_list_wrap - def _wrap_method_check_statement(self, args: parser.ArgumentList): + def _wrap_method_check_statement(self, args: parser.ArgumentList, + instantiated_class=None): """ Wrap the given arguments into either just a varargout call or a call in an if statement that checks if the parameters are accurate. @@ -338,7 +365,8 @@ def _wrap_method_check_statement(self, args: parser.ArgumentList): continue variable = f'varargin{{{arg_id}}}' - check = self._matlab_type_check(arg.ctype, variable) + check = self._matlab_type_check( + arg.ctype, variable, instantiated_class=instantiated_class) if check: check_statement += f' && {check}' @@ -372,9 +400,9 @@ def _unwrap_value_expression(self, ctype_camel = self._format_type_name(ctype.typename, separator='') ctype_sep = self._format_type_name(ctype.typename) - if instantiated_class and self.is_enum(ctype, instantiated_class): - enum_type = f'{ctype.typename}' - return f'unwrap_enum<{enum_type}>({value})' + enum = self.resolve_enum(ctype, instantiated_class) + if enum: + return f'unwrap_enum<{enum[0]}>({value})' if self.is_matrix_view(ctype): return f'unwrapMatrixView< {ctype_sep} >({value})' @@ -399,17 +427,16 @@ def _unwrap_value_expression(self, def _unwrap_argument(self, arg, arg_id=0, instantiated_class=None): ctype_camel = self._format_type_name(arg.ctype.typename, separator='') ctype_sep = self._format_type_name(arg.ctype.typename) + enum = self.resolve_enum(arg.ctype, instantiated_class) if self.is_optional(arg.ctype): arg_type = arg.ctype.get_typename() unwrap = self._unwrap_value_expression( arg.ctype, f'in[{arg_id}]', instantiated_class) + ';' - elif instantiated_class and \ - self.is_enum(arg.ctype, instantiated_class): - enum_type = f"{arg.ctype.typename}" - arg_type = f"{enum_type}" - unwrap = f'unwrap_enum<{enum_type}>(in[{arg_id}]);' + elif enum: + arg_type = enum[0] + unwrap = f'unwrap_enum<{arg_type}>(in[{arg_id}]);' elif self.is_matrix_view(arg.ctype): arg_type = self._format_type_name(arg.ctype.typename) @@ -779,7 +806,7 @@ def wrap_class_constructors(self, namespace_name, inst_class, parent_name, {ptr}{wrapper}({num}{comma}{var_arg}); ''').format(len=len(ctor.args.list()), varargin=self._wrap_variable_arguments( - ctor.args, False), + ctor.args, False, inst_class), ptr=wrapper_return, wrapper=self._wrapper_name(), num=self._update_wrapper_id( @@ -989,7 +1016,7 @@ def wrap_class_methods(self, return_type_formatted) check_statement = self._wrap_method_check_statement( - overload.args) + overload.args, inst_class) class_name = namespace_name + ('' if namespace_name == '' else '.') + inst_class.name @@ -1053,7 +1080,7 @@ def wrap_static_methods(self, namespace_name, instantiated_class, for static_overload in static_method: check_statement = self._wrap_method_check_statement( - static_overload.args) + static_overload.args, instantiated_class) return_type_formatted = self._format_return_type( static_overload.return_type, include_namespace=True, @@ -1079,7 +1106,8 @@ def wrap_static_methods(self, namespace_name, instantiated_class, return_type=return_type_formatted, length=len(static_overload.args.list()), var_args_list=self._wrap_variable_arguments( - static_overload.args), + static_overload.args, + instantiated_class=instantiated_class), check_statement=check_statement, spacing='' if check_statement == '' else ' ', varargout=varargout, @@ -1218,11 +1246,9 @@ def wrap_instantiated_class(self, # e.g. gtsam.Class.Enum.A for enum in instantiated_class.enums: enum_text = self.wrap_enum(enum) - if namespace_name != '': - submodule = f"+{namespace_name}/" - else: - submodule = "" - submodule += f"+{instantiated_class.name}" + submodule = '/'.join( + f'+{name}' for name in instantiated_class.namespaces()[1:] + + [instantiated_class.name]) self.content.append((submodule, [enum_text])) return file_name + '.m', content_text @@ -1257,6 +1283,9 @@ def wrap_namespace(self, namespace, add_mex_file=True): namespace: the interface_parser.namespace instance of the namespace add_cpp_file: Flag indicating whether the mex file should be added """ + if add_mex_file: + self._register_enums(namespace) + namespaces = namespace.full_namespaces() inner_namespace = namespace.name != '' wrapped = [] @@ -1346,16 +1375,9 @@ def _collector_wrap_expression(self, cpp_type=cpp_type, wrapped=wrapped) - if instantiated_class and self.is_enum(ctype, instantiated_class): - if self.is_class_enum(ctype, instantiated_class): - class_name = '.'.join(instantiated_class.namespaces()[1:] + - [instantiated_class.name]) - else: - class_name = '.'.join( - instantiated_class.parent.full_namespaces()[1:]) - if class_name: - class_name += '.' - return f'wrap_enum({obj},"{class_name}{ctype.typename.name}")' + enum = self.resolve_enum(ctype, instantiated_class) + if enum: + return f'wrap_enum({obj},"{enum[1]}")' ctype_cpp = self._format_type_name(ctype.typename) if self.is_fixed_size_eigen_value(ctype): @@ -1387,15 +1409,20 @@ def _collector_wrap_expression(self, return f'wrap< {ctype_cpp} >({obj})' - def wrap_collector_function_return_types(self, return_type, func_id): + def wrap_collector_function_return_types(self, return_type, func_id, + instantiated_class=None): """ Wrap the return type of the collector function when a std::pair is returned. """ return_type_text = ' out[' + str(func_id) + '] = ' pair_value = 'first' if func_id == 0 else 'second' new_line = '\n' if func_id == 0 else '' + enum = self.resolve_enum(return_type, instantiated_class) - if self.is_fixed_size_eigen_value(return_type): + if enum: + return_type_text += ( + f'wrap_enum(pairResult.{pair_value},"{enum[1]}");{new_line}') + elif self.is_fixed_size_eigen_value(return_type): return_type_text += 'wrapFixedSizeEigen(pairResult.{0});{1}'.format( pair_value, new_line) elif self.is_shared_ptr(return_type) or self.is_ptr(return_type) or \ @@ -1434,28 +1461,16 @@ def _collector_return(self, instantiated_class: InstantiatedClass = None): """Helper method to get the final statement before the return in the collector function.""" expanded = '' + enum = self.resolve_enum(ctype, instantiated_class) if self.is_optional(ctype): expanded = ' out[0] = {wrapped};'.format( wrapped=self._collector_wrap_expression( obj, ctype, instantiated_class)) - elif instantiated_class and \ - self.is_enum(ctype, instantiated_class): - if self.is_class_enum(ctype, instantiated_class): - class_name = ".".join(instantiated_class.namespaces()[1:] + - [instantiated_class.name]) - else: - # Get the full namespace - class_name = ".".join( - instantiated_class.parent.full_namespaces()[1:]) - - if class_name != "": - class_name += '.' - - enum_type = f"{class_name}{ctype.typename.name}" + elif enum: expanded = textwrap.indent( - f'out[0] = wrap_enum({obj},\"{enum_type}\");', prefix=' ') + f'out[0] = wrap_enum({obj},"{enum[1]}");', prefix=' ') elif self.is_fixed_size_eigen_value(ctype): expanded += ' out[0] = wrapFixedSizeEigen({0});'.format(obj) @@ -1549,11 +1564,11 @@ def wrap_collector_function_return(self, method, instantiated_class=None): expanded += ' const auto& pairResult = *optionalPairResult;\n' expanded += textwrap.indent( self.wrap_collector_function_return_types( - optional_pair_types[0], 0), + optional_pair_types[0], 0, instantiated_class), prefix=' ') expanded += textwrap.indent( self.wrap_collector_function_return_types( - optional_pair_types[1], 1), + optional_pair_types[1], 1, instantiated_class), prefix=' ') expanded += '\n } else {\n' expanded += ' out[0] = mxCreateDoubleMatrix(0, 0, mxREAL);\n' @@ -1573,9 +1588,9 @@ def wrap_collector_function_return(self, method, instantiated_class=None): expanded += ' auto pairResult = {};\n'.format(obj) expanded += self.wrap_collector_function_return_types( - return_1, 0) + return_1, 0, instantiated_class) expanded += self.wrap_collector_function_return_types( - return_2, 1) + return_2, 1, instantiated_class) else: expanded += obj + ';' @@ -2093,11 +2108,14 @@ def generate_content(self, cc_content, path): def wrap(self, files, path): """High level function to wrap the project.""" - content = "" + contents = [] modules = {} for file in files: with open(file, 'r') as f: - content += f.read() + contents.append(f.read()) + + # End trailing line comments before the next interface file begins. + content = '\n'.join(contents) # Parse the contents of the interface file source_name = files[0] if len(files) == 1 else ";".join(files) diff --git a/tests/expected/matlab/enum_wrapper.cpp b/tests/expected/matlab/enum_wrapper.cpp index 71e8075..66f3c35 100644 --- a/tests/expected/matlab/enum_wrapper.cpp +++ b/tests/expected/matlab/enum_wrapper.cpp @@ -205,7 +205,7 @@ void gtsamOptimizerGaussNewtonParams_constructor_13(int nargout, mxArray *out[], mexAtExit(&_deleteAllObjects); typedef std::shared_ptr> Shared; - Optimizer::Verbosity verbosity = unwrap_enum::Verbosity>(in[0]); + gtsam::Optimizer::Verbosity verbosity = unwrap_enum::Verbosity>(in[0]); Shared *self = new Shared(new gtsam::Optimizer(verbosity)); collector_gtsamOptimizerGaussNewtonParams.insert(self); out[0] = mxCreateNumericMatrix(1, 1, mxUINT32OR64_CLASS, mxREAL); @@ -243,7 +243,7 @@ void gtsamOptimizerGaussNewtonParams_setVerbosity_17(int nargout, mxArray *out[] { checkArguments("setVerbosity",nargout,nargin-1,1); auto obj = unwrap_shared_ptr>(in[0], "ptr_gtsamOptimizerGaussNewtonParams"); - Optimizer::Verbosity value = unwrap_enum::Verbosity>(in[1]); + gtsam::Optimizer::Verbosity value = unwrap_enum::Verbosity>(in[1]); obj->setVerbosity(value); } diff --git a/tests/fixtures/matlab_enum_arguments.i b/tests/fixtures/matlab_enum_arguments.i new file mode 100644 index 0000000..126ebc1 --- /dev/null +++ b/tests/fixtures/matlab_enum_arguments.i @@ -0,0 +1,29 @@ +namespace demo { +namespace nested { + +class NoLocalEnum { + static demo::nested::Owner::Mode Echo(demo::nested::Owner::Mode mode); +}; + +// The owner is declared later, and Consumer has its own enum of the same name. +class Consumer { + enum Mode { Local }; + Consumer(demo::nested::Owner::Mode mode); + static demo::nested::Owner::Mode Echo(demo::nested::Owner::Mode mode); + demo::nested::Owner::Mode get() const; + void set(const demo::nested::Owner::Mode& mode); + std::optional maybe( + std::optional mode) const; + pair pair() const; + demo::nested::Owner::Mode mode; +}; + +class Owner { + enum Mode { First, Second }; + Owner(); +}; + +demo::nested::Owner::Mode echo(demo::nested::Owner::Mode mode); + +} // namespace nested +} // namespace demo diff --git a/tests/test_matlab_wrapper.py b/tests/test_matlab_wrapper.py index d7b3330..c7ae27e 100644 --- a/tests/test_matlab_wrapper.py +++ b/tests/test_matlab_wrapper.py @@ -611,6 +611,108 @@ def test_enum(self): actual = osp.join(self.MATLAB_ACTUAL_DIR, file) self.compare_and_diff(file, actual) + def test_class_enum_namespace_paths(self): + """Class enums retain each namespace component, including typedefs.""" + for namespaces in ([], ['outer'], ['outer', 'inner'], + ['outer', 'inner', 'detail']): + with self.subTest(namespaces=namespaces), \ + tempfile.TemporaryDirectory() as directory: + root = Path(directory) + source = root / 'class_enums.i' + qualified = '::'.join(namespaces + ['Base']) + template = '::'.join(namespaces + ['Loss']) + source.write_text( + ''.join(f'namespace {name} {{\n' for name in namespaces) + + f''' +class Base {{ + enum ReweightScheme {{ Scalar, Block }}; + Base(const {qualified}::ReweightScheme scheme); + {qualified}::ReweightScheme scheme() const; +}}; +template +class Loss {{ + enum Mode {{ First, Second }}; + Loss(const This::Mode mode); + This::Mode mode() const; +}}; +typedef {template} LossDouble; +typedef {template} AnotherLossDouble; +''' + + '}\n' * len(namespaces), encoding='UTF-8') + output = root / 'output' + MatlabWrapper(module_name='class_enums').wrap( + [str(source)], path=str(output)) + + package = output.joinpath(*(f'+{ns}' for ns in namespaces)) + scheme = package / '+Base' / 'ReweightScheme.m' + self.assertTrue(scheme.is_file(), str(scheme)) + self.assertIn('Scalar(0)', scheme.read_text(encoding='UTF-8')) + self.assertTrue((package / '+LossDouble' / 'Mode.m').is_file()) + matlab = (package / 'Base.m').read_text(encoding='UTF-8') + qualified_enum = '.'.join(namespaces + ['Base', 'ReweightScheme']) + self.assertIn(f"isa(varargin{{1}},'{qualified_enum}')", matlab) + cpp = (output / 'class_enums_wrapper.cpp').read_text( + encoding='UTF-8') + self.assertIn(f'"{qualified_enum}"', cpp) + for alias in ('LossDouble', 'AnotherLossDouble'): + alias_enum = '.'.join(namespaces + [alias, 'Mode']) + self.assertIn(f'wrap_enum(obj->mode(),"{alias_enum}")', cpp) + matlab = (package / f'{alias}.m').read_text(encoding='UTF-8') + self.assertIn(f"isa(varargin{{1}},'{alias_enum}')", matlab) + + def test_interface_file_boundaries(self): + """A trailing line comment cannot consume the next interface file.""" + for ending in ('', '\n', '\r\n'): + with self.subTest(ending=repr(ending)), \ + tempfile.TemporaryDirectory() as directory: + root = Path(directory) + first = root / 'first.i' + second = root / 'second.i' + first.write_text( + 'namespace first { class First { First(); }; }' + '// namespace first' + ending, encoding='UTF-8') + second.write_text( + 'namespace second { class Second { Second(); }; }', + encoding='UTF-8') + output = root / 'output' + MatlabWrapper(module_name='boundaries').wrap( + [str(first), str(second)], path=str(output)) + + self.assertTrue((output / '+first' / 'First.m').is_file()) + self.assertTrue((output / '+second' / 'Second.m').is_file()) + cpp = (output / 'boundaries_wrapper.cpp').read_text( + encoding='UTF-8') + self.assertIn('new first::First()', cpp) + self.assertIn('new second::Second()', cpp) + + def test_enum_arguments_from_other_classes(self): + """Qualified enums use value conversions regardless of their owner.""" + source = Path(self.INTERFACE_DIR) / 'matlab_enum_arguments.i' + with tempfile.TemporaryDirectory() as directory: + output = Path(directory) + MatlabWrapper(module_name='matlab_enum_arguments').wrap( + [str(source)], path=directory) + cpp = (output / 'matlab_enum_arguments_wrapper.cpp').read_text( + encoding='UTF-8') + enum = 'demo::nested::Owner::Mode' + matlab_enum = 'demo.nested.Owner.Mode' + self.assertIn(f'{enum} mode = unwrap_enum<{enum}>(in[0]);', cpp) + self.assertIn(f'{enum} mode = unwrap_enum<{enum}>(in[1]);', cpp) + self.assertIn('new demo::nested::Consumer(mode)', cpp) + self.assertIn('demo::nested::Consumer::Echo(mode)', cpp) + self.assertIn('demo::nested::NoLocalEnum::Echo(mode)', cpp) + self.assertIn(f'wrap_enum(obj->get(),"{matlab_enum}")', cpp) + self.assertIn(f'wrap_enum(obj->mode,"{matlab_enum}")', cpp) + self.assertIn('obj->mode = mode;', cpp) + self.assertIn(f'unwrap_enum<{enum}>(value)', cpp) + self.assertIn(f'wrap_enum(value,"{matlab_enum}")', cpp) + self.assertIn(f'wrap_enum(demo::nested::echo(mode),"{matlab_enum}")', cpp) + self.assertIn(f'wrap_enum(pairResult.first,"{matlab_enum}")', cpp) + self.assertIn(f'wrap_enum(pairResult.second,"{matlab_enum}")', cpp) + self.assertNotIn(f'unwrap_shared_ptr< {enum} >', cpp) + self.assertNotIn(f'make_shared<{enum}>', cpp) + self.assertTrue((output / '+demo' / '+nested' / '+Owner' / 'Mode.m').is_file()) + def test_templates(self): """Test interface file with template info.""" file = osp.join(self.INTERFACE_DIR, 'templates.i')