YosysHQ / mau

Modular Application Utilities
ISC License
3 stars 2 forks source link

Mau uses __dunder__ names that are reserved by Python #11

Open whitequark opened 2 months ago

whitequark commented 2 months ago

I found code in mau like this:

    def __option_parser_result__(self, proto: OptionParser[T]) -> T:

Unfortunately this is actually undefined behavior; Python reserves all __x__ names for itself. You might want to use _option_parser_result_ instead, though this would be bizarre and unconventional. There is also no reason to use __names, since they provide absolutely no extra protection--you can still access _ClassName__names from the outside like with any other attribute. The __name syntax exists for collision avoidance, not safety.

The conventional way would be to use a single prefixed underscore for all of the private functions and methods, since this provides the maximum possible safety (none, other than being labelled "do not touch") and uses conventional syntax.

jix commented 2 months ago

The use of __ prefixes for collision avoidance (outside of the __dunder__ methods) is intentional and should be limited to places where the intended use of the API involves user code implementing subclasses, i.e. where there is an actual collision risk.

I'll think about what to do about the __dunder__ methods.

whitequark commented 2 months ago

The use of __ prefixes for collision avoidance (outside of the __dunder__ methods) is intentional and should be limited to places where the intended use of the API involves user code implementing subclasses, i.e. where there is an actual collision risk.

Ah, I see. In my (admittedly incomplete) review I couldn't see a single file without using __ prefixes so I jumped to a conclusion.

By the way (I am reviewing mau with my packager hat on here)--is the plan to put it on PyPI and depend on it in Yosys itself? Or is the plan to vendor it? I can see both plans to have some undesirable consequences and I'd rather know sooner than later.

jix commented 2 months ago

There are currently no plans to add this as dependency for yosys (or to any of the python programs in the main yosys repo like yosys-smtbmc). The plan is to eventually migrate all of our frontend tools (SBY, EQY, etc.) to use this, some newer and/or experimental frontend tools already use mau. I'd be in favor of eventually releasing mau as well as these frontend tools themselves also on PyPI, but still have to discuss how that would fit into our release process (cc @mmicko).

whitequark commented 2 months ago

Thanks, this actually clears up my concerns (which were mainly about Yosys itself, as that is a hybrid C++/Python application that is a considerable pain to package as such), so feel free to ignore this line of questioning.

Pure Python applications are much easier to package for Wasm: either it uses subprocesses, in which case I just don't, or it doesn't, in which case pyodide can usually install and run it as-is.