pyosys: fix ref-only classes, implicit conversions

+ cleanup

.github/workflows/wheels.yml

@@ -2,7 +2,6 @@ name: Build Wheels for PyPI
 
 # run every Sunday at 10 AM
 on:
-  push: # TODO: REMOVE THIS, DO NOT MERGE TO UPSTREAM THIS IS JUST SO I DON'T HAVE TO MANUALLY RUN THE WORKFLOW WITH EVERY PUSH
   workflow_dispatch:
   schedule:
     - cron: "0 10 * * 0"

Makefile

@@ -1017,12 +1017,12 @@ ifeq ($(ENABLE_LIBYOSYS),1)
 	if [ -n "$(STRIP)" ]; then $(INSTALL_SUDO) $(STRIP) -S $(DESTDIR)$(LIBDIR)/libyosys.so; fi
 ifeq ($(ENABLE_PYOSYS),1)
 	$(INSTALL_SUDO) mkdir -p $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys
+	$(INSTALL_SUDO) cp pyosys/__init__.py $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys/__init__.py
 	$(INSTALL_SUDO) cp libyosys.so $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys/libyosys.so
 	$(INSTALL_SUDO) cp -r share $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys
 ifeq ($(ENABLE_ABC),1)
 	$(INSTALL_SUDO) cp yosys-abc $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys/yosys-abc
 endif
-	$(INSTALL_SUDO) cp misc/__init__.py $(DESTDIR)$(PYTHON_DESTDIR)/$(subst -,_,$(PROGRAM_PREFIX))pyosys/
 endif
 endif
 ifeq ($(ENABLE_PLUGINS),1)

kernel/yosys.cc

@@ -198,6 +198,15 @@ bool already_shutdown = false;
 PYBIND11_MODULE(pyosys, m) {
 	m.add_object("__path__", py::list());
 }
+
+// Catch uses of 'import libyosys' which can import libyosys.so, causing a ton
+// of symbol collisions and overall weird behavior.
+//
+// This should not affect using wheels as the dylib has to actually be called
+// libyosys_dummy.so for this function to be interacted with at all.
+PYBIND11_MODULE(libyosys_dummy, _) {
+	throw py::import_error("Change your import from 'import libyosys' to 'from pyosys import libyosys'.");
+}
 #endif
 
 void yosys_setup()
@@ -217,6 +226,8 @@ void yosys_setup()
 		PyImport_AppendInittab((char*)"pyosys.libyosys", PyInit_libyosys);
 		// compatibility with wheels
 		PyImport_AppendInittab((char*)"pyosys", PyInit_pyosys);
+		// prevent catastrophes
+		PyImport_AppendInittab((char*)"libyosys", PyInit_libyosys_dummy);
 		Py_Initialize();
 		PyRun_SimpleString("import sys");
 		signal(SIGINT, SIG_DFL);

pyosys/generator.py

@@ -33,13 +33,13 @@ is a common problem. ``ruff check pyosys/generator.py`` suffices.
 import os
 import io
 import shutil
+import argparse
 from pathlib import Path
 from sysconfig import get_paths
 from dataclasses import dataclass, field
 from typing import Any, Dict, FrozenSet, Iterable, Tuple, Union, Optional, List
 
 import pybind11
-import argparse
 from cxxheaderparser.simple import parse_file, ClassScope, NamespaceScope, EnumDecl
 from cxxheaderparser.options import ParserOptions
 from cxxheaderparser.preprocessor import make_gcc_preprocessor
@@ -84,6 +84,7 @@ class PyosysClass:
     :param denylist: If specified, one or more methods can be excluded from
         wrapping.
     """
+
     name: str
     ref_only: bool = False
 
@@ -101,6 +102,7 @@ class PyosysHeader:
     :param classes: A list of ``PyosysClass`` classes to be wrapped
     :param enums: A list of enums to be wrapped
     """
+
     name: str
     classes: List[PyosysClass] = field(default_factory=lambda: [])
 
