qntm / greenery

Regular expression manipulation library
http://qntm.org/greenery
MIT License
331 stars 40 forks source link

Pylance sees greenery.lego.parse as returning NoReturn #52

Closed Delwin9999 closed 2 years ago

Delwin9999 commented 3 years ago

This code fails to pass Pylance with a 'Cannot access member "everythingbut" for type "NoReturn"'

from greenery.lego import parse

test_regex = parse(".")
test_regex.everythingbut()

The heart of the issue is that the lego class has a number of functions that do not have a return type hint and are not marked with @abstractmethod thus forcing Pylance to infer that the class never actually returns and thus is labeled NoReturn.

Suggestions would be to add at least return type hints to the lego and pattern classes.

qntm commented 3 years ago

That strikes me as a problem with Pylance, surely? If there is no type hint, it should infer that the method could return anything, not that it doesn't return at all.

Delwin9999 commented 3 years ago

Without the type hint it's actually undecidable. You can't call it 'Any' because a lot of the functions actually never return - they only raise. If you want the base class to be skipped then you need to make it abstract and have any function that only raises and never returns marked as abstract.

microsoft/pylance-release#248 comment

There is another way though... It appears that you can trigger Pylance's assumption of abstract if a method if it: (https://github.com/microsoft/pylance-release/issues/248#issuecomment-683432834)

The issue here is you're using raise Error("Not Implemented") instead of raise NotImplementedError

qntm commented 3 years ago

OK so since Pylance is a very recent thing I guess the actual problem here is just a shortage of proper type annotations for greenery and/or general aspects of the code making it difficult to statically analyse to determine types.

I have done some vague research into this but the advice seems to be somewhat scattered at present. I will address this at some point but I can't promise a particular date.

Seanny123 commented 2 years ago

Would you be okay with a PR replacing raise Error("Not Implemented") instead of raise NotImplementedError?

I could then make a separate PR adding some basic type annotations if you're open to that as well.

qntm commented 2 years ago

Yes, replacing an ad hoc Error with a proper NotImplementedError seems like a logical step forward, especially if it helps with type inference downstream. I would accept that PR. (EDIT: Probably.)

As for adding type annotations, I would certainly take a look at that PR if you raised it, but I can't say for sure unless I saw it.

qntm commented 2 years ago

greenery version 3.3.3 has these changes in.