sumtype icon indicating copy to clipboard operation
sumtype copied to clipboard

The class-based syntax confuses linters

Open kkirka opened this issue 5 years ago • 4 comments

What I need is rust-like enums in python, and your library really suits my needs. However, my linter is pretty much confused by the syntax you propose. I'm not sure whether one can work around this but I really don't want to put "ignore" comments every time I declare or use the ADTs.

The error messages are as follows:

  • "Usually the first parameter of a method is called self" — in a declaration if the alternative has some arguments,
  • "Method must have a first parameter, usually called self" — in a declaration if the alternative does not have any arguments,
  • "Function 'X' does not return anything" — when trying to instantiate an alternative.

I'm using PyCharm 2017.2 Community Edition but I believe other linters don't like the syntax as well.

kkirka avatar Oct 04 '18 09:10 kkirka

Hi, thank you for your interest! Yeah, compatibility with static analysis tools is a problem. My linter is okay with def Just(x): ..., but complains about def Nothing(): ..., and mypy just fails completely¹. It seems it's not easy to solve this while maintaining a nice syntax. I thought about it a bit, and this reply ended up being a bit of brain dump, but bear with me :)

The first two error messages could easily be solved by supporting a dummy first parameter called self or _:

class Example(sumtype):
    def Foo(self, x: int, y: int): ...
    def Bar(self, z: str): ...
    def Zap(self):  ...

However, I'm not too happy about this solution, because it's misleading to readers – it makes Foo/Bar/Zap look like alternative __init__s, while their actual semantics are:

class Example(sumtype):
    @classmethod
    def Foo(cls, x: int, y: int) -> 'Example':
        ...
    @classmethod
    def Bar(cls, z: str) -> 'Example':
        ...
    @classmethod
    def Zap(cls) -> 'Example':
        ...

i.e. they behave more like __new__ on immutable classes. (modulo implementation details) I guess I could add support for definitions like the one above – it's verbose, but it should alleviate some of the problems with static analyzers.

A (hopefully) better way would be to add a new class definition syntax:

class NewExample(sumtype):
    class Foo: x: int; y: int
    class Bar: y: str
    class Zap: ...

(unfortunately, it requires Python 3.6+'s variable type annotations)

Those inner classes won't actually be present in the final class, so it's still a bit misleading, but it's close enough. Perhaps it's also more readable to someone who stumbles upon a sumtype class definition because it doesn't abuse method definitions like the current syntax does. And it's pretty easy to implement :smile:

Please let me know what you think about these solutions, or if you have any other ideas or alternative syntax proposals! Also, I don't have PyCharm installed, so if you could test these and see which one works best with its linter, that'd be great.


¹ I'm working on a plugin to add support for sumtype-generated classes though, it should be ready for release soon!

lubieowoce avatar Oct 04 '18 21:10 lubieowoce

Also, it looks like I forgot to put it in the README, but there's an alternate way of creating a sumtype:

Example = sumtype(
  'Example', [
    ('Foo', [('x', int), ('y', int)]),
    ('Bar', [('y', str)]),
    ('Zap', []),
])

It's not pretty (unless you like lisp 😉), but it should reduce linter complaints.

In addition, you can make a syntax like this work without any changes to sumtype – maybe the linter will like this better?

class Example(sumtype):
    Foo = variant(x=int, y=int)
    Bar = variant(y=str)
    Zap = variant()


from sys import version_info as PY_VERSION

def func_from_signature(**args_and_types) -> 'typing.Callable':
    'Creates a dummy function with the specified signature'
    # CPython preserves kwargs order since 3.6
    # (required by the language standard in 3.7+)
    assert PY_VERSION >= (3,6)
    args_str = ', '.join(args_and_types.keys())
    func_str = 'lambda {}: ...'.format(args_str)
    func = eval(func_str)
    func.__annotations__ = args_and_types
    return func

variant = func_from_signature

lubieowoce avatar Oct 04 '18 23:10 lubieowoce

The "variant"-based syntax works fine, there are no linter errors and the class behavior is as expected. Moreover, I personally consider this syntax easier to read than the other ones you propose. The only concern I have is that it moves the type definitions from the "type" level to the value level (i.e. uses "=" instead of ":") but I believe it's more about the "philosophy" than readability, and I can live with it. By the way, why don't you make this syntax possible by default (e.g. by including the definition of the "variant" function into the library)? Are there any performance drawbacks to this solution?

kkirka avatar Oct 11 '18 08:10 kkirka

I personally consider [the V = variant(...)] syntax easier to read

Thank you, that's valuable feedback!

The only concern I have is that it moves the type definitions from the "type" level to the value level (i.e. uses "=" instead of ":")

Nicely put, that's exactly my issue with that syntax. It's one of the reasons I went with the current def-based syntax – in Py3.5, function definitions are the only place where type annotations are allowed. And yeah, philosophically speaking, I'd prefer the default syntax to be something "type-level", even if it looks a bit odd. (but see the next paragraph)

By the way, why don't you make this syntax possible by default (e.g. by including the definition of the "variant" function into the library)

I'm a bit torn about this. On the one hand, I'd prefer to have a single "official" syntax for consistency¹, and this one doesn't sit right with me as described above. On the other hand, I'm not sure syntax bike-shedding is a hill I want to die on – it's all just different ways of shoehorning new pseudo-syntax into Python anyway. Either way, I'm planning to make the bit that interprets the class body more pluggable, so when that lands, I can add experimental support for this V = variant(...) syntax, possibly making it more official in the future.

In the meantime: the variant recipe I posted only relies on the public API², and I intend to respect semver w.r.t public API changes, so it's guaranteed to work at least until 1.0.


¹ This also makes the mypy plugin easier to develop – every additional syntax will probably require dedicated support in the plugin. (unless I figure out a way to apply sumtype's code on the AST level, which is where mypy works at)

² TODO - document what the public API is in a better way than "what the readme mentions"

lubieowoce avatar Oct 11 '18 15:10 lubieowoce