@@ -110,11 +112,13 @@ class PyosysHeader:
             for cls in classes:
                 self.classes_by_name[cls.name] = cls
 
+
 # MARK: Inclusion and Exclusion
 global_denylist = frozenset(
     {
         # deprecated
         "builtin_ff_cell_types",
+        "logv_file_error",
         # no implementation
         "set_verific_logging",
         # can't bridge to python cleanly
@@ -167,7 +171,11 @@ pyosys_headers = [
                     }
                 ),
             ),
-            PyosysClass("Const", string_expr="s.as_string()", denylist=frozenset({"bits", "bitvectorize"})),
+            PyosysClass(
+                "Const",
+                string_expr="s.as_string()",
+                denylist=frozenset({"bits", "bitvectorize"}),
+            ),
             PyosysClass("AttrObject", denylist=frozenset({"get_blackbox_attribute"})),
             PyosysClass("NamedObject", denylist=frozenset({"get_blackbox_attribute"})),
             PyosysClass("Selection"),
@@ -211,7 +219,6 @@ pyosys_headers = [
             ),
             PyosysClass(
                 "Design",
-                ref_only=True,
                 string_expr="s.hashidx_",
                 hash_expr="s",
                 denylist=frozenset({"selected_whole_modules"}),  # deprecated
@@ -227,6 +234,7 @@ class PyosysType:
     Bit of a hacky object all-around: this is more or less used to encapsulate
     container types so they can be later made opaque using pybind.
     """
+
     base: str
     specialization: Tuple["PyosysType", ...]
     const: bool = False
@@ -340,6 +348,10 @@ class PyosysWrapperGenerator(object):
                 f'\tpy::hashlib::bind_{container.base}<{", ".join(tpl_args)}>(m, "{container.generate_identifier()}");',
                 file=self.f_inc,
             )
+            print(
+                f"\tpy::implicitly_convertible<py::iterable, {identifier}>();",
+                file=self.f_inc,
+            )
         print(f"}}", file=self.f_inc)
 
     # helpers
@@ -414,14 +426,21 @@ class PyosysWrapperGenerator(object):
             self.found_containers.update(self.find_containers(supported, target.type))
 
     # processors
-    def get_overload_cast(self, function: Function, class_basename: Optional[str]) -> str:
+    def get_overload_cast(
+        self, function: Function, class_basename: Optional[str]
+    ) -> str:
         is_method = isinstance(function, Method)
         function_return_type = function.return_type.format()
-        if class_basename == "Const" and function_return_type in {"iterator", "const_iterator"}:
+        if class_basename == "Const" and function_return_type in {
+            "iterator",
+            "const_iterator",
+        }:
             # HACK: qualify Const's iterators
             function_return_type = f"{class_basename}::{function_return_type}"
 
-        pointer_kind = f"{class_basename}::*" if (is_method and not function.static) else "*"
+        pointer_kind = (
+            f"{class_basename}::*" if (is_method and not function.static) else "*"
+        )
 
         retval = f"static_cast <"
         retval += function_return_type
@@ -437,10 +456,17 @@ class PyosysWrapperGenerator(object):
         retval += ")"
         return retval
 
-    def get_definition_args(self, function: Function, class_basename: Optional[str], python_name_override: Optional[str] = None) -> List[str]:
+    def get_definition_args(
+        self,
+        function: Function,
+        class_basename: Optional[str],
+        python_name_override: Optional[str] = None,
+    ) -> List[str]:
         function_basename = function.name.segments[-1].format()
 
-        python_function_basename = python_name_override or keyword_aliases.get(function_basename, function_basename)
+        python_function_basename = python_name_override or keyword_aliases.get(
+            function_basename, function_basename
+        )
 
         def_args = [f'"{python_function_basename}"']
         def_args.append(self.get_overload_cast(function, class_basename))
@@ -453,7 +479,7 @@ class PyosysWrapperGenerator(object):
 
         return def_args
 
-    def process_method(self, function: Method, class_basename: str):
+    def process_method(self, metadata: PyosysClass, function: Method):
         if (
             function.deleted
             or function.template
@@ -473,6 +499,9 @@ class PyosysWrapperGenerator(object):
             return
 
         if function.constructor:
+            if (
+                not metadata.ref_only
+            ):  # ref-only classes should not be constructed from python
                 print(
                     f"\t\t\t.def(py::init<{self.get_parameter_types(function)}>())",
                     file=self.f,
@@ -496,15 +525,13 @@ class PyosysWrapperGenerator(object):
         if function.static:
             definition_fn = "def_static"
 
-        print(f"\t\t\t.{definition_fn}({', '.join(self.get_definition_args(function, class_basename, python_name_override))})", file=self.f)
+        print(
+            f"\t\t\t.{definition_fn}({', '.join(self.get_definition_args(function, metadata.name, python_name_override))})",
+            file=self.f,
+        )
 
     def process_function(self, function: Function):
-        if (
+        if function.deleted or function.template or function.vararg or function.static:
-            function.deleted
-            or function.template
-            or function.vararg
-            or function.static
-        ):
             return
 
         if function.operator is not None:
@@ -516,9 +543,12 @@ class PyosysWrapperGenerator(object):
 
         self.register_containers(function)
 
-        print(f"\t\t\tm.def({', '.join(self.get_definition_args(function, None))});", file=self.f)
+        print(
+            f"\t\t\tm.def({', '.join(self.get_definition_args(function, None))});",
+            file=self.f,
+        )
 
-    def process_field(self, field: Field, class_basename: str):
+    def process_field(self, metadata: PyosysClass, field: Field):
         if field.access != "public":
             return
 
@@ -544,7 +574,7 @@ class PyosysWrapperGenerator(object):
         field_python_basename = keyword_aliases.get(field.name, field.name)
 
         print(
-            f'\t\t\t.{definition_fn}("{field_python_basename}", &{class_basename}::{field.name})',
+            f'\t\t\t.{definition_fn}("{field_python_basename}", &{metadata.name}::{field.name})',
             file=self.f,
         )
 
@@ -558,9 +588,13 @@ class PyosysWrapperGenerator(object):
 
         self.register_containers(variable)
 
-        definition_fn = f"def_{'readonly' if variable.type.const else 'readwrite'}_static"
+        definition_fn = (
+            f"def_{'readonly' if variable.type.const else 'readwrite'}_static"
+        )
 
-        variable_python_basename = keyword_aliases.get(variable_basename, variable_basename)
+        variable_python_basename = keyword_aliases.get(
+            variable_basename, variable_basename
+        )
         variable_name = variable.name.format()
 
         print(
@@ -569,21 +603,18 @@ class PyosysWrapperGenerator(object):
         )
 
     def process_class_members(
-        self,
+        self, metadata: PyosysClass, cls: ClassScope, basename: str
-        metadata: PyosysClass,
-        cls: ClassScope,
-        basename: str
     ):
         for method in cls.methods:
             if method.name.segments[-1].name in metadata.denylist:
                 continue
-            self.process_method(method, basename)
+            self.process_method(metadata, method)
 
         visited_anonymous_unions = set()
         for field_ in cls.fields:
             if field_.name in metadata.denylist:
                 continue
-            self.process_field(field_, basename)
+            self.process_field(metadata, field_)
 
             # Handle anonymous unions
             for subclass in cls.classes:
@@ -594,7 +625,7 @@ class PyosysWrapperGenerator(object):
                         continue
                     visited_anonymous_unions.add(au.id)
                     for subfield in subclass.fields:
-                        self.process_field(subfield, basename)
+                        self.process_field(metadata, subfield)
 
     def process_class(
         self,
@@ -627,10 +658,16 @@ class PyosysWrapperGenerator(object):
                 self.process_class_members(metadata, base_scope, basename)
 
         if expr := metadata.string_expr:
-            print(f'\t\t.def("__str__", [](const {basename} &s) {{ return {expr}; }})', file=self.f)
+            print(
+                f'\t\t.def("__str__", [](const {basename} &s) {{ return {expr}; }})',
+                file=self.f,
+            )
 
         if expr := metadata.hash_expr:
-            print(f'\t\t.def("__hash__", [](const {basename} &s) {{ return run_hash({expr}); }})', file=self.f)
+            print(
+                f'\t\t.def("__hash__", [](const {basename} &s) {{ return run_hash({expr}); }})',
+                file=self.f,
+            )
 
         print(f"\t\t;}}", file=self.f)
 
@@ -653,10 +690,12 @@ class PyosysWrapperGenerator(object):
         enum_class = enum.typename.classkey == "enum class"
         for value in enum.values:
             enum_class_qualifier = f"{basename}::" * enum_class
-            print(f'\t\t\t.value("{value.name}", {enum_class_qualifier}{value.name})', file=self.f)
+            print(
+                f'\t\t\t.value("{value.name}", {enum_class_qualifier}{value.name})',
+                file=self.f,
+            )
         print(f"\t\t\t.finalize();}}", file=self.f)
 
-
     def process_namespace(
         self,
         header: PyosysHeader,
@@ -668,7 +707,10 @@ class PyosysWrapperGenerator(object):
         if len(namespace_components) and (len(ns.functions) + len(ns.variables)):
             # TODO: Not essential but maybe move namespace usage into
             # process_function for consistency?
-            print(f"\t\t{{ using namespace {'::'.join(namespace_components)};", file=self.f)
+            print(
+                f"\t\t{{ using namespace {'::'.join(namespace_components)};",
+                file=self.f,
+            )
             for function in ns.functions:
                 self.process_function(function)
             for variable in ns.variables:

pyosys/hashlib.h

@@ -95,8 +95,8 @@ void difference(C &lhs, const iterable &rhs) {
 template <typename C, typename T>
 void intersect(C &lhs, const iterable &rhs) {
 	// Doing it in-place is a lot slower
-	// TODO?: Leave modifying lhs to caller (saves a copy) but complicates
+	// TODO?: Leave modifying lhs to caller (saves a copy in some cases)
-	//        chaining intersections.
+	//        but complicates chaining intersections.
 	C storage(lhs);
 
 	for (auto &element_cxx: lhs) {
@@ -449,6 +449,13 @@ void bind_idict(module &m, const char *name_cstr) {
 	auto cls = class_<C>(m, name_cstr)
 		.def(init<>())
 		.def(init<const C &>()) // copy constructor
+		.def(init([](const iterable &other){ // copy instructor from arbitrary iterables
+			auto s = new C();
+			for (auto &e: other) {
+				(*s)(cast<K>(e));
+			}
+			return s;
+		}))
 		.def("__len__", [](const C &s){ return (size_t)s.size(); })
 		.def("__getitem__", [](const C &s, int v) { return s[v]; })
 		.def("__call__", [](C &s, const K &k) { return s(k); })
@@ -480,20 +487,20 @@ void bind_idict(module &m, const char *name_cstr) {
 		.def("items", [](args _){
 			throw type_error("idicts do not support pairwise iteration");
 		})
-		.def("update", [](C &s, iterable iterable) {
+		.def("update", [](C &s, iterable other) {
-			for (auto &e: iterable) {
+			for (auto &e: other) {
 				s(cast<K>(e));
 			}
 		})
-		.def("__or__", [](const C &s, iterable iterable) {
+		.def("__or__", [](const C &s, iterable other) {
 			auto result = new C(s);
-			for (auto &e: iterable) {
+			for (auto &e: other) {
 				(*result)(cast<K>(e));
 			}
 			return result;
 		})
-		.def("__ior__", [](C &s, iterable iterable) {
+		.def("__ior__", [](C &s, iterable other) {
-			for (auto &e: iterable) {
+			for (auto &e: other) {
 				s(cast<K>(e));
 			}
 			return s;

pyosys/wrappers_tpl.cc

@@ -21,7 +21,6 @@
 // <!-- generated includes -->
 #include <pybind11/pybind11.h>
 #include <pybind11/native_enum.h>
-
 #include "pyosys/hashlib.h"
 
 namespace py = pybind11;
@@ -35,7 +34,7 @@ using namespace RTLIL;
 
 #include "wrappers.inc.cc"
 
-namespace YOSYS_PYTHON {
+namespace pyosys {
 	struct Globals {};
 
 	// Trampolines for Classes with Python-Overridable Virtual Methods
@@ -160,12 +159,6 @@ namespace YOSYS_PYTHON {
 		}
 	};
 
-	/// @brief Use an auxiliary function to adapt the legacy function.
-	void log_to_stream(py::object object)
-	{
-		// TODO
-	};
-
 	PYBIND11_MODULE(libyosys, m) {
 		// this code is run on import
 		m.doc() = "python access to libyosys";
@@ -195,9 +188,9 @@ namespace YOSYS_PYTHON {
 		auto global_variables = py::class_<Globals>(m, "Globals");
 
 		// Trampoline Classes
-		py::class_<Pass, YOSYS_PYTHON::PassTrampoline, std::unique_ptr<Pass, py::nodelete>>(m, "Pass")
+		py::class_<Pass, pyosys::PassTrampoline, std::unique_ptr<Pass, py::nodelete>>(m, "Pass")
 			.def(py::init([](std::string name, std::string short_help) {
-				auto created = new YOSYS_PYTHON::PassTrampoline(name, short_help);
+				auto created = new pyosys::PassTrampoline(name, short_help);
 				Pass::init_register();
 				return created;
 			}), py::arg("name"), py::arg("short_help"))
@@ -219,9 +212,9 @@ namespace YOSYS_PYTHON {
 			.def("call", py::overload_cast<RTLIL::Design *,std::vector<std::string>>(&Pass::call))
 		;
 
-		py::class_<RTLIL::Monitor, YOSYS_PYTHON::MonitorTrampoline>(m, "Monitor")
+		py::class_<RTLIL::Monitor, pyosys::MonitorTrampoline>(m, "Monitor")
 			.def(py::init([]() {
-				return new YOSYS_PYTHON::MonitorTrampoline();
+				return new pyosys::MonitorTrampoline();
 			}))
 			.def("notify_module_add", &RTLIL::Monitor::notify_module_add)
 			.def("notify_module_del", &RTLIL::Monitor::notify_module_del)
@@ -255,6 +248,9 @@ namespace YOSYS_PYTHON {
 		bind_autogenerated_opaque_containers(m);
 
 		// <!-- generated pymod-level code -->
+
+		py::implicitly_convertible<std::string, RTLIL::IdString>();
+		py::implicitly_convertible<const char *, RTLIL::IdString>();
 	};
 };
 

tests/pyosys/test_idict.py

@@ -1,11 +1,11 @@
 from pyosys import libyosys as ys
 
 my_idict = ys.IdstringIdict()
-print(my_idict(ys.IdString("\\hello")))
+print(my_idict(ys.IdString("\\hello"))) # test explicit IdString construction
-print(my_idict(ys.IdString("\\world")))
+print(my_idict("\\world"))
-print(my_idict.get(ys.IdString("\\world")))
+print(my_idict.get("\\world"))
 try:
-	print(my_idict.get(ys.IdString("\\dummy")))
+	print(my_idict.get("\\dummy"))
 except IndexError as e:
 	print(f"{repr(e)}")
 print(my_idict[0])
@@ -22,10 +22,10 @@ current_len = len(my_idict)
 assert current_len == 2, "copy"
 
 my_copy = my_idict.copy()
-my_copy(ys.IdString("\\copy"))
+my_copy("\\copy")
 assert len(my_idict) == current_len, "copy seemed to have mutate original idict"
 assert len(my_copy) == current_len + 1, "copy not behaving as expected"
 
 current_copy_len = len(my_copy)
-my_copy |= (ys.IdString(e) for e in ("\\the", "\\world")) # 1 new element
+my_copy |= ("\\the", "\\world") # 1 new element
 assert len(my_copy) == current_copy_len + 1, "or operator returned unexpected result"