Skip to content

fix[cartesian, eve, next]: latent bugs and deprecated typing shims surfaced by the Python 3.12 floor - #2755

Open
egparedes wants to merge 28 commits into
mainfrom
py312-fix-latent-bugs
Open

fix[cartesian, eve, next]: latent bugs and deprecated typing shims surfaced by the Python 3.12 floor#2755
egparedes wants to merge 28 commits into
mainfrom
py312-fix-latent-bugs

Conversation

@egparedes

@egparedes egparedes commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Description

Auditing the codebase after the floor moved to 3.12 (#2326) turned up several real
bugs, plus deprecated compatibility code, configuration and documentation that still
assumed an older floor. Merged up to date with main.

Important

@romancmain is currently broken by #2772, and this PR contains the fix
(still reproducible on main as of 0d8fb34d3).
That PR replaced the ellipsis check with isinstance(cn, types.EllipsisType), but
that is the type of the ... object, not of its AST node (an ast.Constant whose
value is Ellipsis), so the condition is never true. On current main:

field_b[...] = field_a
→ GTScriptSyntaxError: Unexpected type found <class 'ellipsis'>.
  Expected one of: int, AxisIndex, string (var ref), or None.

The merge here keeps this branch's _is_ellipsis_node helper, which matches
correctly on every supported version, and takes #2772's visit_Expr (better than
what this branch had: it drops only string constant statements, not every
constant). Nothing else from #2772 is reverted.

Bugs fixed

  • cartesian: '...' index detection was version-dependent. Originally
    getattr(ast, "Ellipsis", types.EllipsisType), which resolved to the deprecated
    ast.Ellipsis on 3.12/3.13 (a DeprecationWarning on every import) and, on 3.14,
    fell back to a type that never matches. Now matched via _is_ellipsis_node, reused
    by the vertical-interval parser. See the note above for the interaction with fix[cartesian]: remove usage of deprecated ast types #2772.

  • next: _type_conversion_helper rejected PEP 604 unions. A | B is a
    types.UnionType with no __origin__, so the __origin__ is Union branch never
    matched and control reached raise AssertionError("Illegal type encountered.");
    the builtin tuple spelling was missed the same way. Now dispatches on
    get_origin(). This also unblocks ruff's UP006 (relevant to Add more ruff rulesets #2224).

  • eve: frozen="strict" was completely broken. It called a non-existent
    xtyping.is_hashable_type, so every strict-frozen datamodel raised
    AttributeError at class definition — dead since Adapt functional branch to repository layout and configs in main #1146 (2023), unnoticed because
    the flag is used nowhere, had no tests, and the star-import plus module
    __getattr__ in extended_typing hides a missing name from both ruff and mypy.
    Renaming was not enough: the check now decomposes composite annotations, so
    wrapping a mutable type in a union, a container (tuple[list[int], ...]), or a
    TypeVar bound is no longer a way around it, and bare tuple/frozenset are
    rejected for the same reason as tuple[Any, ...]. PEP 695 aliases are resolved and
    checked through what they stand for, so type Pair = tuple[int, int] is accepted
    like the plain annotation while one hiding a mutable type is still rejected; an
    alias that cannot be evaluated proves nothing and is rejected rather than escaping
    as a raw NameError. The error names the offending fields and their datamodel.

  • next: the typing-export tests were not really parametrized.
    test_typing_exports runs on 3.12/3.13/3.14, but pytest-mypy-plugins passes no
    --config-file, so mypy walked up out of its temp directory and picked up the
    project [tool.mypy] table — pinning every run to the python_version floor and
    making the 3.13/3.14 runs duplicates of 3.12. The session now has its own
    typing_tests/mypy.ini, which sets no python_version and, since these tests
    model downstream client code, carries over only implicit_reexport (the setting
    that makes them check exports at all).

  • next: multiple compilation failures lost all but one traceback.
    wait_for_compilation flattened them into a single RuntimeError, leaving
    failures 2..n as repr text in the message. They are now raised as a group, so
    each keeps its own traceback. BaseExceptionGroup is the constructor, since
    Future.exception() is typed BaseException; it yields a plain ExceptionGroup
    whenever every member is an Exception, so except* on Exception still catches
    it. The multi-failure path had no test before.

