Do not adopt an already known method into a template overload

src/cpyrt/CPPMethod.h

@@ -74,6 +74,8 @@ class CPPMethod : public PyCallable {
   PyObject* Call(CPPInstance*& self, cpyrt_PyArgs_t args, size_t nargsf,
                  PyObject* kwds, CallContext* ctxt = nullptr) override;
 
+  interop::TCppMethod_t GetMethod() const { return fMethod; }
+
 protected:
   virtual bool ProcessArgs(PyCallArgs& args);
 
@@ -83,7 +85,6 @@ class CPPMethod : public PyCallable {
                          CallContext* ctxt = nullptr);
   PyObject* Execute(void* self, ptrdiff_t offset, CallContext* ctxt = nullptr);
 
-  interop::TCppMethod_t GetMethod() { return fMethod; }
   interop::TCppScope_t GetScope() { return fScope; }
   Executor* GetExecutor() { return fExecutor; }
   std::string GetSignatureString(bool show_formalargs = true);

src/cpyrt/TemplateProxy.cxx

@@ -283,9 +283,23 @@ PyObject* TemplateProxy::Instantiate(const std::string& fname,
     // Case 3/4: pre-existing method that was either not found b/c the full
     // templated name was constructed in this call or it failed as overload
     else if (bIsCppOL) {
-      // TODO: see above, since the call hasn't happened yet, this overload may
-      // already exist and fail again.
-      ((CPPOverload*)pyol)->AdoptMethod(meth); // takes ownership
+      // The lookup is cached before the call, so when the call then fails
+      // (e.g. an implicit conversion attempt that is not allowed to nest), the
+      // next call instantiates the same method again. Adopting it again would
+      // grow the overload set with duplicates and force the whole set to be
+      // ranked anew on every call.
+      bool known = false;
+      for (PyCallable* pc : ((CPPOverload*)pyol)->fMethodInfo->fMethods) {
+        auto* existing = dynamic_cast<CPPMethod*>(pc);
+        if (existing && existing->GetMethod() == cppmeth) {
+          known = true;
+          break;
+        }
+      }
+      if (known)
+        delete meth;
+      else
+        ((CPPOverload*)pyol)->AdoptMethod(meth); // takes ownership
     }
 
     // Case 5: must be a template proxy, meaning that current template name is

src/interop/interop_wrapper.cxx

@@ -524,6 +524,36 @@ bool interop::AppendTypesSlow(const std::string& name,
 
   std::lock_guard<std::recursive_mutex> Lock(InterOpMutex);
 
+  // Every lookup below declares code through the interpreter, which parses,
+  // commits a transaction and leaves a new declaration in the AST, so each one
+  // costs more than the last. The same names come back constantly (overload
+  // ranking resolves every argument type of every candidate on each call that
+  // is not memoized), so remember what a name resolved to. A name that fails
+  // may still resolve once more declarations are available, so only successes
+  // are cached.
+  using SlowTypesKey = std::pair<std::string, const void*>;
+  static std::map<SlowTypesKey, std::vector<Cpp::TemplateArgInfo>> s_cache;
+  const SlowTypesKey cache_key{name, parent.data};
+  // the integral values are strings whose ownership passes to the caller
+  // (which frees them), so the cache keeps and hands out copies of its own
+  auto copy_of = [](const Cpp::TemplateArgInfo& info) {
+    return Cpp::TemplateArgInfo(info.m_Type, info.m_IntegralValue
+                                                 ? strdup(info.m_IntegralValue)
+                                                 : nullptr);
+  };
+  if (auto cached = s_cache.find(cache_key); cached != s_cache.end()) {
+    for (const Cpp::TemplateArgInfo& info : cached->second)
+      types.push_back(copy_of(info));
+    return false;
+  }
+  const size_t n_types_before = types.size();
+  auto remember = [&] {
+    std::vector<Cpp::TemplateArgInfo> resolved;
+    for (size_t i = n_types_before; i < types.size(); ++i)
+      resolved.push_back(copy_of(types[i]));
+    s_cache.emplace(cache_key, std::move(resolved));
+  };
+
   // We might have an entire expression such as int, double.
   static unsigned long long struct_count = 0;
   std::string code =
@@ -554,7 +584,10 @@ bool interop::AppendTypesSlow(const std::string& name,
       TCppScope_t instance_class = Cpp::GetScopeFromType(varN);
       size_t oldSize = types.size();
       Cpp::GetClassTemplateInstantiationArgs(instance_class, types);
-      return oldSize == types.size();
+      if (oldSize == types.size())
+        return true;
+      remember();
+      return false;
     }
   }
 
@@ -593,6 +626,7 @@ bool interop::AppendTypesSlow(const std::string& name,
             strdup(std::to_string(Cpp::GetEnumConstantValue(scope)).c_str());
     types.emplace_back(type.data, integral_value);
   }
+  remember();
   return false;
 }
 

test/test_regression.py

@@ -1727,3 +1727,51 @@ def test56_enum_arg_typedefed_underlying_type(self):
 
         assert ns.pick(ns.S.A) == 1
         assert ns.pick(ns.S.B) == 5
+
+    def test57_repeated_enum_constant_template_argument(self):
+        """A template argument spelled as an enum constant resolves repeatedly"""
+
+        import cppjit
+
+        cppjit.cppdef("""\
+        namespace RepeatedEnumArg {
+            enum E { A = 1, B = 2 };
+            template <E e> struct T1 { static constexpr int v = (int)e; };
+            template <E e> struct T2 { static constexpr int v = (int)e; };
+        }""")
+
+        ns = cppjit.gbl.RepeatedEnumArg
+
+        # the resolved argument is memoized by name; its integral value is a
+        # string the first instantiation frees, so the second needs its own
+        assert ns.T1["RepeatedEnumArg::E::A"].v == 1
+        assert ns.T2["RepeatedEnumArg::E::A"].v == 1
+        assert ns.T2["RepeatedEnumArg::E::B"].v == 2
+
+    def test58_repeated_implicit_tuple_conversion(self):
+        """Repeated implicit tuple-to-pair conversion does not grow the overload set"""
+
+        import cppjit
+
+        cppjit.cppdef("""\
+        #include <utility>
+        namespace RepeatedImplicitTuple {
+            int first(std::pair<int, int>&& p) { return p.first; }
+        }""")
+
+        ns = cppjit.gbl.RepeatedImplicitTuple
+        pair = cppjit.gbl.std.pair["int", "int"]
+
+        def n_ctor_overloads():
+            # the instantiated converting constructor is cached on the class
+            # under the constructor's own name
+            ol = pair.__dict__.get("pair")
+            return len(ol.__doc__.split("\n")) if ol is not None else 0
+
+        assert ns.first((1, 2)) == 1
+        n = n_ctor_overloads()
+        assert n > 0
+
+        for i in range(20):
+            assert ns.first((i, 2)) == i
+        assert n_ctor_overloads() == n