Skip to content

Add typing to sage_input methods #40897

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Add typing to sage_input methods #40897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62b5191
Add typing info for _sage_input_
tobiasdiez Sep 28, 2025
c76581b
Add future annotations import
tobiasdiez Oct 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/sage/geometry/cone.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,11 @@
# (at your option) any later version.
# https://www.gnu.org/licenses/
# ****************************************************************************
from __future__ import annotations

from collections.abc import Hashable, Iterable, Container
from copy import copy
from typing import TYPE_CHECKING, Literal
from warnings import warn

from sage.misc.lazy_import import lazy_import
Expand Down Expand Up @@ -241,6 +243,9 @@
lazy_import('ppl', ['ray', 'point'], as_=['PPL_ray', 'PPL_point'],
feature=PythonModule("ppl", spkg='pplpy', type='standard'))

if TYPE_CHECKING:
from sage.misc.sage_input import SageInputBuilder, SageInputExpression


def is_Cone(x):
r"""
Expand Down Expand Up @@ -1500,7 +1505,7 @@ def __init__(self, rays=None, lattice=None,
if PPL is not None:
self._PPL_C_Polyhedron = PPL

def _sage_input_(self, sib, coerced):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
"""
Return Sage command to reconstruct ``self``.

Expand Down
7 changes: 6 additions & 1 deletion src/sage/geometry/fan.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,9 +233,11 @@
# (at your option) any later version.
# https://www.gnu.org/licenses/
# ****************************************************************************
from __future__ import annotations

from collections.abc import Callable, Container
from copy import copy
from typing import TYPE_CHECKING, Literal
from warnings import warn

import sage.geometry.abc
Expand All @@ -262,6 +264,9 @@
from sage.rings.integer_ring import ZZ
from sage.rings.rational_field import QQ

if TYPE_CHECKING:
from sage.misc.sage_input import SageInputBuilder, SageInputExpression


