wlav / cppyy

Other
412 stars 42 forks source link

🐞cppyy appears to be dropping a nested template argument #262

Closed lukevalenty closed 1 month ago

lukevalenty commented 1 month ago

Not quite sure what's going on here, but cppyy appears to be dropping a nested template argument. I minimized the example as much as I could to provide a test case. The interesting part is that the failing test passes if the template function is previously instantiated within C++. The tests below show the C++ code, the failing case in python and two similar passing cases in python.

import cppyy

cppyy.cppdef("""
    namespace test {
    template<typename... Ts>
    struct list {
        template<typename... Us>
        constexpr auto operator==(list<Us...>) const -> bool {
            return false;
        }

        constexpr auto operator==(list<Ts...>) const -> bool {
            return true;
        }
    };

    template<typename... Ts>
    constexpr auto make_list(Ts...) {
        return list<Ts...>{};
    }

    template<typename... Ls, typename... Rs>
    constexpr auto cat(list<Ls...>, list<Rs...>) {
        return list<Ls..., Rs...>{};
    }
    } // namespace test

    // constexpr auto expected = test::make_list(
    //     test::make_list(0), 
    //     test::make_list(), test::make_list());

    // NOTE: If you uncomment just the following declaration then
    //       the 'test_cat_hier_list_fail' pytest passes!
    // constexpr auto actual = test::cat(
    //     test::make_list(test::make_list(0)),
    //     test::make_list(test::make_list(), test::make_list()));

    // static_assert(actual == expected);
""")

test = cppyy.gbl.test

def test_cat_hier_list_fail():
    expected = test.make_list(
        test.make_list(0), 
        test.make_list(), test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0)), 
        test.make_list(test.make_list(), test.make_list()))

    assert actual == expected

def test_cat_hier_list_pass_0():
    expected = test.make_list(
        test.make_list(0), 
        test.make_list(1), test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0)), 
        test.make_list(test.make_list(1), test.make_list()))

    assert actual == expected

def test_cat_hier_list_pass_1():
    expected = test.make_list(
        test.make_list(0), 
        test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0)), 
        test.make_list(test.make_list()))

    assert actual == expected
lukevalenty commented 1 month ago

I just tested this in a fresh venv and cppyy installed from pip. Still fails the same way, let me know what you find.

wlav commented 1 month ago

This one fails for me in both the existing release and with HEAD. I haven't found the exact location yet, but the signature provided by Cling is correct (in fact, the returned object type isn't among the known signatures), so this must be on the cppyy end.

wlav commented 1 month ago

Not in cppyy. The problem comes in from name normalization, deep in meta. It's one of the uglier parts: it uses Clang printing to get a human-representable name, but that's very buggy.

lukevalenty commented 1 month ago

deep in meta

Is that in CPyCppyy?

wlav commented 1 month ago

No, it's a piece of legacy software that upstream is actively removing in their libcppinterop project. The basic idea is to canonicalize names while respecting the I/O layer (the original reason for that software to exist). Using strings was never a good solution, but was easy for backwards compatibility with CINT. Code is here: https://github.com/wlav/cppyy-backend/blob/master/cling/src/core/clingutils/src/TClingUtils.cxx#L3670 And yes, it's not pretty. I didn't write that, though. :)

I suspect it's either a problem of the name splitting (having <> in the template name, i.e. w/o arguments, isn't common and I don't think I have a test for it) or in Clang's printing facilities (not unheard of, b/c for Clang, it exists mainly for debugging purposes and isn't heavily tested). I hope it's the former.

Note that libcppinterop is still struggling with it b/c using decls isn't as straightforward as it sounds either. E.g. certain types in C++ are meant to be used differently than their actual type (e.g. int8_t which is supposed to be an integer type, but is a typedef to char) and thus if used as template parameters, fun ensues. There are some more corner cases. E.g. if the type used is a public typedef pointing to a private class then it's not useful to work with the canonical type, but such cases are easier to handle.

wlav commented 1 month ago

The specific problem appears to be here: https://github.com/wlav/cppyy-backend/blob/master/cling/src/core/clingutils/src/TClingUtils.cxx#L2607

This code is recursively normalizing the template arguments. The more I look at the iteration logic, the less I understand how it's supposed to work for parameters packs, but in the failing case, it is one short. Will look at it fresh tomorrow and ask upstream.

lukevalenty commented 1 month ago

I asked OpenAI's o1 model to debug the issue and it actually worked provides some interesting results...just tested it out. It provides rewritten function that fixes the handling at the end of this chat:

https://chatgpt.com/share/66fc2933-2c58-8008-8f9d-4e7dff463cd0

The test cases along with a couple more pass:

import cppyy

cppyy.cppdef("""
    namespace test {
    template<typename... Ts>
    struct list {
        template<typename... Us>
        constexpr auto operator==(list<Us...>) const -> bool {
            return false;
        }

        constexpr auto operator==(list<Ts...>) const -> bool {
            return true;
        }
    };

    template<typename... Ts>
    constexpr auto make_list(Ts...) {
        return list<Ts...>{};
    }

    template<typename... Ls, typename... Rs>
    constexpr auto cat(list<Ls...>, list<Rs...>) {
        return list<Ls..., Rs...>{};
    }
    } // namespace test

    // constexpr auto expected = test::make_list(
    //     test::make_list(0), 
    //     test::make_list(), test::make_list());

    // NOTE: If you uncomment just the following declaration then
    //       the 'test_cat_hier_list_fail' pytest passes!
    // constexpr auto actual = test::cat(
    //     test::make_list(test::make_list(0)),
    //     test::make_list(test::make_list(), test::make_list()));

    // static_assert(actual == expected);
""")

test = cppyy.gbl.test

def test_cat_hier_list_fail_0():
    expected = test.make_list(
        test.make_list(0), 
        test.make_list(), test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0)), 
        test.make_list(test.make_list(), test.make_list()))

    assert actual == expected

def test_cat_hier_list_fail_1():
    expected = test.make_list(
        test.make_list(0), 
        test.make_list(), test.make_list(), test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0)), 
        test.make_list(test.make_list(), test.make_list(), test.make_list()))

    assert actual == expected

def test_cat_hier_list_fail_2():
    expected = test.make_list(
        test.make_list(0), test.make_list(0),
        test.make_list(), test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0), test.make_list(0)), 
        test.make_list(test.make_list(), test.make_list()))

    assert actual == expected

def test_cat_hier_list_fail_3():
    expected = test.make_list(
        test.make_list(0), test.make_list(0),
        test.make_list(), test.make_list(), test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0), test.make_list(0)), 
        test.make_list(test.make_list(), test.make_list(), test.make_list()))

    assert actual == expected

def test_cat_hier_list_pass_0():
    expected = test.make_list(
        test.make_list(0), 
        test.make_list(1), test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0)), 
        test.make_list(test.make_list(1), test.make_list()))

    assert actual == expected

def test_cat_hier_list_pass_1():
    expected = test.make_list(
        test.make_list(0), 
        test.make_list())

    actual = test.cat(
        test.make_list(test.make_list(0)), 
        test.make_list(test.make_list()))

    assert actual == expected

def test_cat_hier_list_pass_2():
    expected = test.make_list(
        test.make_list(0), 
        test.make_list(0), test.make_list(0))

    actual = test.cat(
        test.make_list(test.make_list(0)), 
        test.make_list(test.make_list(0), test.make_list(0)))

    assert actual == expected

Here's the output:

(venv_tvargs_dev) lvalenty@Zenbook:~/cppyy_tvargs_dev$ python -m pytest -sv test_cat_hier_list.py
================================================================================ test session starts ================================================================================
platform linux -- Python 3.10.12, pytest-8.3.3, pluggy-1.5.0 -- /home/lvalenty/cppyy_tvargs_dev/venv_tvargs_dev/bin/python
cachedir: .pytest_cache
rootdir: /home/lvalenty/cppyy_tvargs_dev
collecting ... error: Problems loading PCH: '/home/lvalenty/cppyy_tvargs_dev/venv_tvargs_dev/lib/python3.10/site-packages/cppyy/allDict.cxx.pch.6.30.0'.
collected 7 items

test_cat_hier_list.py::test_cat_hier_list_fail_0 Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter pack 'Ts' in 'list<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter pack 'Ts' in 'list<>'
PASSED
test_cat_hier_list.py::test_cat_hier_list_fail_1 Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter pack 'Ts' in 'list<>'
PASSED
test_cat_hier_list.py::test_cat_hier_list_fail_2 PASSED
test_cat_hier_list.py::test_cat_hier_list_fail_3 PASSED
test_cat_hier_list.py::test_cat_hier_list_pass_0 PASSED
test_cat_hier_list.py::test_cat_hier_list_pass_1 Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter pack 'Ts' in 'list<>'
PASSED
test_cat_hier_list.py::test_cat_hier_list_pass_2 PASSED

