diff --git a/examples/Makefile b/examples/Makefile index 1f116c8b..f8be5585 100644 --- a/examples/Makefile +++ b/examples/Makefile @@ -41,6 +41,7 @@ EXAMPLES = \ issue32 \ issue353_selected_kind \ issue357_name_conflict \ + issue369_optional_present \ issue41_abstract_classes \ keep_single_interface \ keyword_renaming_issue160 \ diff --git a/examples/issue369_optional_present/Makefile b/examples/issue369_optional_present/Makefile new file mode 100644 index 00000000..88ce63ea --- /dev/null +++ b/examples/issue369_optional_present/Makefile @@ -0,0 +1,33 @@ +#======================================================================= +# define the compiler names +#======================================================================= + +include ../make.inc +PY_MOD = pywrapper +F90_SRC = main.f90 +OBJ = $(F90_SRC:.f90=.o) +F90WRAP_SRC = $(addprefix f90wrap_,${F90_SRC}) +WRAPFLAGS = -v +F2PY = f2py-f90wrap +SRC_AR = libsrc.a +.PHONY: all clean + +all: test + +clean: + rm -rf *.mod *.smod *.o ${SRC_AR} f90wrap*.f90 ${PY_MOD}.py _${PY_MOD}*.so __pycache__/ .f2py_f2cmap build ${PY_MOD}/ + +%.o: %.f90 + ${F90} ${F90FLAGS} -c $< -o $@ + +${F90WRAP_SRC}: ${OBJ} + ${F90WRAP} -m ${PY_MOD} ${WRAPFLAGS} ${F90_SRC} + +${SRC_AR}: ${OBJ} + ar rcs $@ ${OBJ} + +f2py: ${F90WRAP_SRC} ${SRC_AR} + CFLAGS="${CFLAGS}" ${F2PY} -c -m _${PY_MOD} ${F2PYFLAGS} f90wrap_*.f90 -L. -lsrc + +test: f2py + ${PYTHON} tests.py diff --git a/examples/issue369_optional_present/Makefile.meson b/examples/issue369_optional_present/Makefile.meson new file mode 100644 index 00000000..b2ee9928 --- /dev/null +++ b/examples/issue369_optional_present/Makefile.meson @@ -0,0 +1,6 @@ +include ../make.meson.inc + +NAME := pywrapper + +test: build + $(PYTHON) tests.py diff --git a/examples/issue369_optional_present/main.f90 b/examples/issue369_optional_present/main.f90 new file mode 100644 index 00000000..5588a794 --- /dev/null +++ b/examples/issue369_optional_present/main.f90 @@ -0,0 +1,47 @@ +! Regression example for issue #369. +! +! With --direct-c, passing None for an OPTIONAL argument must make present(arg) +! return .false. inside Fortran. The bug passed a non-NULL pointer (scalar) or an +! allocated blank buffer (character), so present() wrongly returned .true. +! +! Each subroutine reports its decision through an intent(out) result so the Python +! test can assert which branch ran. Both the regular f90wrap+f2py path and the +! --direct-c path are exercised by the shared tests.py. +module m_optional + implicit none +contains + + ! Optional scalar: the exact case from issue #369. + subroutine scalar_default(x, n) + real, intent(out) :: x + integer, intent(in), optional :: n + if (.not. present(n)) then + x = 42.0 + else + x = real(n) + end if + end subroutine scalar_default + + ! Optional character input. + subroutine char_default(label, name) + character(len=16), intent(out) :: label + character(len=*), intent(in), optional :: name + if (present(name)) then + label = name + else + label = "ABSENT" + end if + end subroutine char_default + + ! Optional array input (already correct before the fix: a regression lock). + subroutine array_default(s, v) + integer, intent(out) :: s + real, intent(in), optional :: v(:) + if (present(v)) then + s = size(v) + else + s = -1 + end if + end subroutine array_default + +end module m_optional diff --git a/examples/issue369_optional_present/tests.py b/examples/issue369_optional_present/tests.py new file mode 100644 index 00000000..b5de406d --- /dev/null +++ b/examples/issue369_optional_present/tests.py @@ -0,0 +1,39 @@ +import unittest + +import numpy as np + +from pywrapper import m_optional + + +def as_text(value): + """Character returns come back as bytes (direct-c) or str (f2py).""" + if isinstance(value, bytes): + value = value.decode() + return value.strip() + + +class TestOptionalPresent(unittest.TestCase): + def test_scalar_present(self): + self.assertEqual(m_optional.scalar_default(n=5), 5.0) + + def test_scalar_absent_uses_default(self): + # Bug #369: direct-c saw present(n) == .true. with value 0. + self.assertEqual(m_optional.scalar_default(), 42.0) + + def test_char_present(self): + self.assertEqual(as_text(m_optional.char_default(name="hello")), "hello") + + def test_char_absent_uses_default(self): + self.assertEqual(as_text(m_optional.char_default()), "ABSENT") + + def test_array_present(self): + self.assertEqual( + m_optional.array_default(v=np.array([1.0, 2.0, 3.0], dtype=np.float32)), 3 + ) + + def test_array_absent(self): + self.assertEqual(m_optional.array_default(), -1) + + +if __name__ == "__main__": + unittest.main() diff --git a/f90wrap/directc_cgen/arguments_scalar.py b/f90wrap/directc_cgen/arguments_scalar.py index 31d3d062..0e8341df 100644 --- a/f90wrap/directc_cgen/arguments_scalar.py +++ b/f90wrap/directc_cgen/arguments_scalar.py @@ -102,7 +102,9 @@ def prepare_scalar_argument(gen: 'DirectCGenerator', arg: ft.Argument, intent: s if optional: gen.write(f"if (py_{arg.name} == Py_None) {{") gen.indent() - gen.write(f"{arg.name}_val = 0;") + # Pass a NULL pointer so the Fortran side sees present(arg) == .false. + # instead of an argument whose value happens to be zero. + gen.write(f"{arg.name} = NULL;") gen.dedent() gen.write("} else {") gen.indent() @@ -121,7 +123,11 @@ def _prepare_character_none_case( """Handle None value for character arguments.""" gen.write(f"if (py_{arg.name} == Py_None) {{") gen.indent() - if optional or intent != "in": + if optional: + # Absent optional argument: pass a NULL pointer so present(arg) == .false. + # The output path returns None for a NULL character buffer. + gen.write(f"{arg.name} = NULL;") + elif intent != "in": gen.write(f"{arg.name}_len = {default_len};") gen.write(f"if ({arg.name}_len <= 0) {{") gen.indent() diff --git a/f90wrap/directc_cgen/procedures_return.py b/f90wrap/directc_cgen/procedures_return.py index b7feb475..6f9180f0 100644 --- a/f90wrap/directc_cgen/procedures_return.py +++ b/f90wrap/directc_cgen/procedures_return.py @@ -152,7 +152,13 @@ def _prepare_character_output(gen: DirectCGenerator, arg: ft.Argument) -> None: if parsed: # Check if buffer is from numpy array gen.write(f"PyObject* py_{arg.name}_obj = NULL;") - gen.write(f"if ({arg.name}_is_array) {{") + # An absent optional argument has a NULL buffer: return None for it. + gen.write(f"if ({arg.name} == NULL) {{") + gen.indent() + gen.write("Py_INCREF(Py_None);") + gen.write(f"py_{arg.name}_obj = Py_None;") + gen.dedent() + gen.write(f"}} else if ({arg.name}_is_array) {{") gen.indent() gen.write("/* Numpy array was modified in place, no return object or free needed */") gen.dedent() diff --git a/test/test_directc.py b/test/test_directc.py index 508b1313..63d5b5c0 100644 --- a/test/test_directc.py +++ b/test/test_directc.py @@ -324,6 +324,39 @@ def test_c_code_syntax_basics(self): self.assertNotIn('def ', c_code) self.assertNotIn('import ', c_code) + def _generate_with_argument(self, name, type_spec, attributes): + arg = Mock(spec=ft.Argument) + arg.name = name + arg.type = type_spec + arg.attributes = attributes + self.generator.root.modules[0].procedures[0].arguments = [arg] + return self.generator.generate_module('testmod') + + def test_optional_scalar_passes_null(self): + """Issue #369: an absent optional scalar passes NULL, not a zeroed value.""" + c_code = self._generate_with_argument('n', 'integer', ['intent(in)', 'optional']) + + self.assertIn('if (py_n == Py_None)', c_code) + self.assertIn('n = NULL;', c_code) + + def test_optional_character_input_passes_null(self): + """An absent optional character input passes NULL so present() is .false.""" + c_code = self._generate_with_argument( + 'name', 'character(len=8)', ['intent(in)', 'optional'] + ) + + self.assertIn('name = NULL;', c_code) + + def test_optional_character_output_passes_null_and_returns_none(self): + """An absent optional character output passes NULL and is returned as None.""" + c_code = self._generate_with_argument( + 'label', 'character(len=8)', ['intent(out)', 'optional'] + ) + + self.assertIn('label = NULL;', c_code) + self.assertIn('if (label == NULL)', c_code) + self.assertIn('py_label_obj = Py_None;', c_code) + def test_c_code_has_character_setter(self): """Character module variables should generate setter wrappers.""" element = Mock(spec=ft.Element)