[ty] print diagnostics with fully qualified name to disambiguate some cases

[leandrobbraga]

There are some situations that we have a confusing diagnostics due to identical class names.

Class with same name from different modules

import pandas
import polars

df: pandas.DataFrame = polars.DataFrame()

This yields the following error:

Actual:
error: [invalid-assignment] "Object of type DataFrame is not assignable to DataFrame"
Expected:
error: [invalid-assignment] "Object of type polars.DataFrame is not assignable to pandas.DataFrame"

Nested classes

from enum import Enum

class A:
    class B(Enum):
        ACTIVE = "active"
        INACTIVE = "inactive"

class C:
    class B(Enum):
        ACTIVE = "active"
        INACTIVE = "inactive"

Actual:
error: [invalid-assignment] "Object of type Literal[B.ACTIVE] is not assignable to B"
Expected:
error: [invalid-assignment] "Object of type Literal[my_module.C.B.ACTIVE] is not assignable to my_module.A.B"

Solution

In this MR we added an heuristics to detect when to use a fully qualified name:

• There is an invalid assignment and;
• They are two different classes and;
• They have the same name

The fully qualified name always includes:

• module name
• nested classes name
• actual class name

There was no QualifiedDisplay so I had to implement it from scratch. I'm very new to the codebase, so I might have done things inefficiently, so I appreciate feedback.

Should we pre-compute the fully qualified name or do it on demand?

Not implemented

Function-local classes

Should we approach this in a different PR?

Example:

# t.py
from __future__ import annotations


def function() -> A:
    class A:
        pass

    return A()


class A:
    pass


a: A = function()

mypy

t.py:8: error: Incompatible return value type (got "t.A@5", expected "t.A")  [return-value]

From my testing the 5 in A@5 comes from the like number.

ty

error[invalid-return-type]: Return type does not match returned value
 --> t.py:4:19
  |
4 | def function() -> A:
  |                   - Expected `A` because of return type
5 |     class A:
6 |         pass
7 |
8 |     return A()
  |            ^^^ expected `A`, found `A`
  |
info: rule `invalid-return-type` is enabled by default

Fixes astral-sh/ty#848

[github-actions]

Diagnostic diff on typing conformance tests

No changes detected when running ty on typing conformance tests ✅

[github-actions]

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

[leandrobbraga]

It seems there are some false positives, I'll work on those later.

EDIT: it was an important check that I accidentally deleted while refactoring

[AlexWaygood]

Thank you! A few quick things I noticed:

[AlexWaygood]

I'd also add branches for GenericAlias, ProtocolInstance, SubclassOf and TypedDictType here

[leandrobbraga]

Was this that you had in mind?

[leandrobbraga]

I had to rebase to solve conflicts, but I think I addressed all the suggestions.

I tried doing some testing to the <locals of function 'f'> but I couldn't come up with a working test for this situation.

[carljm]

Thank you for working on this!

Sorry for coming in on the late side with a new suggestion, but I think we should take a slightly different approach in the display.rs part of the implementation. Rather than introducing a totally separate QualifiedDisplayType struct, I think we should have a qualified: bool attribute on the existing DisplayType and `DisplayRepresentation structs, which we can respect whenever we are emitting the name of a class or function. (The current PR doesn't seem to handle functions, and it doesn't need to be done in this PR, but at some point I think we'll want to do this for function names, too.) I think this will make it much easier to share parts of the implementation that are not related to whether or not names are qualified, without code duplication.

The qualified flag could be set using a builder pattern, like ty.display(db).qualified() vs just ty.display(db) for the un-qualified display of a type. Though we may also want some form that allows easily passing a boolean value; this will be convenient for when we want to pass along the "qualified" value to display of a nested inner type.

The recently-merged implementation of a "multiline" display option (which probably needs conflict resolution with this PR anyway) is a good model to follow here.

[carljm]

Looking good! I think we should increase our test coverage a bit, and try to reduce code boilerplate.

I think ultimately in order to support nested types we will probably need to change the settings from just having a qualified boolean, to instead having a vector of types whose names should be qualified, so that we can identify the right types to qualify even in nested display. But this can be a separate PR.

[carljm]

I went ahead and made those updates, this looks good to go!