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

Type invariance forbid method/function calls #1382

Open
barakatzir opened this issue Feb 4, 2025 · 2 comments
Open

Type invariance forbid method/function calls #1382

barakatzir opened this issue Feb 4, 2025 · 2 comments
Labels
bug Something isn't working

Comments

@barakatzir
Copy link
Contributor

Information

When writing a reproduction for #1381 I stumbled upon this.

  • rustworkx version: currently on main (commit f2f45425c70b49d89aa5527271a34f64fb114f1d)
  • Python version: 3.12
  • Rust version: ...
  • Operating system: ubuntu

What is the current behavior?

The method PyDiGraph.extend_from_edge_list restricts the input graph's to PyDiGraph[_S | None, _T | None]. In turn this causes static type checker to rightfully complain when the typevars are declared to but not None.
This is because PyDiGraph is defined to be invariant WRT the _T and _S typevars. This implies that PyDiGraph[int, int] is not assignable to PyDiGraph[int | None, int | None].

I tried searching for more function with | None and found that the methods (of both graph classes) extend_from_edge_list, extend_from_weighted_edge_list, add_edges_from_no_data, and the functions digraph_complement, graph_complement, complement, local_complement.
Probably suffer from a similar issue.

What is the expected behavior?

I expected to be able to run the reproduction below without mypy / pyright complaining about bad argument.

As annotation fixes:

  • for the methods extend_from_edge_list, extend_from_weighted_edge_list, add_edges_from_no_data, this is simply removing the | None.
  • For the universal complement and it two non-universal variant functions, if I understand the documentation correctly, the return type should have just None on the edge payload typvar, and not _T | None.
  • For local_complement, I think the type annotation is correct as is.

The annotations make me think that the graph classes are not intended to behave invariantly, but rather covariantly, if this is desirable the way to do it is:

# in rustworks.pyi

_S_co = TypeVar("_S_co", default=Any, covariant=True)
_T_co = TypeVar("_T_co", default=Any, covariant=True)

class PyGraph(Generic[_S_co, _T_co]):
    ...

class PyDiGraph(Generic[_S_co, _T_co]):
    ...

Personally, I think its better to keep them invariant, and if users of rustworkx want covariance / contravariance in their external functions they can define their signature as they like.

Steps to reproduce the problem

# t.py

import rustworkx as rx

graph = rx.PyDiGraph[int, int]()
graph.extend_from_edge_list([(0, 1), (2, 1), (3, 1), (1, 4)])

When run mypy t.py I get

t.py:5: error: Invalid self argument "PyDiGraph[int, int]" to attribute function "extend_from_edge_list" with type "Callable[[PyDiGraph[_S | None, _T | None], Iterable[tuple[int, int]]], None]"  [misc]
Found 1 error in 1 file (checked 1 source file)

and pyright t.py (in "basic" setting):

/home/barak-katzir/dev/rustworkx-check/t.py
  /home/barak-katzir/dev/rustworkx-check/t.py:6:7 - error: Cannot access attribute "extend_from_edge_list" for class "PyDiGraph[int, int]"
    Could not bind method "extend_from_edge_list" because "PyDiGraph[int, int]" is not assignable to parameter "self"
      "PyDiGraph[int, int]" is not assignable to "PyDiGraph[int | None, int | None]"
        Type parameter "_S@PyDiGraph" is invariant, but "int" is not the same as "int | None"
        Type parameter "_T@PyDiGraph" is invariant, but "int" is not the same as "int | None" (reportAttributeAccessIssue)
1 error, 0 warnings, 0 informations 
@barakatzir barakatzir added the bug Something isn't working label Feb 4, 2025
@IvanIsCoding
Copy link
Collaborator

We had this discussion in #401 and I think that adding Optional was the possibly wrong solution we came at the time.

The best solution would to have a specialization forbidding adding edges without data if _T cannot be None.

The second best solution in the meantime is to cast, we are not covariant on purpose. It is kind like Python lists. But casting to a covariant type is always right

@barakatzir
Copy link
Contributor Author

Interesting, now that you write it like that it seems that mypy and pyright did exactly that, and forbade adding None to int in the reproduction I gave.
If so, the only thing left of this issue is maybe to change the return type of complement, graph_complement and digraph_complement to have edge type of None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants