Skip to content

Conversation

@mbyrnepr2
Copy link
Member

@mbyrnepr2 mbyrnepr2 commented May 12, 2024

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

Add the __annotations__ attribute to the ClassDef object model.
Pylint now does not emit a no-member error when accessing __annotations__ in the following cases:

case 1.

class Test:
    print(__annotations__)

case 2.

from typing import TypedDict

OtherTypedDict = TypedDict('OtherTypedDict', {'a': int, 'b': str})
print(OtherTypedDict.__annotations__)

case 1 is similar to the behaviour of some of the other special attributes:

class Test:
    print(__module__)
    print(__qualname__)
    print(__annotations__)

It turns out that this fix also fixed case 2.
However, it's still unclear why case 2 was only a false positive for Python 3.9 and older 😬

Closes pylint-dev/pylint#7126

Pylint now does not emit a ``no-member`` error when accessing
``__annotations`` in the following cases:

```
class Test:
    print(__annotations__)
```

```
from typing import TypedDict

OtherTypedDict = TypedDict('OtherTypedDict', {'a': int, 'b': str})
print(OtherTypedDict.__annotations__)
```

Closes pylint-dev/pylint#7126
@codecov
Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 92.72%. Comparing base (2c38c02) to head (90f6f67).
Report is 185 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2431      +/-   ##
==========================================
- Coverage   92.78%   92.72%   -0.06%     
==========================================
  Files          94       94              
  Lines       11098    10996     -102     
==========================================
- Hits        10297    10196     -101     
+ Misses        801      800       -1     
Flag Coverage Ξ”
linux 92.60% <100.00%> (+0.01%) ⬆️
pypy 92.72% <100.00%> (-0.06%) ⬇️
windows 92.70% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Ξ”
astroid/interpreter/objectmodel.py 93.27% <100.00%> (+0.02%) ⬆️
astroid/nodes/scoped_nodes/scoped_nodes.py 92.77% <100.00%> (+0.25%) ⬆️

... and 70 files with indirect coverage changes

@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review May 12, 2024 17:40
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the change, but should this behind a version flag?

See https://docs.python.org/3/howto/annotations.html. It seems it was only added to all objects in 3.10+?

@mbyrnepr2
Copy link
Member Author

I'm fine with the change, but should this behind a version flag?

See https://docs.python.org/3/howto/annotations.html. It seems it was only added to all objects in 3.10+?

I see what you mean @DanielNoord great point. On Python 3.8, for example, we can demonstrate your point (this corresponds to case 2 in the description above):

>>> class Fruit:
...     pass
... 
>>> Fruit().__annotations__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'Fruit' object has no attribute '__annotations__'

Now, let's look at case 1 on Python 3.8:

>>> class Fruit:
...     print(__annotations__)
... 
{}
>>> 

Without the fix, Pylint does emit the error here. At this moment I don't know if it's better to make a distinction or keep it as-is but I'm open to ideas.

@DanielNoord
Copy link
Collaborator

I think a distinction would be better as that better represents the actual versions right?

@mbyrnepr2
Copy link
Member Author

I would need to investigate a bit further @DanielNoord but my thinking is that we need the attribute anyway to satisfy both cases; so is the distinction necessary? Possibly but I need to see how that could work

DanielNoord
DanielNoord previously approved these changes Jun 26, 2024
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this some more, we shouldn't optimize for old versions. Most people will be running their linter with any of the newer versions anyway. Let's merge! (with one last nit :))

@jacobtylerwalls
Copy link
Member

However, it's still unclear why case 2 was only a false positive for Python 3.9 and older 😬

At a glance, I expect that's because on 3.9 and below, the base class __annotations__ was returned.

@jacobtylerwalls jacobtylerwalls added this to the 3.3.0 milestone Jul 28, 2024
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!



def test_class_annotations_typed_dict() -> None:
"""Test that we can access class annotations on various TypedDicts"""
Copy link
Member

@jacobtylerwalls jacobtylerwalls Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not important at the present moment, but elsewhere I've been encouraged to avoid boilerplate like "test that" and "we", and I think it's a helpful edit ;)

@jacobtylerwalls jacobtylerwalls merged commit 6f1eb89 into pylint-dev:main Jul 28, 2024
jacobtylerwalls added a commit that referenced this pull request Jul 28, 2024
@mbyrnepr2 mbyrnepr2 deleted the pylint_class_annotations branch July 28, 2024 14:59
jacobtylerwalls added a commit that referenced this pull request Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False positive no-member for TypedDict.__annotations__

3 participants