Strange type error on `attrs` field with converter (affected by file order) since 1.1.384

[Azureblade3808]

Look at following 4 files -

a.py

import attrs

@attrs.mutable(slots=False)
class A:
    x: str = attrs.field(converter=str)

b.py

import attrs

from .a import A

@attrs.mutable
class B(A):
    pass

c.py

import attrs

from .d import D

@attrs.mutable
class C(D):
    pass

d.py

import attrs

@attrs.mutable(slots=False)
class D:
    x: str = attrs.field(converter=str)

We can see that "a.py" is similar to "d.py" and "b.py" is similar to "c.py".

When they get placed into one folder, Pyright (1.1.384 and 1.1.385, both Pylance and CLI) complains about D.x (but not A.x).

The only significant difference between A and D is the file order, and I believe that's the key to this issue.

[erictraut]

There are several issues here.

First, the "override" error is being reported in the wrong location.

Second, the error probably shouldn't be reported in the first place. One could argue this is an invalid override, but it's not unsafe, and it's not something that a user has any control over. It's due to the fact that attrs synthesizes a new descriptor object on the subclass for fields that are found in the parent class.

Third, if an error is generated, it should be consistent and not dependent on file import ordering.

@debonte, this regression is a result of the recent change you made to add a declaration for the synthesized converter descriptor object. The problem is that there are cases where there is no valid declaration, such as when a new converter descriptor is synthesized in a subclass (such as C in the example above) based on the presence of a field in a parent class (D in the example above). Modeling this as a new symbol in the child class is correct, but this symbol should not have a declaration in this case. We may need to come up with some other way to provide semantic token types for fields with converters. Let me know if you have any ideas.

[debonte]

We may need to come up with some other way to provide semantic token types for fields with converters. Let me know if you have any ideas.

FWIW, semantic tokenization was just the symptom reported by the user. The missing declaration also prevented Go to Def from working on usages of the converted field. There may be other issues as well.

[erictraut]

The missing declaration also preventing Go to Def from working on usages of the converted field.

The problem is that there is no explicit declaration in the code for the converter descriptor object, so modeling this as a declaration is going to inevitably cause problems. The converter object is created by behind-the-scenes magic within attrs when a "converter" argument is passed to it, and it's recreated for every subclass. This is why the type evaluator code previously didn't provide a declaration in this case. I was nervous when you added the declaration because I suspected it would cause problems, but I failed to come up with any specific examples.

I can understand why your instinct was to add a declaration. It allows the language services modules do the expected thing in most cases, but it is a kludge and has other bad side effects elsewhere in the type evaluator.

In the example above, the internal declaration synthesized for C.x indicates that the declaration comes from module c, but the parser node associated with the same declaration is located in module d. That's clearly wrong, as the associated parse node should be within the module where the declaration is found.

Thinking through our options... Perhaps we should revert the change that synthesizes a pseudo-declaration and enhance the "synthesized type" mechanism in the Symbol class so it can track a "go to" node. This would give the logic in the definition provider enough information to take the user to the correct location when they choose "Go To Definition", and it would avoid confusing the rest of the type checker logic.

Any other suggestions?

[debonte]

That sounds reasonable. I don't have any other suggestions.

Btw, I based my implementation off of https://github.com/microsoft/pyright/blob/main/packages/pyright-internal/src/analyzer/namedTuples.ts#L221. And I see Symbol.addDeclaration calls in typedDicts.ts as well. So those may need to be fixed also.