Skip to content

feat[next]: Add support for tuple comprehensions - #2833

Open
SF-N wants to merge 5 commits into
fix-concat-where-local-fieldsfrom
sf_n_tracer_support
Open

feat[next]: Add support for tuple comprehensions#2833
SF-N wants to merge 5 commits into
fix-concat-where-local-fieldsfrom
sf_n_tracer_support

Conversation

@SF-N

@SF-N SF-N commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Adds support for tuple comprehensions, e.g. for usage on tracers.

@gtx.field_operator
def testee(
    tracers: tuple[cases.IField, cases.IField], factor: int32
) -> tuple[cases.IField, cases.IField]:
    return tuple(tracer * factor for tracer in tracers)

TODO:

  • document that precompilation is not possible yet and why.

AI disclaimer: This code was to a small degree refactored using AI tools. Code has been reviewed in detail, tests only briefly.

Comment on lines +461 to +464
case ts.VarArgType(element_type=element_type):
new_type = (
element_type # TODO: we only temporarily allow any index for vararg types
)

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.

Comment from @tehrengruber in #2487:
This is for direct access to tracers[0] * factor, tracers[1] * factor, which I personally think is an anti pattern. I left it here until we take a decision on this. We could also make it an optional feature. One of the disadvantages is that it is not possible to fully type check the field operator at definition time, since the tuple length is only known at call / compile time. The user will then get an error in unroll_map_tuple.

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.

Removed from this PR and put into #2838

Comment thread src/gt4py/next/ffront/foast_to_gtir.py Outdated
Comment thread src/gt4py/next/ffront/foast_to_gtir.py Outdated
Comment thread src/gt4py/next/ffront/foast_to_gtir.py Outdated
Comment thread src/gt4py/next/ffront/func_to_foast.py Outdated

def visit_Name(self, node: ast.Name, **kwargs: Any) -> foast.Name:
return foast.Name(id=node.id, location=self.get_location(node))
def visit_Name(self, node: ast.Name, **kwargs: Any) -> foast.DataSymbol | foast.Name:

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.

@tehrengruber-ai What is this change about?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It made 'visit_Name' produce a 'DataSymbol' for names in 'ast.Store' context, which the comprehension-target parsing ('parse_target') relied on when visiting the loop target; everything else only ever visits names in 'Load' context. Since 'parse_target' was the only user, 374327f (pending push) builds the 'DataSymbol' directly there and reverts 'visit_Name' to main's version.