def is_Fan(x) -> bool:
r"""
Expand Down Expand Up @@ -1217,7 +1222,7 @@ def __init__(self, cones, rays, lattice,
if virtual_rays is not None:
self._virtual_rays = PointCollection(virtual_rays, self.lattice())

def _sage_input_(self, sib, coerced):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you have Literal[2] here, where is this behaviour for _sage_input_ documented?

Also, Literal only works for Python literals. So Literal[2] means the int 2, not the Sage Integer 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's defined here:

sage/src/sage/misc/sage_input.py

Lines 56 to 66 in 899d400

A ``_sage_input_`` method takes two parameters, conventionally named
``sib`` and ``coerced``. The first argument is a
:class:`SageInputBuilder`; it has methods to build SIEs. The second
argument, ``coerced``, is a boolean. This is only useful if your class
is a subclass of :class:`Element` (although it is always present). If
``coerced`` is ``False``, then your method must generate an expression
which will evaluate to a value of the correct type with the correct
parent. If ``coerced`` is ``True``, then your method may generate an
expression of a type that has a canonical coercion to your type; and if
``coerced`` is 2, then your method may generate an expression of a type
that has a conversion to your type.

not the Sage Integer 2.

I don't think there is a good way to express this as a type.

Copy link
Member

Choose a reason for hiding this comment

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

not the Sage Integer 2.

I don't think there is a good way to express this as a type.

If Integer(2) is acceptable input (which I believe it is from some of the examples) then we can't use Literal[2] here. If only Integer(2) is acceptable then use Integer. If Integer(2) and int(2) are both acceptable then either Integer | int, or if you want to use Literal then Integer | Literal[2].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good, simple example of the sage vs python integer typing issue we were discussing in the other PR.

My impression is that python is not aiming to become strongly typed but that the purpose of these type annotations is to mainly help developers write better code. Applying this philosophy here, the typing for coerced should reveal at least these two 'bugs':

  1. Pass 1 or 3 (or another integer) as an argument to sage_input
  2. Assume in the implementation of sage_input that it's always a boolean

So in particular, if you have an integer and you would like to pass it as coerced, you would want to check that it's equal to 2 before going ahead, eg using:

Integer num = ... 
if num == 2:
    _sage_input(..., num)
else:
    raise "expected num to be 2, but got ..."

At this point the type checker is able to deduct that num is the literal 2 (although it's not, it's only equal to it)

I feel like supporting (and highlighting the need) for such a pattern is more important than being 100% right about the typing info. One can always add a pyright: ignore comment if the typing is to strict/wrong, but one is sure to have covered the case correctly. Of course, you don't want to force to many such comments - it would be stupid to artificially restrict the possible input to int if half of sage's code is passing in Integer and that works well.

Curious what you think about it.

Copy link
Member

@vincentmacri vincentmacri Oct 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This ended up being a longer response than I expected, I've been thinking about type annotations a lot lately.

This is a good, simple example of the sage vs python integer typing issue we were discussing in the other PR.

Just thinking out loud, definitely don't do this in this PR and don't do this without a sage-devel discussion, but is there anything preventing Integer from being a subclass of int? It would solve a lot of annoyances with Integer. I'm guessing it would be too slow for such an important Sage object.

My impression is that python is not aiming to become strongly typed but that the purpose of these type annotations is to mainly help developers write better code.

Interesting, I think about it differently. My impression is that type annotations are for linters/type checkers to check that typing rules are respected without having to enforce type checking at runtime. But using Literal for parameters I think is beginning to veer into the territory of validating the values of inputs rather than just their types. So to me the utility of type annotations is the ability to have most of the benefits of strongly typed code without the runtime overhead by running a linter/type checker on the code which enforces the rules strictly. The more we adhere to this approach the more useful the type checkers will become (and the more bugs they'll be able to find). Perhaps user code does weirder things that respect the spirit but not the letter of type annotations (for example, a function that accepts Integer as a parameter but a user passes a Rational with denominator 1). Users shouldn't have to worry about details like that, and the nice thing about types not being checked at runtime is that something like that will (usually) work fine. I don't think many Sage users are running mypy or pyright on their Sage scripts. On the other hand, Sage developers should be expected to write slightly more explicit code that doesn't rely on the coercion system handling incorrect types (if only because relying on the coercion system will introduce overhead that could be avoided by more explicit code).

A big reason I favour the stricter annotations approach is that in theory it could be used by Cython to generate more efficient code one day (Cython's ability to use Python annotation is currently a bit limited though, but if we have strict type annotations then as Cython improves here so will the speed of Sage).

Something to consider: when Literal is used for a parameter, arguments cannot be a variable. So this is invalid:

def foo(a: Literal[5]) -> int:
    return a + 1

b = 5
foo(b)  # invalid, b is not a literal

That's kind of awkward, so I don't think I like Literal for parameters. It's probably best used only as a return type annotation for functions that return a constant (so things like .is_finite for objects that are always/never finite).

Curious what you think about it.

I think that we should use the most specific type annotation that is correct (doesn't require pyright: ignore). Having to use pyright: ignore defeats the purpose of type checking the function, the only time I'd use it is if there is a bug in the type checker and we don't want to wait for the type checker to get fixed before merging the PR. If it's legitimately too hard to come up with a correct annotation then we shouldn't annotate it. Or, just annotate it with the most specific correct thing (even if that annotation is more permissive than the function in reality). I don't think an annotation that is slightly too permissive is an issue. We probably have plenty of functions in Sage that accept only a positive Integer and check at runtime if the Integer is non-positive and raise an error if so, but I don't think there is a need to have a more specific PositiveInteger type.

Literal doesn't support Integer, so if a function accepts Integer then we can't use Literal there. I was discussing Literal with @fchapoton in #40972 and he thinks we should probably avoid it for now, and this situation seem like a good example of why we might want to avoid Literal. Personally I don't think Literal needs to always be avoided, but the only situation where I think it is likely to be useful is for return annotations, not parameter annotations (in #40972 (comment) I give an example of a hypothetical optimization Cython could make using Literal, but the Cython compiler doesn't currently have that optimization so it's a somewhat moot point).

(@fchapoton curious what you think about using Literal only for return but not parameter annotations.)

So for this PR, my suggestion is to change the annotation to bool | Integer | int, as this is the most specific correct annotation that avoids the Literal weirdness (even bool | Integer | Literal[2] is weird as the foo example above shows). Setting to needs work for now.

If you want to do a bit more as a separate PR, there is also my suggestion from #40634 (comment) to have a types.py module where we can define common type aliases/unions that are used in Sage (like Integer | int).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot! I don't have too much time at the moment to properly respond, but please have a look at this playground where I played around with your example:

https://pyright-play.net/?code=GYJw9gtgBALgngBwJYDsDmUkQWEMoAySMApiAIYA2AsAFB0AmJwUwYYAFOQFyHFlUA2gFYAugEooAWgB8mFDG50oKqCBIwAriBRRyUANRQAjHToAjKAF4owum07nJUAMTyAblSQMANFEtIAM5QKGD4%2BpT8FDT0tCia0DamtA4c8RDiZrSopGi8OdZQAORFrlCBkBoAFqgYqJ6RvlBhVWQA7kEksK3yudY2wpjBKCTuZFCjVJrkpAx6wQhwIEhoVfjqAMZgaChIAF4kwTBVM5j4G%2BQoofgnCAgkKHTqY1QA%2BvD3HDkkaJJuOfZ2F8FD9MrEkCxvhgrAMlLRVGpRiQ3h8SMDcn8%2BKRoiJRMpVKkoWCgA

The foo(b) call is not marked as invalid in this example, as pyright is able to properly recognize that b is Literal[5]. Some other code like

num = 1
foo(num)

is properly marked as invalid.

(Note that the behavior under mypy might be different - in my experience pyright is usually the better option.)

"""
Return Sage command to reconstruct ``self``.

