SPARQL results starting with RDFLib 6.3.0 induce new inlined type check needs with `mypy --strict`

[ajnelson-nist]

I just hit an unpleasant surprise on running unit tests on a project that worked yesterday, but failed today. The issue I hit was in using mypy --strict on a SPARQL CONSTRUCT query. A similar issue exists for SELECT queries, but I've resolved that one.

With RDFLib 6.2.0, this worked:

    for result in in_graph.query(construct_query):
        assert isinstance(result[0], URIRef)
        assert isinstance(result[1], URIRef)

Those asserts are a pattern I've had to implement to convince mypy --strict about types when I have later code assuming e.g. result[0] (and [1]) is a URIRef.

With 6.3.0 (released today), I get a complaint that I haven't guaranteed result is indexable. This is true for SELECT and CONSTRUCT queries.

error: Value of type "Union[Tuple[Node, Node, Node], bool, ResultRow]" is not indexable  [index]

I can get by with the following extra bits for SELECT queries:

#!/usr/bin/env/python

# ...
from rdflib.query import ResultRow
# ...
for result in graph.query(select_query):
    assert isinstance(result, ResultRow)

That pretty much followed from the mypy complaint I excerpted above, and checking for where ResultRow was defined. But, for CONSTRUCT queries, the Tuple syntax doesn't quite work the same.

#!/usr/bin/env/python

# ...
for result in graph.query(construct_query):
    assert isinstance(result, Tuple[URIRef, URIRef, URIRef])

mypy --strict throws a different error on that:

error: Argument 2 to "isinstance" has incompatible type "object"; expected "_ClassInfo"  [arg-type]

I don't see a unit test demonstrating how to get this to work. I also am not understanding how the return type of rdflib.graph.Graph.query() -> rdflib.query.Result eventually ties to that Union that covers SELECT, CONSTRUCT and ASK return types.

How should the results of CONSTRUCT queries interact with mypy --strict? Is there a type alias I should use instead of that Tuple? I thought there was an alias somewhere in term.py to cover a general triple, but I'm having trouble finding the symbol now.

[aucampia]

What is happening here is:

rdflib.graph.Graph.query returns a rdflib.query.Result:

rdflib/rdflib/graph.py

Lines 1509 to 1518 in 334787b

	def query(
	self,
	query_object: Union[str, Query],
	processor: Union[str, query.Processor] = "sparql",
	result: Union[str, Type[query.Result]] = "sparql",
	initNs: Optional[Mapping[str, Any]] = None, # noqa: N803
	initBindings: Optional[Mapping[str, Identifier]] = None,
	use_store_provided: bool = True,
	**kwargs: Any,
	) -> query.Result:

rdflib.query.Result's iter method returns a Union of Iterator[Union["_TripleType", bool, ResultRow]].

rdflib/rdflib/query.py

Lines 358 to 360 in 334787b

	def __iter__(
	self,
	) -> Iterator[Union["_TripleType", bool, ResultRow]]:

This Union exists because depending on the type of the query, the result will be different.

The problem is, there is not enough information available to the type checker to determine what kind of result is being returned, so users have to do this themselves.

If you know you passed a CONSTRUCT query, then I think either of the following should eliminate type errors:

for result in graph.query(...):
    assert isinstance(result, tuple)
    ...

for result in cast(Iterable[Tuple[URIRef, URIRef, URIRef]], graph.query(...)):
    ...

I think the reason why assert isinstance(result, Tuple[URIRef, URIRef, URIRef]) causes problems is that you should not use types (i.e. stuff from the typing module) with isinstance and issubclass:

From https://docs.python.org/3/library/typing.html#typing.TypeVar:

In general, isinstance() and issubclass() should not be used with types.

[aucampia]

One way to make this less annoying is to add an enum valued query_type parameter to Graph.query, and then use overload to annotate that the result will be different depending on the value of query_type passed in. That way you could do:

for triple in graph.query(..., query_type=QueryType.CONSTRUCT):
    ...

[ajnelson-nist]

Thank you @aucampia , your first post addressed all of my questions.

Your second post ... I get what the extra parameter would do, but I feel like that's breaking some obtrusiveness boundary of the type system. I'm not sure, just a gut response that will probably change on another day.

[ajnelson-nist]

I'm satisfied with this thread being closed, unless you think there's a graceful way to handle it.

My best guess would be supplanting the first argument to query with an optional exactly one of three keyword arguments, select_query: str, construct_query: str, ask_query: str, and maybe each of those could be union'd with a subclass of PreparedQuery.

[ajnelson-nist]

Eh, pardon:

s/graceful way to handle it/more graceful way to handle automatic type hinting/

s/best guess/reflexive guess/

[aucampia]

The graph.query(..., query_type=QueryType.CONSTRUCT) is not a super great solution, if we did something like that we would then do a check inside query() and raise a value error if the query is not of the requested type before executing, but still it is not nice, and I agree it does not feel right.

But definitely the current Graph.query() interface is very annoying with mypy.

I will close this issue, but we should think of ways to make the interface friendlier to use with type checking than it is now.

[ajnelson-nist]

I caught another typo in my split-out suggestion: I'd confused PreparedQuery with rdflib.plugins.sparql.processor.prepareQuery, and its return result rdflib.plugins.sparql.sparql.Query. Maybe that Query class could be subclassed with the various SPARQL query types, and that could be used for type-sensitive calls to rdflib.graph.Graph.query.

[aucampia]

Maybe that Query class could be subclassed with the various SPARQL query types, and that could be used for type-sensitive calls to rdflib.graph.Graph.query.

This would be a cleaner and better solution.

[ajnelson-nist]

I'm now fine with leaving this Issue open, to relate to a PR implementing that.

I can't start that PR today.

[aucampia]

I'm now fine with leaving this Issue open, to relate to a PR implementing that.

I can't start that PR today.

Sounds good, the title should maybe be adjusted, but the interface should definitely be improved and best to track it.