Deprecated and dead code removed

  • eve: the PEP 585 typing aliases are no longer re-exported. Dict,
    FrozenSet, List, Set, Tuple and Type are deprecated spellings of the
    builtin generics on a 3.12 floor; the use sites move to dict, list, … Note they
    are not created by the re-export block — the typing / typing_extensions star
    imports bind them and the module __getattr__ forwards anything missing — so
    deleting the block alone would have left all six in place, silently rebound from
    the builtins to the deprecated typing objects. They are therefore dropped from
    the namespace and rejected explicitly, with a message naming the replacement. A
    bare typing.Type means type[Any], so unsubscripted occurrences become that
    rather than the stricter bare type.
    Forward-reference resolution is unchanged: these names stay valid in user-written
    annotations, so List[int] and typing.List[int] still resolve to list[int].
    The re-exports from collections.abc, collections, contextlib and re are
    deliberately kept — there the name is already the modern one, only its typing
    home is deprecated.

  • Dead version guards. An if sys.version_info >= (3, 9) block, an always-true
    isinstance(typing.Any, type) guard, and two branches for
    typing_extensions.Any being a distinct object from typing.Any — true only
    below 3.11, and typing_extensions re-exports typing.Any on 3.12, 3.13 and 3.14
    alike. In next/errors/exceptions.py, the add_note override and __notes__
    fold-in, both dead since PEP 678 landed in 3.11; DSLError now inherits
    add_note, and a test that ran on no supported version is deleted. Also fixes
    dir() on extended_typing, which listed only typing names and hid the
    module's own API.

Diagnostics

try* is named as a construct instead of falling back to a generic ast class name.
Constructs newer than the supported floor can now be catalogued too: naming
ast.TemplateStr (PEP 750 t-strings, 3.14) directly would break the import on
3.12/3.13, so they are registered by name and skipped where the interpreter lacks
them — a t-string now reports Unsupported Python syntax: t-string on 3.14.

Build and docs

.gitpod.Dockerfile still based the workspace on Python 3.11, so .gitpod.yml would
build a venv the project cannot be installed into. The noxfile's PEP 723 block
declared requires-python = ">=3.11". [tool.mypy] declared no python_version, so
the type-check floor followed whichever interpreter ran mypy. Drops a mypy override
and an isort known-third-party entry that reference things which do not exist.
docs/development/next/error-messages.md was advising contributors to write
needlessly compatible code and misdescribed where add_note breadcrumbs go.

Follow-ups

Filed rather than fixed here, to keep this PR scoped:

Requirements

  • All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the appropriate ADR inside the docs/development/ADRs/ folder. — n/a per review, no design decisions here

'ELLIPSIS_TYPE = getattr(ast, "Ellipsis", types.EllipsisType)' resolved
differently across supported Python versions. On 3.12/3.13 it returned the
deprecated 'ast.Ellipsis' (emitting a DeprecationWarning on every import); on
3.14, where that alias is removed, it fell back to 'types.EllipsisType', which
is the type of the '...' object rather than of its AST node, so the
'isinstance()' check would silently never match and '_eval_index' would stop
recognizing '...' indices.

Match 'ast.Constant' with an 'Ellipsis' value instead, which behaves the same
on every supported version.
'Union[A, B]' and 'A | B' are different runtime objects: the PEP 604 form
builds a 'types.UnionType', which carries no '__origin__' at all, so the
'__origin__ is Union' branch never matched it and control fell through to
'raise AssertionError("Illegal type encountered.")'.