Expand Down
29 changes: 16 additions & 13 deletions src/sage/geometry/lattice_polytope.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,24 +120,26 @@
# (at your option) any later version.
# https://www.gnu.org/licenses/
# ****************************************************************************
from __future__ import annotations

import os
import shlex
from collections.abc import Hashable
from copyreg import constructor as copyreg_constructor
from functools import reduce
from io import IOBase, StringIO
from subprocess import Popen, PIPE
from warnings import warn
import os
import shlex
from subprocess import PIPE, Popen
from typing import TYPE_CHECKING, Literal

import sage.geometry.abc
from sage.arith.misc import GCD as gcd
from sage.features import PythonModule
from sage.features.palp import PalpExecutable
from sage.features.databases import DatabaseReflexivePolytopes
from sage.features.palp import PalpExecutable
from sage.geometry.cone import _ambient_space_point, integral_length
from sage.geometry.point_collection import (PointCollection,
read_palp_point_collection)
from sage.geometry.toric_lattice import ToricLattice, ToricLattice_generic
from sage.geometry.convex_set import ConvexSet_compact
from sage.geometry.point_collection import PointCollection, read_palp_point_collection
from sage.geometry.toric_lattice import ToricLattice, ToricLattice_generic
from sage.matrix.constructor import matrix
from sage.misc.cachefunc import cached_method
from sage.misc.flatten import flatten
Expand All @@ -149,11 +151,9 @@
from sage.rings.rational_field import QQ
from sage.sets.set import Set_generic
from sage.structure.element import Matrix
from sage.structure.richcmp import richcmp_method, richcmp
from sage.structure.richcmp import richcmp, richcmp_method
from sage.structure.sage_object import SageObject
from sage.structure.sequence import Sequence
import sage.geometry.abc


lazy_import("sage.combinat.posets.posets", 'FinitePoset')
lazy_import("sage.geometry.hasse_diagram", 'lattice_from_incidences')
Expand All @@ -167,6 +167,9 @@
lazy_import('ppl', 'point', as_='PPL_point',
feature=PythonModule("ppl", spkg='pplpy', type='standard'))

if TYPE_CHECKING:
from sage.misc.sage_input import SageInputBuilder, SageInputExpression