Comment thread src/gt4py/next/ffront/foast_to_gtir.py Outdated
iterable_element: itir.Expr | str,
) -> itir.Expr:
"""
Wrap `element_expr` in a `let` binding the comprehension target to `iterable_element`.

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.

Suggested change
Wrap `element_expr` in a `let` binding the comprehension target to `iterable_element`.
Wrap `element_expr` in a `let` binding the `comprehension_target` to `iterable_element`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Applied in 410f729a5 (pending push).

Comment thread src/gt4py/next/ffront/foast_to_gtir.py Outdated
Comment on lines +276 to +277

returns `let a = iterable_element[0], b = iterable_element[1] in element_expr`.

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.

Suggested change
returns `let a = iterable_element[0], b = iterable_element[1] in element_expr`.
returns
`let a = iterable_element[0], b = iterable_element[1] in element_expr`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Applied in 410f729a5 (pending push).

tuple_types = [from_type_hint_same_ns(arg) for arg in args]
assert all(isinstance(elem, ts.DataType) for elem in tuple_types)
return ts.TupleType(types=tuple_types)
if (

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.

@tehrengruber-ai Add a comment with short annotation examples for all the 3 cases here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Done in 410f729a5 (pending push): one short example comment per branch — fixed-length 'tuple[int32, float64]', variable-length 'tuple[int32, ...]', and unparametrized 'tuple' / 'tuple[()]'.

case common.Field:
if (n_args := len(args)) != 2:
raise ValueError(f"Field type requires two arguments, got {n_args}: '{type_hint}'.")
if args is None or len(args) != 2:

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.

@tehrengruber-ai What is this about?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It adapts the 'Field' case to the change above in 'canonicalize_type_hint', which now returns 'args=None' for unsubscripted annotations (needed to distinguish 'tuple' from 'tuple[()]'). A bare 'Field' annotation thus arrives here with 'args=None', and main's 'len(args)' would raise a 'TypeError'; the guard preserves the original diagnostic (verified: bare 'Field' still raises ValueError "Field type requires two arguments, got 0"). Added a short comment saying so in 410f729a5 (pending push).

tehrengruber added a commit that referenced this pull request Aug 27, 2026
Allow indexing 'tuple[Field, ...]' parameters directly, e.g.
'tracers[0] * factor': the subscript type deduction accepts any literal
index on a 'ts.VarArgType' and yields the element type. Extracted from
#2833 pending a decision; likely to be skipped, since the field operator
cannot be fully type checked at definition time (the tuple length is only
known at call/compile time, so an out-of-range index only surfaces late,
in 'unroll_map_tuple').
@tehrengruber
tehrengruber changed the base branch from main to fix-concat-where-local-fields August 27, 2026 20:20

@tehrengruber-ai tehrengruber-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.

Automated review by Fable (posted on behalf of @tehrengruber). Six findings, each verified empirically by comparing pre-PR vs. post-PR behavior. No issues found in the core comprehension typing/lowering itself; everything clusters around the variadic-tuple plumbing at the edges (out= validation, builtin-constructor arity, the DeferredType stand-in for bare tuple, dispatch, and test-harness coverage).

if operator_return_type != new_kwargs["out"].type:
# Not equality: variadic comprehensions give a `VarArg[...]` return type,
# while 'out' is a concrete tuple.
if not type_info.is_compatible_type(operator_return_type, new_kwargs["out"].type):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[correctness, verified] Loosening this from equality to is_compatible_type/is_concretizable for all operators silently drops tuple-length validation for VarArg-returning operators: the return type is never specialized to the call-site length, and is_concretizable(VarArg[T], Tuple) ignores length (it even accepts an empty tuple, see type_info.py:555).

Verified: calling a tuple[IField, ...] -> tuple[IField, ...] operator with a 3-element input and a 2-element out passes PAST type deduction; embedded execution then dies with a bare AssertionError in utils.tree_map (utils.py:211), roundtrip with the opaque 'target_domain' cannot be 'NEVER' error. Pre-PR the equality check gave a located "Expected keyword argument out to be of type ..." error.

Suggestion: specialize the VarArg return type against the concrete argument length (available in with_args) instead of weakening the check for every operator. Note foast_to_past.py:121 uses a different predicate (is_concretizable) for the same conceptual check.

@tehrengruber tehrengruber Aug 27, 2026

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.

This is a complicated issue that requires in-depth investigation. One solution would be integer type parameters that are resolved later with a proper frontend message or moving type checking to GTIR completely with a way to translate source locations. Skipped for now.
@tehrengruber-ai Write a test that shows this. Maybe pass two var args tuples of different length and conditionally return either one of them.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test added in ed23528 (pending push): 'test_var_len_tuple_length_mismatch_rejected' in test_tuples.py uses your construction — 'a: tuple[IField, ...]' of length 2, 'b' of length 3, 'return a if c else b', 'out' of length 2 — so no single length can be consistent with 'out'. It asserts the desired located 'DSLError' and is marked xfail(strict=True) documenting the gap: today every backend dies with an internal error instead (embedded and roundtrip with a bare 'AssertionError' from 'utils.tree_map', gtfn with a bare 'AssertionError' at compile). Verified XFAIL on all 7 runnable matrix variants, no XPASS.

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.

Related but separate half, in my review body above: _is_generic does not recognise VarArgType, so tuple length never enters the compiled-program cache key either. #2834 fixes that predicate, but compile() never passes arg_specialization_info, so the fixed predicate makes variadic-tuple programs un-AOT-compilable. Not covered by the xfail test here.

f"'{self._func_name(node)}()' only takes literal arguments.",
)
assert isinstance(node.func, ast.Name)
(arg,) = node.args

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[correctness, verified] The new (arg,) = node.args turns wrong-arity builtin constructor calls into raw unpacking errors, regressing the located diagnostic: pre-PR, int32() or int32(1, 2) in a field operator raised a located DSLError: Invalid argument types in call to 'int32'; post-PR both crash with a bare ValueError: not enough/too many values to unpack and no source location. The old if len(node.args) > 0 tolerated non-1 arity and let type deduction report it properly.

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.

@tehrengruber-ai Fix this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in ed23528 (pending push): wrong arity now raises a located DSLError ("'int32()' takes exactly one argument, got 2.") before the unpack, pinned by 'test_scalar_cast_wrong_arity' for both the 0- and 2-argument case. Same wording as the arity check on the element-wise branch, so the two merge cleanly.

)
)

case ts.DeferredType(constraint=ts.TupleType):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[correctness, verified] This constructor case returns DeferredType(constraint=VarArgType), but no call path can ever concretize it: the genexp path bypasses it in the parser, and the still-permitted literal path trips an internal assert. Verified: tuple(1.0) inside a field operator (accepted by _verify_builtin_type_constructor, whose own error message offers "literal arguments or a generator expression") fails with AssertionError: 'FOAST' expression is not fully typed.

Either reject tuple(<literal>) in the parser alongside other non-genexp forms, or make the constructor produce a concrete type.

@tehrengruber tehrengruber Aug 27, 2026

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.

@tehrengruber-ai What is this branch for then?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Traced it: this branch types the 'tuple' builtin symbol itself, not user annotations. 'tuple' is in 'fbuiltins.PYTHON_TYPE_BUILTINS', so when a field operator references the name outside the intercepted genexp form, the symbol is typed via 'from_value(tuple)' -> 'type[tuple]' -> 'make_constructor_type', which lands here; without the branch 'make_constructor_type' falls through to its closing ValueError and parsing crashes while typing the symbol. The genexp form never needs it — verified: a parsed 'tuple(genexp)' operator has no 'tuple' closure symbol at all, since the parser intercepts the call before name resolution. The DeferredType return value is only ever consumed on the 'tuple()' path, which then dead-ends in the not-fully-typed assert — that path should be rejected in '_verify_builtin_type_constructor' so the return type is never consulted.

elif args is None or (isinstance(args, tuple) and len(args) == 0):
# TODO(tehrengruber): We use `DeferredType` until we have an actual representation
# for a generic type.
return ts.DeferredType(constraint=ts.TupleType)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[correctness, verified] A bare tuple annotation now maps silently to this DeferredType stand-in instead of being rejected here, so the rejection surfaces downstream as an internal-repr message: a parameter annotated bare tuple fails at decoration with InvalidParameterAnnotationError: ... 'DeferredType(constraint=<class ...TupleType>)' (raw datamodel repr shown to the user), where pre-PR it was the clear ValueError: Tuple annotation 'tuple' requires at least one argument. Any consumer of from_type_hint not guarding against DeferredType inherits the same leak.

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.

@tehrengruber-ai What is this branch for then?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same trace from the 'from_type_hint' side: 'from_value(tuple)' goes through the 'type[Foo]' case ('type_translation.py:300'), which calls 'from_type_hint(tuple)' for the constructed type — so bare 'tuple' must map to something, and this branch supplies the 'DeferredType(constraint=TupleType)' stand-in (per the TODO, until a real generic-tuple type exists). The side effect is that bare 'tuple' annotations share the code path, which is why they now surface downstream as the 'InvalidParameterAnnotationError' with a raw DeferredType repr instead of main's clear "requires at least one argument" ValueError. If wanted, the annotation entry point could reject DeferredType with a proper message while keeping this branch for the builtin-symbol use.

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.

Adjacent: the comment at line 191 still says the None/() split exists to distinguish tuple from tuple[()], but this branch maps both to the same DeferredType. Commented there.

if isinstance(node.func, ast.Name):
func_name = self._func_name(node)

if (

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[design, verified] Comprehension recognition is ~50 lines inlined in visit_Call, keyed purely on the syntactic name tuple, ahead of and outside the dialect's builtin-constructor dispatch (TYPE_BUILTIN_NAMES below), with no symbol resolution. Aliasing (t = tuple at module scope, then t(x * f for x in tracers)) falls through to the generic UnsupportedPythonFeatureError whose hint says generator expressions "are only supported as the argument of tuple(...)" - exactly what the user semantically wrote; conversely a closure variable that merely happens to be named tuple is rerouted into comprehension parsing. Extracting a _visit_tuple_comprehension (or using the builtin-constructor table) and resolving the name against closure vars would make the dispatch robust.

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.

This is a general problem that we want to solve, out-of-scope in this PR. One idea is to introduce a "type of types" like type[int] and use this to propagate type information.

_allocate_from_type(
case=case, arg_type=t, domain=domain, dtype=dtype, strategy=strategy
)
for t in [element_type] * 3 # TODO: revisit

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[test-coverage, verified] The harness hardcodes exactly 3 elements for every VarArgType allocation (and again in get_param_size, both marked TODO), so no integration test can exercise any other variadic length. That makes length-dependent bugs structurally invisible - including the missing out-length check flagged in past_passes/type_deduction.py on this PR. Suggestion: make it an explicit, overridable harness parameter (e.g. a per-case default_varargs_length).

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.

The user can always allocate a tuple of the size he likes so this doesn't feel like an issue.

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.

Let's add a comment and then remove or add a message to the todo.

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.

And extract the magic number into a named object.

@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.

Review of the current head (ed23528a). Everything below was checked by running against the branch unless I say otherwise.

Two findings have no anchor in the diff, because the files they concern are not touched by this PR. They are the two most important ones, so they go here.


1. Module-level names are unreachable from inside a comprehension body

This is a functional blocker for the feature, not a rough edge.

@gtx.field_operator
def testee(tracers: tuple[EField, ...]) -> tuple[CField, ...]:
    return tuple(neighbor_sum(t(C2E), axis=C2EDim) for t in tracers)
# UndefinedSymbolError: Undeclared symbol 'neighbor_sum'

neighbor_sum is imported at module scope and works one line earlier outside a comprehension.

Cause. A generator expression compiles to its own code object, so names referenced only inside it land in that object's co_names, never in the enclosing function's. ffront/source_utils.py:24 get_closure_vars_from_function uses inspect.getclosurevars, which inspects only the enclosing function:

outer co_names            : ('tuple',)
nested <genexpr> co_names : ('neighbor_sum', 'C2E', 'C2EDim')
collected closure vars    : ['tuple']

Scope. Every module-level name: gt4py builtins (neighbor_sum, where, astype, ...), every FieldOffset, every local Dimension, and module-level @field_operators — so the clean factoring tuple(_inner(t, ...) for t in tracers) does not work either. Free variables are unaffected, because the enclosing code object carries co_freevars for the nested one.

Why CI is green. Every comprehension test in test_tuples.py defines its helpers inside the test function, so they are free variables. test_tuple_comprehension_other_fo passes for that reason alone. The entire suite sits on the working side of the bug.

Two things make this worse than a plain limitation: the diagnostic names a symbol that is plainly imported, and the workaround is to also reference the name outside the comprehension — so whether a stencil compiles depends on an unrelated line elsewhere in the function.

Prototype fix. Offered as a starting point, not a finished patch — it has not been reviewed by anyone but me, and the notes below are things I would want settled before it landed:

def _iter_code_objects(code: types.CodeType) -> Iterator[types.CodeType]:
    yield code
    for const in code.co_consts:
        if isinstance(const, types.CodeType):
            yield from _iter_code_objects(const)


def _global_vars_from_nested_code(function: Callable) -> tuple[dict[str, Any], dict[str, Any]]:
    global_ns = function.__globals__
    builtin_ns = global_ns.get("__builtins__", builtins.__dict__)
    if inspect.ismodule(builtin_ns):
        builtin_ns = builtin_ns.__dict__

    global_vars: dict[str, Any] = {}
    builtin_vars: dict[str, Any] = {}
    for nested_code in _iter_code_objects(function.__code__):
        if nested_code is function.__code__:
            continue
        for name in nested_code.co_names:
            if name in global_ns:
                global_vars[name] = global_ns[name]
            elif name in builtin_ns:
                builtin_vars[name] = builtin_ns[name]
    return global_vars, builtin_vars


def get_closure_vars_from_function(function: Callable) -> dict[str, Any]:
    (nonlocals, globals, builtins_, _unbound) = inspect.getclosurevars(function)
    nested_globals, nested_builtins = _global_vars_from_nested_code(function)
    return dict(sorted(
        {**nested_builtins, **builtins_, **nested_globals, **globals, **nonlocals}.items()
    ))

Resolution order mirrors inspect.getclosurevars itself; nonlocals still win. The comprehension target is not collected, because it is in the nested object's co_varnames rather than co_names.

Results on this branch: tests/next_tests/unit_tests/ 2144 passed, 51 skipped, 14 xfailed — no regressions. The comprehension tests in test_tuples.py pass on gtfn CPU including skip_value_mesh. mypy src/ and pre-commit are clean.

Caveats I would want settled: _iter_code_objects yields the root only for its caller to discard it; the merge ordering is defensive rather than meaningful, since both sources resolve from the same namespaces; and I have not checked the bound-method path, where getclosurevars unwraps to __func__ but this helper does not. It also does not address gtx.astype(...) inside a comprehension — but that fails outside a comprehension too, with DSLError: Functions can only be called directly, so it is a separate pre-existing restriction and not part of this bug.

I have not opened a PR for this; it is yours to take or discard.


2. Variable-length tuple programs are not treated as generic

CompiledProgramsPool._is_generic (otf/compiled_program.py) tests only ts.DeferredType. A program taking tuple[Field, ...] has VarArgType argument types, so it is classed non-generic and the tuple length never enters the compiled-program cache key — two calls with different tuple lengths reuse one compiled variant. On this head:

program arg types: ['VarArg[Field[[IDim], float64]]', 'float64', 'VarArg[Field[[IDim], float64]]']
any DeferredType (the rule used here): False

#2834 fixes the predicate by recursing into VarArgType. Please do not simply move that fix down here, though: compile() never passes arg_specialization_info, and _compile_variant then raises "Can not precompile generic program or scan operator without argument types." for generic programs. So the #2834 predicate makes ahead-of-time compilation of variadic-tuple programs impossible, and there is no parameter on the public compile() through which to supply the length. That raise pre-exists on main, where it already blocks AOT for scan operators, so this is a gap in the AOT API surface rather than a defect introduced here — but it means fixing the cache key alone converts a silent wrong-result into a hard block. Worth deciding deliberately.

The new test_var_len_tuple_length_mismatch_rejected xfail acknowledges an adjacent symptom; this is the other half.


Smaller points inline below.

return self.visit(target_el, refine_type=type_, **kwargs)
except IndexError:
raise errors.DSLError(
target_el.location, f"Cannot unpack non-iterable '{type_}' object."

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.

Two issues with this handler.

First, the message conflates two different failures. An IndexError here means either the element is not a tuple at all, or it is a tuple that is too short for the requested unpacking pattern. Both report "Cannot unpack non-iterable ... object", which is wrong for the arity-mismatch case. (Raised on the predecessor PR as #2487 (comment) 3318935481.)

Second, and newly introduced by this formulation: the try block wraps self.visit(target_el, refine_type=type_, **kwargs) on line 769, not just the path walk above it. Any IndexError raised anywhere inside that visit — including one from an unrelated bug — is caught and reported as an unpacking failure, with type_ holding the fully resolved leaf type by then, which makes the message nonsense. Narrowing the try to the for loop would fix both the masking and half the message ambiguity.

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.

@tehrengruber-ai agreed. Make sure there is a type_deduction test for the two errors.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed in 4f24c6d (pending push). The two failure modes now raise distinct located DSLErrors — "Cannot unpack non-iterable '' object." for a non-tuple element and "Not enough values to unpack (expected at least N, got M)." for a too-short tuple — and the 'try' wrapper is gone entirely (the checks raise directly, so an unrelated IndexError from 'self.visit' can no longer be swallowed and misreported). Both errors are pinned by new type_deduction tests: 'test_tuple_comprehension_unpack_non_tuple' and 'test_tuple_comprehension_unpack_too_short_tuple'.

element_types = cast(list[ts.DataType], iterable.type.types)
# Only homogeneous iterables are supported, see ADR 0028.
if not all(element_type == element_types[0] for element_type in element_types):
raise NotImplementedError(

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.

NotImplementedError rather than errors.DSLError for what is a user-facing restriction: the user gets a bare traceback with no source location pointing at their comprehension.

ADR 0028 is a good writeup of why the restriction exists, and the "(see ADR 0028)" reference is helpful — but the diagnostic should still be able to point at the offending line. Every other rejection in visit_TupleComprehension (empty tuple, non-data element types, non-tuple iterable) already raises DSLError with iterable.location.

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.

@tehrengruber-ai Use a plain DSLError mentioning that this is not implemented.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Done in 4f24c6d (pending push): now a plain located DSLError at 'iterable.location' — "Tuple comprehensions over fixed-length tuples with differently typed iterable elements are not implemented (see ADR 0028)." The three rejection tests in test_foast_to_gtir.py updated accordingly.

Comment thread src/gt4py/next/type_system/type_translation.py

canonical_type = typing.get_origin(type_hint) or type_hint
args = typing.get_args(type_hint)
# In order to distinguish `tuple` from `tuple[()]`, the former returns None here.

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.

This comment and the one at line 225 now contradict each other.

Here: "In order to distinguish tuple from tuple[()], the former returns None here." At line 225: "Unparametrized annotation, i.e. tuple or tuple[()]" — and the branch maps both to the same ts.DeferredType(constraint=ts.TupleType). I confirmed they are identical in output:

tuple[()] -> DeferredType(constraint=TupleType)
tuple     -> DeferredType(constraint=TupleType)

So the None-versus-() machinery is built and then deliberately not used. That is a defensible choice — but as written, the first comment tells a reader the distinction is load-bearing when it is not. Either drop this comment, or keep the distinction and map tuple[()] to ts.TupleType(types=[]), which is what the annotation actually means.

(Note tuple[()] raised ValueError on main, so nothing regresses either way — it is only a question of which behaviour is intended.)

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.

@tehrengruber-ai Why is it that way? The unparameterize tuple should just return None.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It was that way because 'get_args' cannot distinguish the cases: bare 'typing.Tuple' also has origin 'tuple' and 'get_args() == ()', identical to 'tuple[()]' — so the branch lumped '()' in with 'None' to keep 'typing.Tuple' working, leaving the None-machinery unused as Hannes observed. Fixed in 4f24c6d (pending push): 'canonicalize_type_hint' now keys on the 'args' attribute (unsubscripted typing aliases have a truthy origin but no 'args'), so 'None' really means unparametrized for every spelling ('tuple', 'typing.Tuple') while the empty-tuple annotation ('tuple[()]', 'typing.Tuple[()]') keeps 'args == ()' and is rejected with the existing "Tuple annotation ..." ValueError. Comments on both sides updated; pinned in test_type_translation.py.

if isinstance(symbol_type, ts.VarArgType) and isinstance(to_type, ts.VarArgType):
return is_concretizable(symbol_type.element_type, to_type.element_type)
if isinstance(symbol_type, ts.VarArgType) and isinstance(to_type, ts.TupleType):
if len(to_type.types) == 0 or (

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.

len(to_type.types) == 0 short-circuits to True unconditionally, so a VarArgType is considered concretizable to the empty tuple regardless of its element type — e.g. a field operator returning tuple[Field, ...] would accept out=().

Probably harmless in practice given where this is called from, but it is an unguarded hole rather than a deliberate identity case. If the empty tuple genuinely should be accepted, a short comment saying why would help; otherwise it wants the same element check as the non-empty path.

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.

@tehrengruber-ai "so a VarArgType is considered concretizable to the empty tuple regardless of its element type" is sound. That a field operator accepts an empty tuple for a vararg can also make sense, add a test. If fixing it is complicated let me know.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test added in 4f24c6d (pending push): 'test_is_concretizable_vararg_to_tuple' in test_type_info.py pins the empty tuple as a valid concretization (vacuously true — no elements whose type could conflict), alongside matching/mismatching non-empty cases; also added the short why-comment at the check itself. On the end-to-end side, letting you know per your offer: an actual empty-tuple call works in embedded ('scale((), 2.0, out=())' runs fine) but the compiled path crashes in 'past_to_itir._visit_stencil_call_out_arg' — an empty 'out' tuple leaves the SetAt with nothing to unpack ('ValueError: not enough values to unpack (expected 2, got 0)'). Supporting it there means deciding what a SetAt with an empty target does (probably: emit no SetAt at all) across past_to_itir and domain inference, so I left the compiled path alone and did not add a matrix-wide integration test.

@tehrengruber

Copy link
Copy Markdown
Contributor

@tehrengruber-ai Some directions:

  1. Let's not do this in this PR, but in case proper error handling for this case disallow it. One can always use a dedicated field operator as a workaround. Introduce a test for the example and propose a text for an issue.
  2. Introduce a test that calls a field operator first with a tuple of length X and then the same one with a length of Y. Fix _is_generic. The compile gap will be solved in a different PR.

@tehrengruber-ai

Copy link
Copy Markdown

Both directions addressed: 1. The test for the example already exists — 'test_var_len_tuple_length_mismatch_rejected' in test_tuples.py (ed23528) uses exactly the conditional-return construction and asserts the desired located DSLError as xfail(strict). The proposed issue text is drafted in 'submit_vararg_length_issue.sh' in the worktree: it describes the missing length validation, both the plain out-mismatch and the ambiguous conditional-return example, the specialization-in-'with_args' direction with the dedicated-field-operator workaround, and the 'foast_to_past' vs PAST-deduction predicate discrepancy. 2. Done in e449e6982 (pending push): 'test_var_len_tuple_comprehension_different_lengths' calls the same operator with lengths 3 then 2 — before the fix the second call reused the length-3 compiled variant and crashed with 'IndexError: tuple index out of range'. '_is_generic' now treats parameters containing a 'VarArgType' (also nested inside tuples) as generic, so each length gets its own specialized variant; verified across the backend matrix and the otf unit tests. The compile gap for the empty tuple stays out, as decided — xfailed test plus issue draft 'submit_empty_tuple_issue.sh'.

@havogt

havogt commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

I think you @tehrengruber misunderstand the severity of 1 (or I misunderstood the comment). Also module level field_operators have this problem.

havogt pushed a commit to havogt/gt4py that referenced this pull request Sep 4, 2026
`get_closure_vars_from_function` used `inspect.getclosurevars`, which reads only
the enclosing function's code object. A generator expression compiles to its own
code object, so names referenced only inside a tuple comprehension body — gt4py
builtins, `FieldOffset`s, `Dimension`s, module-level field operators — were never
collected, and the comprehension failed with `UndefinedSymbolError` on a name that
is visibly imported.

Free variables were unaffected, which is why every comprehension test in GridTools#2833
passes: they define their helpers inside the test function.

Prototype for gt4py-f24. Not proposed upstream; committed so the reproducer and
fix outlive the worktree they were written in.

Claude-Session: https://claude.ai/code/session_01F8vDQpsHU76tFgD1tk4GAC
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