The same applied to the builtin 'tuple' spelling, which 't is Tuple' missed.
Dispatch on 'get_origin()' instead, which normalizes both spellings, and
accept 'types.UnionType' alongside 'typing.Union' the way 'eve' already does.
The check called 'xtyping.is_hashable_type', which does not exist, so every
'frozen="strict"' datamodel raised 'AttributeError' at class definition. The
star-import plus module '__getattr__' in 'extended_typing' hid the missing
name from both ruff and mypy.

Use the represented types rather than the annotation object: a parametrized
generic alias such as 'List[int]' is itself hashable, so checking it directly
would have wrongly accepted a mutable 'list' field. An annotation which
represents no concrete type is now rejected as well, since strict immutability
cannot be established for it.
- '.gitpod.Dockerfile' still based the workspace on Python 3.11, which no
  longer satisfies 'requires-python', so '.gitpod.yml' would build a venv the
  project cannot be installed into.
- The noxfile's PEP 723 block declared 'requires-python = ">=3.11"' and its
  note referred to a '--python 3.11' shebang it does not have.
- '[tool.mypy]' declared no 'python_version', so the type-check floor followed
  whichever interpreter ran mypy instead of the lowest supported one.
- Dropped a mypy override for 'typing_tests.test_next_exports' and an isort
  'known-third-party' entry for 'importlib_resources'; neither exists (the only
  use is 'import importlib.resources as importlib_resources', from the stdlib).
'docs/development/next/error-messages.md' told contributors to import 'Self'
from 'gt4py.eve.extended_typing' rather than 'typing' and not to reference
'ast' nodes newer than 3.10 — advice that produces needlessly compatible code
now that the floor is 3.12. It now describes what is actually available, and
which shims are dead and on their way out.

Also fixes the language line in 'AGENTS.md' and three stale in-code remarks.
The Python floor is 3.12, so the version shims in the diagnostics code are
dead on every supported version:

- 'errors/exceptions.py': the 'sys.version_info < (3, 11)' 'add_note'
  override and the matching '__notes__' fold-in in '__str__'. 'DSLError'
  now inherits 'BaseException.add_note' (PEP 678), which is what already
  happened at runtime.
- 'errors/exceptions.py': 'Self' is imported from 'typing' directly
  instead of 'gt4py.eve.extended_typing'.
- 'ffront/dialect_parser.py': the deferred 'ast.TryStar' catalogue entry
  is now present, so 'try*' is named as a construct instead of falling
  back to the generic 'ast' class name.

Note that 'ast.TryStar' is not reachable through the frontend today: a
'try*' statement always names an exception type in its 'except*' clause,
and that name is rejected as an unsupported closure variable before the
AST is visited. The entry is added for consistency with 'ast.Try' and
pinned at the catalogue level; the reachability gap is pre-existing and
affects 'try'/'except X' too.

Also correct the recipe step 4 description of 'add_note', which claimed
the breadcrumb is routed into the structured 'notes' field. It goes to
'__notes__'; 'notes' is reserved for content authored at the raise site,
as 'test_add_note_uses_pep678_notes' has always pinned.
Extract the per-annotation immutability test into '_is_strictly_immutable_type'
and recurse into the types an annotation actually stands for, so wrapping a
field in a union no longer bypasses the check: 'Optional[PlainFrozenInner]'
and 'Optional[List[int]]' are now rejected the same way the bare annotations
are. 'Literal' arguments are values rather than types, so they are checked
via the type of each argument.

Also name the offending fields and their datamodel in the error message, and
document the 'frozen="strict"' value in the 'datamodel' docstring.
Replace the inline 'isinstance(node, ast.Constant) and node.value is Ellipsis'
test with the '_is_ellipsis_node' helper, and update the docstring that still
referred to the removed 'ast.Ellipsis' node.

