swig / swig

SWIG is a software development tool that connects programs written in C and C++ with a variety of high-level programming languages.
http://www.swig.org
Other
5.8k stars 1.24k forks source link

default argument handling v4.3.0-beta1 regression #3052

Closed jschueller closed 1 month ago

jschueller commented 1 month ago

since 9cba248beccc1ec38d58fea03cfd5639eef6fa1f swig emits wrong code for this simple function (in c++ to Python mode):

void draw3(int index = 0, const bool interpolate = true);

calling the variant without any argument result in an error at runtime, here for Python (did not test other languages):

mesh.draw3()

TypeError: Wrong number or type of arguments for overloaded function 'Mesh_draw3'.
  Possible C/C++ prototypes are:
    OT::Mesh::draw3(int,bool const)
    OT::Mesh::draw3(int)
    OT::Mesh::draw3()

weirdly, if I remove the const or make the second argument an int the error goes away

Looks like it could affect other languages as well:

9cba248beccc1ec38d58fea03cfd5639eef6fa1f is the first bad commit
commit 9cba248beccc1ec38d58fea03cfd5639eef6fa1f
Author: Olly Betts <olly@survex.com>
Date:   Mon Sep 16 16:10:02 2024 +1200

    Straighten out handling of integer constants

    This provides a generic framework to aid converting C/C++ integer and
    boolean literals to target language literals, replacing custom code in
    several target language backends (and fixing some bugs in that code).

 Examples/test-suite/d/enum_thorough_runme.2.d |   8 ++
 Examples/test-suite/enum_thorough.i           |  10 +++
 Source/CParse/cscanner.c                      |  37 ++++++++-
 Source/CParse/parser.y                        | 113 ++++++++++++++++++++++++--
 Source/Modules/csharp.cxx                     |   8 +-
 Source/Modules/d.cxx                          |   7 +-
 Source/Modules/go.cxx                         |  46 +++--------
 Source/Modules/java.cxx                       |   8 +-
 Source/Modules/octave.cxx                     |  32 ++++----
 Source/Modules/python.cxx                     |  94 ++++++++-------------
 Source/Modules/r.cxx                          |   8 +-
 Source/Modules/ruby.cxx                       |  38 +++++----
 Source/Swig/misc.c                            |   2 +-
 13 files changed, 258 insertions(+), 153 deletions(-)
bisect found first bad commit

This non-regression test could be added in Examples/test-suite/default_arg_values.i

/cc @ojwb

ojwb commented 1 month ago

Thanks for spotting this before the release.

The value is now stored in the parse tree in a canonical form (so false -> 0, true -> 1) and is then converted to True/False based on the argument's type (which should be better as it means we consistently handle cases where the value type isn't the same as the argument's type, such as void f(bool x = 1, int y = false)).

However we need to strip qualifiers before checking if the type is bool when deciding whether to convert back.

I have a fix working but need to go out for a bit - will clean up and push later today.

ojwb commented 1 month ago

Looks like it affects Ruby too, which has similar code to Python here. The other languages which use numval effectively check it with SwigType_type(t) == T_BOOL which strips qualifiers so works already.

jschueller commented 1 month ago

thanks