class SetOfAllLatticePolytopesClass(Set_generic):
def _repr_(self) -> str:
Expand Down Expand Up @@ -570,7 +573,7 @@ def __init__(self, points=None, compute_vertices=None,
self._ambient_facet_indices = tuple(ambient_facet_indices)
self._vertices = ambient.vertices(self._ambient_vertex_indices)

def _sage_input_(self, sib, coerced):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
"""
Return Sage command to reconstruct ``self``.

Expand Down Expand Up @@ -4443,7 +4446,7 @@ def _repr_(self):
pass
return result

def _sage_input_(self, sib, coerced):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool) -> SageInputExpression:
"""
Return Sage command to reconstruct ``self``.

Expand Down
4 changes: 3 additions & 1 deletion src/sage/geometry/point_collection.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ from sage.geometry.toric_lattice import ToricLattice
from sage.matrix.constructor import matrix
from sage.misc.latex import latex

from sage.misc.sage_input import SageInputBuilder, SageInputExpression


def is_PointCollection(x):
r"""
Expand Down Expand Up @@ -172,7 +174,7 @@ cdef class PointCollection(SageObject):
self._points = tuple(points)
self._module = self._points[0].parent() if module is None else module

def _sage_input_(self, sib, coerced):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
r"""
Return Sage command to reconstruct ``self``.

Expand Down
7 changes: 6 additions & 1 deletion src/sage/geometry/polyhedron/base0.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,17 @@
# (at your option) any later version.
# https://www.gnu.org/licenses/
# ****************************************************************************
from __future__ import annotations

from typing import TYPE_CHECKING, Literal
from sage.misc.cachefunc import cached_method
from sage.misc.abstract_method import abstract_method
from sage.structure.element import Element
import sage.geometry.abc

if TYPE_CHECKING:
from sage.misc.sage_input import SageInputBuilder, SageInputExpression


class Polyhedron_base0(Element, sage.geometry.abc.Polyhedron):
"""
Expand Down Expand Up @@ -296,7 +301,7 @@ def _delete(self):
"""
self.parent().recycle(self)

def _sage_input_(self, sib, coerced):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
"""
Return Sage command to reconstruct ``self``.

Expand Down
7 changes: 6 additions & 1 deletion src/sage/geometry/toric_lattice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@
#
# https://www.gnu.org/licenses/
# ****************************************************************************
from __future__ import annotations

from typing import TYPE_CHECKING, Literal
from sage.geometry.toric_lattice_element import ToricLatticeElement
from sage.misc.lazy_import import lazy_import
lazy_import('sage.geometry.toric_plotter', 'ToricPlotter')
Expand All @@ -162,6 +164,9 @@
from sage.rings.rational_field import QQ
from sage.structure.factory import UniqueFactory

if TYPE_CHECKING:
from sage.misc.sage_input import SageInputBuilder, SageInputExpression


def is_ToricLattice(x):
r"""
Expand Down Expand Up @@ -897,7 +902,7 @@ def __init__(self, rank, name, dual_name, latex_name, latex_dual_name):
self._latex_name = latex_name
self._latex_dual_name = latex_dual_name

def _sage_input_(self, sib, coerced):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
r"""
Return Sage command to reconstruct ``self``.

Expand Down
3 changes: 2 additions & 1 deletion src/sage/matrix/matrix1.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ from cpython.sequence cimport PySequence_Fast
import sage.modules.free_module
from sage.structure.coerce cimport coercion_model

from sage.misc.sage_input import SageInputBuilder, SageInputExpression

cdef class Matrix(Matrix0):
###################################################
Expand Down Expand Up @@ -622,7 +623,7 @@ cdef class Matrix(Matrix0):
matrix._sage_object = self
return matrix

def _sage_input_(self, sib, coerce):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
r"""
Produce an expression which will reproduce this value when evaluated.

Expand Down
38 changes: 20 additions & 18 deletions src/sage/misc/explain_pickle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,23 +152,26 @@
# (at your option) any later version.
# http://www.gnu.org/licenses/
# *****************************************************************************
from __future__ import annotations


import bz2 as comp_other
import pickletools
import re
import sys
import types

import zlib as comp
import bz2 as comp_other

from pickletools import genops

from sage.misc.sage_input import SageInputBuilder, SageInputExpression
from typing import Literal

from sage.misc.persist import (
SageUnpickler,
dumps,
register_unpickle_override,
unpickle_global,
unpickle_override,
)
from sage.misc.sage_eval import sage_eval
from sage.misc.persist import (unpickle_override, unpickle_global, dumps,
register_unpickle_override, SageUnpickler)

from sage.misc.sage_input import SageInputBuilder, SageInputExpression

# Python 3 does not have a "ClassType". Instead, we ensure that
# isinstance(foo, ClassType) will always return False.
Expand Down Expand Up @@ -358,7 +361,7 @@ def __init__(self, value, expression):
self.expression = expression
self.immutable = False

def _sage_input_(self, sib, coerced):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
r"""
Extracts the expression from a PickleObject, and sets the immutable
flag.
Expand Down Expand Up @@ -845,14 +848,13 @@ def _APPENDS_helper(self, lst, slice):
else:
for s in slice:
self.sib.command(lst, self.sib.name('list').append(lst, self.sib(s)))
elif self.pedantic:
app = self.sib(lst).append
for s in slice:
self.sib.command(lst, app(self.sib(s)))
else:
if self.pedantic:
app = self.sib(lst).append
for s in slice:
self.sib.command(lst, app(self.sib(s)))
else:
for s in slice:
self.sib.command(lst, self.sib(lst).append(self.sib(s)))
for s in slice:
self.sib.command(lst, self.sib(lst).append(self.sib(s)))
else:
self.sib.command(lst, self.sib.name('unpickle_appends')(self.sib(lst), slice_exp))
self.push(lst)
Expand Down Expand Up @@ -2509,7 +2511,7 @@ def unpickle_extension(code):
<class 'sage.misc.explain_pickle.EmptyNewstyleClass'>
sage: remove_extension('sage.misc.explain_pickle', 'EmptyNewstyleClass', 42)
"""
from copyreg import _inverted_registry, _extension_cache
from copyreg import _extension_cache, _inverted_registry
# copied from .get_extension() in pickle.py
nil = []
obj = _extension_cache.get(code, nil)
Expand Down
23 changes: 12 additions & 11 deletions src/sage/misc/sage_input.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,9 @@
# (at your option) any later version.
# https://www.gnu.org/licenses/
# ****************************************************************************
from __future__ import annotations

from typing import Literal

from sage.misc.lazy_import import lazy_import

Expand Down Expand Up @@ -343,7 +346,7 @@ def __init__(self, allow_locals=False, preparse=True):
self._next_local = 1
self._locals = {}

def __call__(self, x, coerced=False):
def __call__(self, x, coerced: bool | Literal[2] = False):
r"""
Try to convert an arbitrary value ``x`` into a
:class:`SageInputExpression` (an SIE).
Expand Down Expand Up @@ -477,11 +480,10 @@ def __call__(self, x, coerced=False):
return SIE_literal_stringrep(self, str(x) + 'r')
elif self._preparse is False:
return self.int(x)
elif x < 0:
return -self.name('int')(self.int(-x))
else:
if x < 0:
return -self.name('int')(self.int(-x))
else:
return self.name('int')(self.int(x))
return self.name('int')(self.int(x))

if isinstance(x, float):
# floats could often have prettier output,
Expand All @@ -499,8 +501,8 @@ def __call__(self, x, coerced=False):
return -SIE_literal_stringrep(self, str(-x))
else:
return SIE_literal_stringrep(self, str(x))
from sage.rings.real_mpfr import RR
from sage.rings.integer_ring import ZZ
from sage.rings.real_mpfr import RR
rrx = RR(x)
if rrx in ZZ and abs(rrx) < (1 << 53):
return self.name('float')(self.int(ZZ(rrx)))
Expand Down Expand Up @@ -593,7 +595,7 @@ def float_str(self, n):
"""
return SIE_literal_stringrep(self, n)

def name(self, n):
def name(self, n) -> SageInputExpression:
r"""
Given a string representing a Python name,
produces a :class:`SageInputExpression` for that name.
Expand Down Expand Up @@ -2228,11 +2230,10 @@ def _sie_format(self, sif):
values = [sif.format(val, 0) for val in self._sie_values]
if self._sie_is_list:
return '[%s]' % ', '.join(values), _prec_atomic
elif len(values) == 1:
return '(%s,)' % values[0], _prec_atomic
else:
if len(values) == 1:
return '(%s,)' % values[0], _prec_atomic
else:
return '(%s)' % ', '.join(values), _prec_atomic
return '(%s)' % ', '.join(values), _prec_atomic


class SIE_dict(SageInputExpression):
Expand Down
3 changes: 2 additions & 1 deletion src/sage/modules/free_module_element.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ from sage.rings.abc import RealDoubleField, ComplexDoubleField

from sage.rings.integer cimport Integer, smallInteger
from sage.arith.numerical_approx cimport digits_to_bits
from sage.misc.sage_input import SageInputBuilder, SageInputExpression

# For the norm function, we cache Sage integers 1 and 2
__one__ = smallInteger(1)
Expand Down Expand Up @@ -1258,7 +1259,7 @@ cdef class FreeModuleElement(Vector): # abstract base class
return self
return self.change_ring(R)

def _sage_input_(self, sib, coerce):
def _sage_input_(self, sib: SageInputBuilder, coerced: bool | Literal[2]) -> SageInputExpression:
r"""
Produce an expression which will reproduce this value when evaluated.

Expand Down
Loading
Loading