Drop the 'ast.Str' fallback in 'CallInliner.visit_Expr': 'ast.Str' was removed
in Python 3.12, so the 'hasattr' guard is always false and 'ast.Constant'
alone covers string statements.
The union branch rebound the parameter name 't' in its comprehension and
assertion, making the assertion read as if it constrained the argument
rather than the converted members. Rename to 'member_types'/'m'.
@egparedes egparedes changed the title fix[cartesian, eve, next]: latent bugs surfaced by the Python 3.12 floor fix[cartesian, eve, next]: latent bugs and dead shims surfaced by the Python 3.12 floor Aug 6, 2026
…check

'_is_strictly_immutable_type' only inspected the origin of a parametrized
generic, so a field annotated 'Tuple[List[int], ...]' passed the check
('tuple' has a custom '__hash__') and produced instances that raise
'TypeError: unhashable type' on 'hash()' -- the exact invariant that
'frozen="strict"' exists to guarantee.

Decompose composite annotations instead: for a generic alias both the
origin and every type argument must be strictly immutable, and unions are
matched explicitly so that both 'Union[A, B]' and 'A | B' are covered.
An unresolvable forward reference now yields the regular 'EveTypeError'
naming the field rather than escaping as a raw 'NameError'.
The PEP 585 aliases sat inside 'if _sys.version_info >= (3, 9):' with no
'else' branch, which the 3.12 floor turns into a compile-time constant.

De-indenting them needs care: the block deliberately rebinds names that
the 'from typing import *' above it also defines, and the 'if' was
incidentally keeping the import sorter away from it. Without a barrier,
ruff hoists the block above the star import and silently undoes every
rebinding, so an explicit 'isort: split' marker takes over that role.
…ions

'test_typing_exports' is parametrized over 3.12/3.13/3.14, but its mypy
runs were all type-checking as 3.12. pytest-mypy-plugins creates its
execution directory under '--mypy-testing-base' and passes no
'--config-file', so mypy discovers the project's own '[tool.mypy]' table
by walking up out of that directory and picks up the 'python_version'
floor pinned there. The 3.13 and 3.14 runs were therefore duplicates of
the 3.12 one, and a client-visible typing regression specific to a newer
interpreter would not have been caught.

Point the session at a dedicated 'typing_tests/mypy.ini' instead. It
deliberately declares no 'python_version', so each run checks the version
it claims to. As these tests model *downstream client* code, the new file
also keeps gt4py-internal strictness from applying to client snippets,
while still enabling the mypy plugin that downstream users are told to
enable (and whose false-positive fixes these tests exist to pin).
Two known defects surfaced while reviewing this branch are left in place
but documented where they are, rather than fixed here.

The unsupported-construct catalogue entries for 'try'/'try*' are largely
unreachable: naming an exception type makes it a closure variable, and
'visit_FunctionDef' types closure variables before visiting the body, so
'try: ... except ValueError: ...' fails with "Unexpected object
'ValueError' ..." instead. Only 'try/finally' and bare 'try/except:'
reach the catalogue, and 'try*' never does. The existing test covers the
'try/finally' shape only, which is now stated so it is not mistaken for
general 'try' coverage. Fixing this means reordering the
unsupported-syntax scan ahead of closure-variable type deduction, which
changes user-visible messages and belongs in its own change.

'wait_for_compilation' still flattens multiple compilation failures into
a 'RuntimeError'. The 3.10 floor named in the original TODO is gone, so
the note now records the real remaining blocker: it would change the
documented 'Raises:' contract of a public function.

Also re-wraps a ragged docstring line in 'fingerprinting' and moves an
'isinstance' precondition above the action it guards.
'extended_typing' re-exported 'Dict', 'FrozenSet', 'List', 'Set', 'Tuple'
and 'Type'. Since PEP 585 these are deprecated spellings of the builtin
generics, so drop them and move the 87 use sites to 'dict', 'frozenset',
'list', 'set', 'tuple' and 'type'.

Removing the re-export is not enough on its own. The names are bound by
the 'typing' / 'typing_extensions' star imports at the top of the module,
and the module '__getattr__' forwards anything missing to those same
modules, so simply deleting the block would leave all six names in place,
silently rebound from the builtins to the deprecated 'typing' objects --
the opposite of the intent. They are therefore dropped from the namespace
explicitly and rejected in '__getattr__' with a message naming the
replacement. This is a runtime guarantee only: a type checker still
resolves the names through the star imports.

A bare 'typing.Type' means 'Type[Any]', but a bare builtin 'type' is
stricter (it is not indexable and carries no arbitrary attributes), so the
37 unsubscripted occurrences become 'type[Any]' to keep their meaning. The
other five aliases need no such care.

'eval_forward_ref' binds the name 'typing' to this module so that
annotations resolve through it, which would make 'typing.List[int]' in a
user-written forward reference hit the rejection above. That string names
the real 'typing' module and stays valid, so forward-ref resolution now
falls back to it for these six names; eve's type validation is tested
against exactly those annotations.

Only the builtin-generic aliases are affected. The re-exports from
'collections.abc', 'collections', 'contextlib' and 're' are unchanged:
there the name is already the modern one and only its 'typing' home is
deprecated. Direct 'from typing import List' imports elsewhere in the
repo are untouched.
Comment thread src/gt4py/eve/datamodels/core.py
Comment thread src/gt4py/eve/datamodels/core.py Outdated
Comment thread src/gt4py/eve/datamodels/core.py Outdated
Comment thread src/gt4py/eve/extended_typing.py
Comment thread src/gt4py/eve/extended_typing.py Outdated
Comment thread src/gt4py/eve/extended_typing.py Outdated
Comment thread src/gt4py/eve/extended_typing.py
Comment thread src/gt4py/eve/extended_typing.py
Comment thread typing_tests/mypy.ini
Comment thread src/gt4py/next/ffront/dialect_parser.py
…ables

Two annotations still slipped through the immutability check.

An unparametrized hashable container was accepted, so
'@datamodel(frozen="strict") class M: values: tuple' built instances whose
'hash()' raises 'TypeError: unhashable type'. Used bare, 'tuple' and
'frozenset' say nothing about the items whose hashes they fold in, which
is the same reason 'tuple[Any, ...]' is rejected -- the check now turns on
what the annotation proves, not on whether type arguments were spelled
out. An alias with no arguments ('typing.Tuple') is treated as its origin.

The 'get_represented_types' fallback flattened an annotation to its
origins and dropped the type arguments, so a mutable payload behind a
'TypeVar' bound survived: a bound of 'tuple[list[int], ...]' came back as
a plain, hashable 'tuple'. Type variables are now resolved through their
bound, constraints or default, and forward references through the
annotation itself, so both keep being decomposed. Handling forward
references here also widens the previous over-narrow 'except NameError':
a reference that resolves to something non-subscriptable, is malformed,
or resolves to another 'ForwardRef' is now rejected as the docstring
promises, rather than escaping as a raw exception.
…rences

Dropping 'Dict', 'List' and friends from 'extended_typing' also changed
how forward references resolve, which was not intended: these names stay
valid in user-written annotations even though gt4py no longer uses them.

Resolving a reference through this module has always normalized them to
the builtin generic. That broke in both directions: a bare 'List[int]'
raised 'NameError' (the names were popped from the module namespace that
serves as the default 'globalns'), and 'typing.List[int]' came back as
the deprecated 'typing._GenericAlias' object -- reintroducing exactly
what the module rejects elsewhere -- instead of 'list[int]'. Both paths
now yield the builtin again, matching the pre-removal behaviour on every
spelling, while an explicit 'globalns' is still left untouched.