================================================================================= 7 passed in 0.96s =================================================================================
(venv_tvargs_dev) lvalenty@Zenbook:~/cppyy_tvargs_dev$

ETA: It does break the rest of the tests though in cppyy! 😂

(venv_tvargs_dev) lvalenty@Zenbook:~/cppyy_tvargs_dev/cppyy/test$ make -j8
genreflex advancedcpp.h --selection=advancedcpp.xml --rootmap=advancedcppDict.rootmap --rootmap-lib=advancedcppDict.so
genreflex advancedcpp2.h --selection=advancedcpp2.xml --rootmap=advancedcpp2Dict.rootmap --rootmap-lib=advancedcpp2Dict.so
genreflex conversions.h --selection=conversions.xml --rootmap=conversionsDict.rootmap --rootmap-lib=conversionsDict.so
genreflex cpp11features.h --selection=cpp11features.xml --rootmap=cpp11featuresDict.rootmap --rootmap-lib=cpp11featuresDict.so
genreflex crossinheritance.h --selection=crossinheritance.xml --rootmap=crossinheritanceDict.rootmap --rootmap-lib=crossinheritanceDict.so
genreflex datatypes.h --selection=datatypes.xml --rootmap=datatypesDict.rootmap --rootmap-lib=datatypesDict.so
genreflex doc_helper.h --selection=doc_helper.xml --rootmap=doc_helperDict.rootmap --rootmap-lib=doc_helperDict.so
genreflex example01.h --selection=example01.xml --rootmap=example01Dict.rootmap --rootmap-lib=example01Dict.so
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
make: *** [Makefile:31: doc_helper_rflx.cpp] Error 1
make: *** Waiting for unfinished jobs....
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
make: *** [Makefile:31: conversions_rflx.cpp] Error 1
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
make: *** [Makefile:31: cpp11features_rflx.cpp] Error 1
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
make: *** [Makefile:31: crossinheritance_rflx.cpp] Error 1
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
make: *** [Makefile:31: example01_rflx.cpp] Error 1
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__shared_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp1' in '__weak_ptr<class CppyyLegacy::TObjLink>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
Error in <CppyyLegacy::TMetaUtils::AddDefaultParameters>: Missing template argument for parameter '_Lp' in '__weak_count<>'
make: *** [Makefile:31: advancedcpp2_rflx.cpp] Error 1
make: *** [Makefile:31: advancedcpp_rflx.cpp] Error 1
make: *** [Makefile:31: datatypes_rflx.cpp] Error 1
wlav commented 1 month ago

ChatGPT dropped the protection of I != E to break out of the loop, making it now fail for template specializations which need not match the declared parameters. Likewise, this is dubious: Idecl != Edecl && Param != Params->end(). Yes, that makes it work b/c the Idecl != Edecl is what stops the iterations early and this new predicate is basically ignoring it, but now the original purpoe of Idecl is gone. Adjusting Edecl to the actual number of parameters in the pack and keeping the original iteration is probably better.

But yes, I'm impressed. :) We've tried ChatGPT on data analysis for our quantum control system, which is super simple, but it was worse than a coin toss (as was Gemini). This is another level. (I do think it misses the purpose as it doesn't understand the effect of maxAddArg, which admittedly makes a mockery of the name of the function.)

Aside, upstream says:

"""I have a proposal deadline at the 10th. Maybe worth looking in CppInterOp because I remember some discussions about this. Maybe there is fixed and you can get the code. Will try to look as soon as I can."""

I'll have a look there, but am otherwise partial to adjusting Edecl as outlined above.

lukevalenty commented 1 month ago

I took a look at cppinterop....I think they are using Sema::DeduceTemplateArguments from 'clang/Sema/Sema.h'.

ETA: Looks like they are only using this for template function instantiation.

CppInterOp.cpp:2945

static Decl* InstantiateTemplate(TemplateDecl* TemplateD,
                                   TemplateArgumentListInfo& TLI, Sema& S) {
    // This is not right but we don't have a lot of options to choose from as a
    // template instantiation requires a valid source location.
    SourceLocation fakeLoc = GetValidSLoc(S);
    if (auto* FunctionTemplate = dyn_cast<FunctionTemplateDecl>(TemplateD)) {
      FunctionDecl* Specialization = nullptr;
      clang::sema::TemplateDeductionInfo Info(fakeLoc);

      Template_Deduction_Result Result = S.DeduceTemplateArguments(
          FunctionTemplate, &TLI, Specialization, Info,
          /*IsAddressOfFunction*/ true);

      if (Result != Template_Deduction_Result_Success) {
        // FIXME: Diagnose what happened.
        (void)Result;
      }
      return Specialization;
    }

    ...
}

clang/include/clang/Sema/Sema.h:12327

  /// Perform template argument deduction from a function call
  /// (C++ [temp.deduct.call]).
  ///
  /// \param FunctionTemplate the function template for which we are performing
  /// template argument deduction.
  ///
  /// \param ExplicitTemplateArgs the explicit template arguments provided
  /// for this call.
  ///
  /// \param Args the function call arguments
  ///
  /// \param Specialization if template argument deduction was successful,
  /// this will be set to the function template specialization produced by
  /// template argument deduction.
  ///
  /// \param Info the argument will be updated to provide additional information
  /// about template argument deduction.
  ///
  /// \param CheckNonDependent A callback to invoke to check conversions for
  /// non-dependent parameters, between deduction and substitution, per DR1391.
  /// If this returns true, substitution will be skipped and we return
  /// TemplateDeductionResult::NonDependentConversionFailure. The callback is
  /// passed the parameter types (after substituting explicit template
  /// arguments).
  ///
  /// \returns the result of template argument deduction.
  TemplateDeductionResult DeduceTemplateArguments(
      FunctionTemplateDecl *FunctionTemplate,
      TemplateArgumentListInfo *ExplicitTemplateArgs, ArrayRef<Expr *> Args,
      FunctionDecl *&Specialization, sema::TemplateDeductionInfo &Info,
      bool PartialOverloading, bool AggregateDeductionCandidate,
      QualType ObjectType, Expr::Classification ObjectClassification,
      llvm::function_ref<bool(ArrayRef<QualType>)> CheckNonDependent);
wlav commented 1 month ago

I think that deduction serves different purposes and anyway, wouldn't be able to leave the default parameters out of STL classes (as most people seem to prefer to deal with).

I've push some changes that cover this case and don't break anything, but it's not complete: I found some corner cases that aren't handled properly, but in that case I decided to drop back to the original type rather than producing a broken/different type. Either way, it fixes this specific problem and doesn't break any existing test.

lukevalenty commented 1 month ago

Great! I'll give it a try.

lukevalenty commented 1 month ago

This does the trick!! 😎

I ran it through the ringer with this random test suite at 500 different examples and it passed with flying colors.

import hypothesis
from hypothesis import given, settings, example, note, event, strategies as st
import cppyy
import pytest

cppyy.include("limits")

cppyy.cppdef("""
    namespace test {
    template <typename T>
    constexpr std::size_t tuple_size(const T&) {
        return std::tuple_size<T>::value;
    }
    constexpr bool as_bool(bool b) {
        return static_cast<bool>(b);
    }
    template<typename... Ts>
    struct list {
        template<typename... Us>
        constexpr auto operator==(list<Us...>) const -> bool {
            return false;
        }
        constexpr auto operator==(list<Ts...>) const -> bool {
            return true;
        }
    };
    template<typename... Ts>
    constexpr auto make_list(Ts...) {
        return list<Ts...>{};
    }
    template<typename... Ls, typename... Rs>
    constexpr auto cat(list<Ls...>, list<Rs...>) {
        return list<Ls..., Rs...>{};
    }
    } // namespace test
""")

std = cppyy.gbl.std
test = cppyy.gbl.test

bools = st.booleans().map(lambda v: test.as_bool(v))

def make_c_type_strat(t):
    min_val = std.numeric_limits[t].min()
    max_val = std.numeric_limits[t].max()
    return st.integers(min_value=int(min_val), max_value=int(max_val)).map(lambda v: t(v))

int32s = make_c_type_strat(std.int32_t)

cpp_primitives = st.one_of(bools, int32s)

def as_test_list_tree(value):
    if isinstance(value, list):
        values = [as_test_list_tree(v) for v in value]
        return test.make_list(*values)
    else:
        return value

@st.composite
def py_list_trees(draw, leaves=cpp_primitives):
    t = draw(st.recursive(leaves, lambda children: st.lists(children)))

    if not isinstance(t, list):
        t = [t]

    return t

@settings(deadline=10000, max_examples=500)
@given(py_list_trees(), py_list_trees())
@example([0], [[], []]) # <- the original bug, this ensures it will be included
def test_list_cat(pytestconfig, left, right):
    expected = as_test_list_tree(left + right)
    event(str(expected))
    actual = test.cat(as_test_list_tree(left), as_test_list_tree(right))
    assert actual == expected