Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make transformation of class annotations configurable to fix issue with transitive inheritance from TypedDict #54

Open
whummer opened this issue May 21, 2022 · 1 comment
Labels
enhancement New feature or request

Comments

@whummer
Copy link

whummer commented May 21, 2022

First of all, huge thanks for providing this fantastic library! 🙌

We've discovered a small issue with type declarations and transitive class inheritance. If we use this sample code:

from typing import Optional, TypedDict
class A(TypedDict):
    arg1: str
class B(A):
    arg2: Optional[int]
class C(B):
    arg3: Optional[str]

... it would get minified to:

from typing import Optional,TypedDict
class A(TypedDict):arg1:str
class B(A):arg2:0
class C(B):arg3:0

... which is invalid Python code (tested under 3.8, 3.10, but also applies to other versions):

$ python test.py
  ...
  File "~/.pyenv/versions/3.10.4/lib/python3.10/typing.py", line 176, in _type_check
    raise TypeError(f"{msg} Got {arg!r:.100}.")
TypeError: TypedDict('Name', {f0: t0, f1: t1, ...}); each t must be a type Got 0.

The issue is that the library is currently not able to detect transitive inheritance dependencies (for good reasons, as this is a hard problem - probably infeasible to solve in the general case, as class hierarchies may be distributed across different source files, and minification is only performed within the scope of a single source file.)

One solution is to use the remove_annotations=False flag to retain all annotations, but in a larger codebase it can actually be beneficial to remove type declarations, especially from function/method args.

The code in question is this piece:

else:
# Valueless annotations cause the interpreter to treat the variable as a local.
# I don't know of another way to do that without assigning to it, so
# keep it as an AnnAssign, but replace the annotation with '0'
node.annotation = self.add_child(ast.Num(0), parent=node.parent)
return node
The ideal case would be if we can introduce a config flag to disable the transformation that is happening for class attributes in this block.

@dflook Would it make sense to distinguish between (1) function annotations and (2) class annotations, and introduce a new flag remove_annotations_class? We could then either introduce a new SuiteTransformer for class annotations, or alternatively leave the current structure and skip executing visit_AnnAssign if remove_annotations_class is False. Happy to work towards a PR, but would like to get your thoughts and guidance first.. 👍 Thanks!

@whummer whummer changed the title Make configurable Make transformation of class annotations configurable May 21, 2022
@whummer whummer changed the title Make transformation of class annotations configurable Make transformation of class annotations configurable to fix issue with transitive inheritance from TypedDict May 21, 2022
@dflook
Copy link
Owner

dflook commented May 21, 2022

Hi @whummer, thanks for creating an issue!

As you said, I think tracking transitive inheritance across arbitrary modules would be quite difficult and easily defeated by the dynamic nature of python. But there is still some room for improvement - I think it could be done within a module, if not completely at least well enough that it results in safer transformations.

The remove annotations transformation is a tricky one because even if we can supress it for specific classes from the python standard library, there are many other classes that should have annotations preserved that python-minifier can't know.

I think it makes sense to have separate options for removing annotations from variables, functions or class attributes.
Variable annotations are much safer to remove (in the sense of breaking arbitrary code) than function annotations, which are again much safer than class attribute annotations. The minification options are already a bit cluttered though, and coarse in that they apply to an entire module.

I'm considering a mechanism such that you can specify much more precise minification options, per namespace.
Something like (pseudo-yaml):

src/__main__.py::*:
  remove_annotations:
    variables: True
    function_arguments: True
    function_return: True
    class_attributes: True
src/__main__.py::MyClass:
  remove_annotations:
    class_attributes: False

For now though, If you want to work towards a PR to solve your problem I would suggest adding to RemoveAnnotations to accept an argument in __init__ which enables or disables class attribute annotations.

@dflook dflook added the enhancement New feature or request label May 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants