feat[next]: type annotations in the ITIR pretty-printed syntax - #2822
feat[next]: type annotations in the ITIR pretty-printed syntax#2822havogt wants to merge 23 commits into
Conversation
`pretty_parser` exists to invert `pretty_printer`, but the two are never
composed in a test: `test_pretty_printer.py` never imports `pparse`,
`test_pretty_parser.py` never imports `pformat`. The two files are
hand-mirrored corpora that have already drifted, and a token added to the
printer alone is invisible to both.
Seed the property with the union of the terms both files already use. Seven
cases fail and are marked `xfail(strict=True)`, in three groups:
- `∞` / `-∞` printed by `visit_InfinityLiteral`, no grammar token
- `<=` / `>=` printed via `BINARY_OPS`, no grammar rule
- `Literal.type` not printed at all, so the parser re-types the lexeme
(this is why `test_pretty_printer.py::test_arithmetic`
uses `int64` while its `test_pretty_parser.py` twin
expects `int32`)
The following two commits clear the first two groups.
`visit_InfinityLiteral` emits `∞` / `-∞`, but the grammar had no token for either, so every printed domain with an unbounded end failed to reparse. `InfinityLiteral` is a distinct node, not a typed `Literal`, and `-∞` is one node rather than `minus(∞)`, so a single terminal covering both signs is the whole fix. It does not interfere with `prec5 "-" prec6` — `a - ∞` still parses as `minus(a, ∞)`, asserted in `test_minus_infinity_is_not_a_subtraction`. Clears the `∞` group of round-trip xfails.
`BINARY_OPS` maps `less_equal`/`greater_equal` to `<=`/`>=` at precedence 4, but `prec4` only covered `==`, `<` and `>`, so any printed comparison using the two-character forms failed to reparse. Clears the `<=`/`>=` group of round-trip xfails. `test_comparison` covers all four operators, since `<` and `<=` share a prefix.
`ToIrTransformer` errors reached the caller wrapped in lark's `VisitError`, burying the message one frame down. Re-raise the original exception so an unsupported type name reports as the `NotImplementedError` it is.
The printer had one place where a type reached the output, `dtype=` on a `Temporary`, and it got there via `str(node.dtype)`. The parser inverted that with `getattr(ts.ScalarKind, value.upper())`. Nothing declared the vocabulary; the two agreed only because `ScalarType.__str__` is `kind.name.lower()` and the parser was its mechanical inverse. That is about to stop being enough, because literal annotations need the same spelling in a second place. Declare it instead. `SCALAR_TYPE_NAMES` in `pretty_printer` is now the sole definition, LLVM/MLIR style, and `pretty_parser` imports the derived reverse map rather than keeping one of its own: bool -> i1 int8 -> i8 uint16 -> u16 int32 -> i32 float64 -> f64 `ScalarType.__str__` is deliberately untouched -- it serves `foast_pretty_printer` and error messages, and ITIR's surface syntax has no business leaking into the type system. `ScalarKind.STRING` has no LLVM spelling and is excluded, so printing or parsing one raises. The table covers exactly `builtins.TYPE_BUILTINS`, asserted in `test_scalar_type_names_cover_the_type_builtins` so the two cannot drift. **Behaviour change**: `pformat` emits `dtype=f64`, and `pparse` no longer accepts `dtype=float64`. This is deliberately not a compatibility shim -- one spelling, both directions. Also replaces `TYPE_LITERAL` in the `declaration` rule with a `type_expr` sub-grammar, fixing a pre-existing round-trip hole: `Temporary.dtype` may be a `TupleType` or a shaped `ScalarType`, both of which printed but did not parse.
`Literal.type` participates in equality but never reached the output, so the parser re-typed the lexeme and any literal whose type differed from that default came back wrong: `float32` became `float64`, `int8`/`int64` became `int32`. Spell the annotation as a postfix `:` on the literal, `1.0:f32`, no space, as in `named_range`'s `KDimᵥ:`. It sits at `prec9`, so it binds tighter than every operator and `1.0:f32 + 2.0` needs no parentheses. `:` was previously used only by `named_range`, which is guarded on both sides -- an `AXIS_LITERAL` before (lark's `CNAME` is ASCII, so it can never be a type name) and a `[` after -- so the two do not overlap. The annotation takes a bare scalar name and nothing else. Admitting the bracketed type forms would make `1:i16[2]` and `1:tuple[f32]` genuinely ambiguous with `tuple_get`, which is also `prec8 "[" prec0 "]"` -- Earley would have resolved that silently by priority. `Literal.type` is a `ts.ScalarType`, so a tuple was never reachable anyway; a *shaped* `ScalarType` now raises on print rather than emitting something that reparses as an index. `pformat(x, types=...)` selects when the annotation is written: "minimal" (default) only where `implied_literal_type` disagrees with the node "none" never, the previous output "all" on every `Literal` `implied_literal_type` is the single definition of what a bare lexeme means, and the parser reads the same function, so the printer's elision rule cannot drift away from what the parser will do with the result. `"minimal"` leaves existing output untouched wherever the types were already the implied ones: over a 156-term corpus with 814 literals (730 `int32`, 74 `float64`, 10 `bool`), no literal gains an annotation. Clears the last group of round-trip xfails.
feb16b7 to
387591d
Compare
`pformat` had no `visit_NoneLiteral` and raised out of eve's `generic_visit`, though `im.ensure_expr(None)` produces one. Spelled `None`, as the Python code generator in `roundtrip.py` already does, and recognised in `SYM_REF` alongside `True`/`False`, so no grammar change is needed.
…rinter-typed-literals # Conflicts: # src/gt4py/next/iterator/pretty_parser.py # tests/next_tests/unit_tests/iterator_tests/test_pretty_roundtrip.py
This reverts commit ec444fb.
Review suggestion: match the two spellings explicitly rather than testing for a leading `-`, so an unexpected token asserts instead of silently becoming positive infinity.
…rinter-typed-literals # Conflicts: # src/gt4py/next/iterator/pretty_parser.py # tests/next_tests/unit_tests/iterator_tests/test_pretty_roundtrip.py
Review suggestion. The 34 terms are unchanged; verified by comparing each against the hand-built version it replaces.
…rinter-typed-literals # Conflicts: # tests/next_tests/unit_tests/iterator_tests/test_pretty_roundtrip.py
…-typed-literals # Conflicts: # tests/next_tests/unit_tests/iterator_tests/test_pretty_parser.py # tests/next_tests/unit_tests/iterator_tests/test_pretty_roundtrip.py
Matches the existing expression syntax for `make_tuple` instead of introducing a second tuple spelling. Also drops the guard against annotating a literal with a shaped type: `Literal.type` with a shape denotes nothing and nothing builds one.
Review suggestion.
Drops the `types` parameter and the elision rule with it: `pformat` annotates every literal except `bool`, whose `True`/`False` spelling already fixes the type. `implied_literal_type` moves to the parser, which is now its only user.
| >>> print(nested_as_fieldop) | ||
| as_fieldop(λ(__arg0, __arg1) → ·__arg0 + ·__arg1, c⟨ IDimₕ: [0, 1[ ⟩)( | ||
| as_fieldop(λ(__arg0, __arg1) → ·__arg0 × ·__arg1, c⟨ IDimₕ: [0, 1[ ⟩)(inp1, inp2), inp3 | ||
| as_fieldop(λ(__arg0, __arg1) → ·__arg0 + ·__arg1, c⟨ IDimₕ: [0:i32, 1:i32[ ⟩)( |
There was a problem hiding this comment.
Should we drop printing the type for domains? Because it's the index type which I believe we fix to 32 bit.
There was a problem hiding this comment.
Pull request overview
This PR updates the gt4py.next.iterator pretty-printed surface syntax so that scalar types are rendered using LLVM/MLIR-style spellings (e.g., f64, i32) and literals can carry postfix type annotations (e.g., 1:i64), improving pformat/pparse roundtripping when Literal.type differs from what the lexeme implies.
Changes:
- Extend the pretty-printer to emit LLVM/MLIR-style scalar type spellings, print typed literals, and print
Temporary.dtypevia a sharedformat_type()helper. - Extend the pretty-parser grammar to accept typed literals (
<literal>:<type>) and richerdtype=type expressions (tuple and shaped scalar forms). - Update tests and doctest-style examples to match the new printed syntax and add new roundtrip coverage.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/gt4py/next/iterator/pretty_printer.py |
Adds scalar type spelling tables and prints typed literals / Temporary.dtype using the new syntax. |
src/gt4py/next/iterator/pretty_parser.py |
Adds parsing for typed literals and richer dtype type expressions; aligns error behavior for invalid types. |
src/gt4py/next/iterator/ir_utils/ir_makers.py |
Updates doctest outputs impacted by the new pretty-printed literal/type syntax. |
src/gt4py/next/iterator/ir_utils/misc.py |
Updates doctest output to reflect typed literal printing. |
src/gt4py/next/iterator/transforms/fuse_as_fieldop.py |
Updates example pretty-printed output in docstring to include typed integer literals. |
src/gt4py/next/iterator/transforms/inline_fundefs.py |
Updates example pretty-printed output in docstring to include typed integer literals. |
src/gt4py/next/iterator/transforms/inline_lambdas.py |
Updates example pretty-printed output in docstring to include typed integer literals. |
src/gt4py/next/iterator/transforms/remove_broadcast.py |
Updates example pretty-printed output in docstring to include typed integer literals. |
src/gt4py/next/iterator/transforms/replace_get_domain_range_with_constants.py |
Updates example pretty-printed output in docstring to include typed integer literals (and wraps lines). |
tests/next_tests/unit_tests/iterator_tests/test_pretty_parser.py |
Adds parser tests for typed literals and compound dtype expressions; updates legacy expectations. |
tests/next_tests/unit_tests/iterator_tests/test_pretty_printer.py |
Adds printer tests for type annotations, type spelling coverage, and compound dtype printing. |
tests/next_tests/unit_tests/iterator_tests/test_pretty_roundtrip.py |
Removes literal-type xfails and adds explicit roundtrip cases for typed literals and compound dtype. |
tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_cse.py |
Updates expected pretty-printed output to include typed integer literals. |
tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_inline_center_deref_lift_vars.py |
Updates expected pretty-printed output to include typed float literal. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tehrengruber
left a comment
There was a problem hiding this comment.
Before I read the code here: I would rather skip the type string in case it is not needed (following the same logic as type_translation.from_value). Booleans are certainly nicer as True and False not 1:f1 for our purposes.
|
|
One behaviour: a literal is annotated iff its type differs from the one its lexeme implies. `implied_literal_type` reads the lexeme back to a Python value and types it with `type_translation.from_value`, the same function the frontend applies to a constant.
| #: Surface spelling of the scalar types, LLVM/MLIR style. | ||
| SCALAR_TYPE_NAMES: Final[Mapping[ts.ScalarKind, str]] = _types.MappingProxyType( | ||
| { | ||
| ts.ScalarKind.BOOL: "i1", |
There was a problem hiding this comment.
fyi, bool is still used for the dtype of temporaries. I was wondering if we should use bool here instead of i1, because it's the least intuitive, but then I thought it doesn't harm to learn this convention instead of mixing styles.
There was a problem hiding this comment.
Another alternative could be to use Numpy dtype abbreviations, where boolean would be b1 and the rest very similar (although the number would indicate number of bytes, not bits) : https://readmedium.com/numpy-typecodes-cheatsheet-1c4cd8fd2318
`typed_literal` accepted a `SYM_REF`, set `type` on the resulting `SymRef`, and lost it again on print. `SymRef.type` does not participate in equality, so the round trip did not notice.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/gt4py/next/iterator/pretty_parser.py:147
type_exprallows shaped scalar types likef64[3], but the grammar/transformer currently accepts negative extents (becauseINT_LITERALisSIGNED_INTandshaped_scalar_typedoes not validate). This would allow invalid dtypes such asf64[-1]to parse successfully and propagate downstream.
def shaped_scalar_type(self, type_: ts.ScalarType, *shape: ir.Literal) -> ts.ScalarType:
return ts.ScalarType(kind=type_.kind, shape=[int(s.value) for s in shape])
egparedes
left a comment
There was a problem hiding this comment.
Only minor code style nitpicks and questions.
|
|
||
| from gt4py.next.iterator import ir | ||
| from gt4py.next.iterator.ir_utils import ir_makers as im | ||
| from gt4py.next.iterator.pretty_printer import SCALAR_TYPE_KINDS, implied_literal_type |
There was a problem hiding this comment.
Direct symbol imports are forbidden in our coding conventions (Google Python Conventions)
| if (kind := SCALAR_TYPE_KINDS.get(value.value)) is not None: | ||
| return ts.ScalarType(kind=kind) | ||
| raise ValueError( | ||
| f"Invalid type '{value}'; expected one of {', '.join(sorted(SCALAR_TYPE_KINDS))}." | ||
| ) |
There was a problem hiding this comment.
Minor coding style nitpick:
| if (kind := SCALAR_TYPE_KINDS.get(value.value)) is not None: | |
| return ts.ScalarType(kind=kind) | |
| raise ValueError( | |
| f"Invalid type '{value}'; expected one of {', '.join(sorted(SCALAR_TYPE_KINDS))}." | |
| ) | |
| if (kind := SCALAR_TYPE_KINDS.get(value.value)) is None: | |
| raise ValueError( | |
| f"Invalid type '{value}'; expected one of {', '.join(sorted(SCALAR_TYPE_KINDS))}." | |
| ) | |
| return ts.ScalarType(kind=kind) | |
| #: Surface spelling of the scalar types, LLVM/MLIR style. | ||
| SCALAR_TYPE_NAMES: Final[Mapping[ts.ScalarKind, str]] = _types.MappingProxyType( | ||
| { | ||
| ts.ScalarKind.BOOL: "i1", |
There was a problem hiding this comment.
Another alternative could be to use Numpy dtype abbreviations, where boolean would be b1 and the rest very similar (although the number would indicate number of bytes, not bits) : https://readmedium.com/numpy-typecodes-cheatsheet-1c4cd8fd2318
| if implied_literal_type(node.value) == node.type: | ||
| return [str(node.value)] | ||
| return [f"{node.value}:{format_type(node.type)}"] |
There was a problem hiding this comment.
Optional style suggestion:
| if implied_literal_type(node.value) == node.type: | |
| return [str(node.value)] | |
| return [f"{node.value}:{format_type(node.type)}"] | |
| return [ | |
| f"{node.value} | |
| if implied_literal_type(node.value) == node.type | |
| else f"{node.value}:{format_type(node.type)}" | |
| ] |
| try: | ||
| py_value = float(value) | ||
| except ValueError: | ||
| return None |
There was a problem hiding this comment.
Question: why is this case allowed? When is it normal behavior to return None?
- Import `pretty_printer` as a module in `pretty_parser` instead of importing symbols directly. - Use a guard clause in `TYPE_LITERAL`. - Express `visit_Literal` as a conditional expression. Claude-Session: https://claude.ai/code/session_01Jg7SRJwuEEVcZNo9Qtmge9
`implied_literal_type` returned `None` for a value that is neither a boolean
nor an integer nor a floating point lexeme. `visit_Literal` compared that
`None` against the node type, found them unequal and printed the annotated
form, so a malformed literal with a supported type was printed into text that
does not parse back -- `Literal('hello', i32)` became `hello:i32`, and
`pparse` rejects that.
Raise a `ValueError` naming the offending value instead. `_bare_literal`'s
`assert type_ is not None` becomes redundant; it was absent under `-O` anyway.
Claude-Session: https://claude.ai/code/session_01Jg7SRJwuEEVcZNo9Qtmge9
itir.Literal.typeparticipates in equality but was never printed, sopparsere-typed the lexeme from scratch and any literal whose type differs from the one
implied by its spelling came back as a different term:
What changed:
1.0:f32,1:i64,True:i1. Nospace, as in
named_range'sKDimᵥ:. It binds atprec9, tighter than everyoperator.
:was previously used only bynamed_range, guarded on both sides.forms would make
1:i16[2]ambiguous withtuple_get, which is alsoprec8 "[" prec0 "]";1:i16[2]is therefore a tuple index. Only a literal maybe annotated, not a symbol.
boolisi1,int32isi32,float64is
f64.SCALAR_TYPE_NAMESinpretty_printeris the single forward mappingand
pretty_parserimports the derived reverse map, so respelling a type is aone-line edit.
ts.ScalarType.__str__is deliberately unchanged; it servesfoast_pretty_printerand error messages.ScalarKind.STRINGhas no LLVM spelling and is excluded. The table coversexactly
builtins.TYPE_BUILTINS, asserted in a test so the two cannot drift.1,1.0andTrueprint as before.implied_literal_typereads the lexemeback to a Python value and types it with
type_translation.from_value, which iswhat the frontend applies to a constant, and the parser calls the same function
rather than mirroring it.
Temporary.dtypewith a shape or a tuple printed but did not parse; thedeclarationrule took onlyTYPE_LITERAL: CNAME. It now takes atype_exprsub-grammar, and a tuple dtype is spelled
{f64, f64}, asmake_tuplealreadyprints.
Breaking:
pformatemitsdtype=f64where it emitteddtype=float64.pformatemitsdtype={f64, f64}where it emitteddtype=tuple[float64, float64].pparseno longer acceptsdtype=float64or1.0:float32. One spelling, bothdirections, deliberately not a compatibility shim.
dtype=was the only place a type reached printed output before this PR, and nocode outside these two modules asserts on
pformatoutput.