Also fixes two unrelated defects in the same module: 'dir()' listed only
the 'typing' / 'typing_extensions' names and so hid the module's own API
('is_actual_type', 'eval_forward_ref', ...), and the 'isinstance(
_typing.Any, type)' guard around '_ArtefactTypes' is unconditionally true
on the 3.12 floor.
Giving 'test_typing_exports' its own config stopped it inheriting the
project's '[tool.mypy]' table, which was the point -- but that table also
supplied 'implicit_reexport = false', and that setting is what makes
these tests check *exports* at all: without it, a name that
'gt4py.next.__init__' merely imports instead of explicitly re-exporting
no longer produces "does not explicitly export attribute", and the
session goes green on exactly the regression it exists to catch.

Carry that one setting over. The remaining gt4py-internal strictness
stays out, since it would apply to the client snippets themselves.
Removing 'Dict', 'FrozenSet', 'List', 'Set', 'Tuple' and 'Type' from
'extended_typing' changes the shape of a public import surface, which
AGENTS.md asks to record in an ADR. Until now the rationale lived only in
a module comment and a commit message.

Records in particular the part that is not obvious from the diff: the
names are not created by the re-export block but by the 'typing' /
'typing_extensions' star imports above it, and the module '__getattr__'
forwards anything missing, so deleting the block alone would have left all
six in place silently rebound to the deprecated 'typing' objects. It also
draws the line against the second group of re-exports ('Sequence',
'Callable', 'Match', ...), whose names are already the modern ones and are
deliberately kept.

'gt4py.eve' had no ADR directory, so this adds one with the same index
README the 'next' and 'cartesian' subsystems use.

@egparedes egparedes left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Drop the whole eves ADR folder, it's not needed for this trivial change.

@egparedes

Copy link
Copy Markdown
Contributor Author

Dropped the docs/development/ADRs/eve/ folder in 4350c05 (revert of 7e7d399) — the ADR and its index README are both gone, so ADRs/ is back to cartesian + next.

For the record, the rationale it carried isn't lost: the non-obvious part (the six names come from the typing/typing_extensions star imports, not the re-export block, and __getattr__ forwards anything missing — so deleting the block alone would have silently rebound them to the deprecated typing objects) is in the module comment next to _DEPRECATED_TYPING_ALIASES and in the commit message of a86bfb9.

@egparedes egparedes changed the title fix[cartesian, eve, next]: latent bugs and dead shims surfaced by the Python 3.12 floor fix[cartesian, eve, next]: latent bugs and deprecated typing shims surfaced by the Python 3.12 floor Aug 10, 2026
Three files conflicted, all against #2772 and #2754.

'gtscript_frontend.py': #2772 removed the same deprecated 'ast' usage this
branch fixes, but replaced the ellipsis check with
'isinstance(cn, types.EllipsisType)', which never matches — that is the type
of the '...' object, not of its AST node ('ast.Constant' with an 'Ellipsis'
value). Kept this branch's '_is_ellipsis_node' helper, which does match, and
took #2772's better 'visit_Expr': it drops only *string* constant statements
rather than every constant, matching the documented intent.

The two cartesian test hunks were pure additions on both sides (the ellipsis
tests here, the IntEnum tests from #2323) and are both kept.

'test_diagnostic_messages.py': #2754 added PEP 695 alias tests; this branch
deleted 'test_add_note_folded_into_str_on_py310', whose
'skipif(version_info >= (3, 11))' means it runs on no supported version. Both
intents are preserved, which leaves 'import sys' unused — dropped.

#2754 also added test cases spelled with the deprecated 'typing' aliases this
branch removes ('List[SampleIntAlias]', 'type SampleGenericAlias[T] =
Tuple[T, T]', ...). Those are migrated to the builtin spelling, which git
could not do since the two changes touch different lines.
'_is_strictly_immutable_type' is a fourth annotation-dispatch funnel, added on
this branch after #2754 landed runtime PEP 695 support, so it did not learn to
resolve aliases. A 'type MyPair = tuple[int, int]' field was rejected while the
identical plain annotation was accepted — conservative, but it makes
'frozen="strict"' unusable with the alias feature that just landed, and an
alias hiding a mutable type was only rejected by accident.

Resolve the alias first and check what it stands for, so parametrized aliases
('PairAlias[int]') are decomposed like any other annotation. An alias whose
value cannot be evaluated — undefined name, recursive, not parametrizable —
proves nothing about immutability and is rejected like any other unresolved
annotation, rather than escaping as the raw 'NameError' / 'TypeError' from
'eval_type_alias'.

The new tests define their aliases at module level: this file uses PEP 563, so
a function-local alias would only ever be seen as an unresolvable forward
reference and the tests would pass for the wrong reason.
@egparedes

Copy link
Copy Markdown
Contributor Author

@romanc heads-up: main is currently broken by #2772, and the fix is in this PR.

That PR swapped the ellipsis check for isinstance(cn, types.EllipsisType), but that is the type of the ... object, not of its AST node — an ast.Constant whose value is Ellipsis — so the condition is never true on any version:

>>> node = ast.parse('a[...]').body[0].value.slice
>>> isinstance(node, types.EllipsisType)                       # #2772
False
>>> isinstance(node, ast.Constant) and node.value is Ellipsis  # this PR
True

Effect on current main — a stencil using an ellipsis index no longer parses:

field_b[...] = field_a
→ GTScriptSyntaxError: Unexpected type found <class 'ellipsis'>.
  Expected one of: int, AxisIndex, string (var ref), or None.

This branch already had _is_ellipsis_node for exactly this (it was the 3.14 half of the same bug: the old getattr(ast, 'Ellipsis', types.EllipsisType) fell back to the same non-matching type once ast.Ellipsis is removed). The merge in bac34fe keeps that helper and takes your visit_Expr — yours is better than what this branch had, since it drops only string constant statements rather than every constant. Nothing else from #2772 is reverted, and TestEllipsisNodeDetection pins both the helper and an end-to-end parse.

If you would rather not wait on this PR's review, the one-line fix is happy to go to main on its own and I will rebase.

@egparedes
egparedes requested review from havogt and romanc and a balanced review from Copilot August 13, 2026 13:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@romanc romanc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Thanks for the cleanup!

@romanc heads-up: main is currently broken by #2772, and the fix is in this PR.

oh oh ... thanks for catching and fixing this.

If you would rather not wait on this PR's review, the one-line fix is happy to go to main on its own and I will rebase.

I think it's fine to wait. I didn't even know that syntax existed and since there was no test coverage, I can only assume it's something old/odd. Seems redundant to me ... I mean why would you write field_b[...] = field_a when you can just write field_b = field_a instead?

I'll bring it to the team. Maybe we can just get rid of the syntax (in subsequent PRs) if nobody cares about it.

Comment thread src/gt4py/cartesian/frontend/gtscript_frontend.py Outdated
Comment thread docs/development/next/error-messages.md Outdated
Comment thread docs/development/next/error-messages.md Outdated
Comment thread docs/development/next/error-messages.md Outdated
Comment thread src/gt4py/eve/datamodels/core.py Outdated
_ITEM_HASHING_CONTAINER_TYPES: Final = (tuple, frozenset)


def _is_strictly_immutable_type(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this a bugfix for a general problem or exposed by Python 3.12+ features

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In case you didn't already, it might be worth to ask an agent for a targeted review on this function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On the origin: a general bug, nothing to do with 3.12. The function called xtyping.is_hashable_type, which has never existed, so every frozen="strict" datamodel raised AttributeError at class-definition time on every Python version. git log -S puts the broken call in b367725 (#1146, 2023-02-01), so it had been dead for ~3.5 years.

It survived that long because three things lined up: frozen="strict" is used nowhere in src/ (so nothing ever executed the path), tests/eve_tests had zero coverage of it before this PR, and the star-import plus module __getattr__ in extended_typing means a missing name is invisible to both ruff and mypy — xtyping.<anything> type-checks. That last one is also why the same module needed the explicit rejection list for the removed typing aliases: absence there is silent by construction.

So the 3.12 audit is what surfaced it, but nothing about 3.12 caused it.

On the agent review: yes, twice, and it was worth it — most of the hardening in this PR came out of it rather than from me. Between the two passes it found:

  • an unparametrized container (values: tuple) passing the check, so hash() still raised TypeError — the exact invariant the flag exists to guarantee;
  • the get_represented_types fallback flattening annotations to their origins, so a tuple[list[int], ...] TypeVar bound came back as a plain hashable tuple;
  • except NameError being too narrow around forward-reference resolution;
  • a Tuple[List[int], ...] field passing because only the origin was inspected;
  • forward references escaping as raw NameError instead of the documented EveTypeError.

All fixed, each with a test. Fair warning that the function has changed again since that last pass — ac1dd6c added PEP 695 alias resolution on top, after #2754 landed, because _is_strictly_immutable_type turned out to be a fourth annotation-dispatch funnel that did not know about aliases (a type Pair = tuple[int, int] field was rejected while the identical plain annotation was accepted). Happy to run another targeted pass over the current version if you want one before merging.

Comment thread src/gt4py/eve/extended_typing.py Outdated
Comment thread src/gt4py/eve/extended_typing.py Outdated
Comment thread src/gt4py/next/otf/compiled_program.py Outdated
Per review: the paragraph explaining why 'ast.Ellipsis' and 'types.EllipsisType'
are both unusable is context for this PR, not something a future reader of the
function needs. The PR and its history carry that; the docstring just says what
the function checks.
Review follow-up. 'wait_for_compilation' flattened several failures into one
'RuntimeError', which left failures 2..n as 'repr' text in the message and gave
only the first one a '__cause__' and a traceback. They are now raised as a
group, so each keeps its own.

'BaseExceptionGroup' is the constructor because 'Future.exception()' is typed
as 'BaseException'; it returns a plain 'ExceptionGroup' whenever every member is
an 'Exception', so 'except*' on 'Exception' still catches it. The 'Raises:'
contract is updated accordingly, and the multi-failure path — which nothing
covered before — gets a test.

Also registers 'ast' nodes newer than the supported floor by name
('_NEWER_UNSUPPORTED_FEATURE_HINTS'), so the catalogue can name them without
breaking the import on 3.12/3.13: PEP 750 t-strings now get a proper
"Unsupported Python syntax: t-string" on 3.14 instead of the generic 'ast'
class-name fallback.
Review follow-up. Two guards existed for the case where 'typing_extensions.Any'
is a distinct object from 'typing.Any', which was only true below the 3.11
floor -- as the second one's own comment said. 'typing_extensions' re-exports
'typing.Any' on every supported version (checked on 3.12, 3.13 and 3.14), so
'_ArtefactTypes' never grew a third entry and 'is_Any' always took its 'else'
branch. Both collapse to the branch that actually ran.

Also rewords the '__hash__' sentence in the 'frozen="strict"' docstring, which
stated the rule without saying why it holds: a type defining its own '__hash__'
is claiming its values hash by content, the mutable builtins opt out with
'__hash__ = None', and the inherited 'object.__hash__' hashes by identity and
says nothing about the value.
…t' nodes

Review follow-up. The 'add_note' paragraph opened with "This is the stock
'BaseException.add_note'", which read as a contrast with the 3.10 override this
PR removes, and so looked like leftover history. It is not: the '__notes__' vs
structured 'notes' split is current, version-independent behaviour --
'DSLError.__str__' emits only the structured parts and 'errors/excepthook.py'
appends '__notes__' itself. Reworded to state that directly.

Drops the "supported floor is 3.12" / "carries no version shims" paragraph as
meta-commentary that goes stale rather than guidance, and turns the remaining
caveat into what to actually do about post-floor 'ast' nodes.

@havogt havogt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants