[Quality] Fixes typings in ast frontend#2520
Conversation
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
📝 WalkthroughWalkthroughThis PR tightens typing and span handling in ChangesEager AST typing and span handling
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| def ast_set_span(ast: ast.AST, span: tuple[int, int, int, int]): | ||
| if not ast_has_span(ast): | ||
| def ast_set_span(ast: ast.AST, span: Span | None): | ||
| if span is None or not ast_has_span(ast): |
There was a problem hiding this comment.
A bit redundant. This is for type alignment for patterns like ast_set_span(ast_set_span()) but ast_has_span already guards it.
| ast_set_span(tmp_store, node.targets[0]) | ||
| ast_set_span(tmp_load, node.targets[0]) | ||
| stmt = self._emit_assign_target(tmp_store, rval) | ||
| ast_set_span(tmp_store, ast_get_span(node.targets[0])) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tilelang/language/eager/ast.py (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
strict=Trueonzip.
_span_attrsandspanare always length-4 by construction (Span: TypeAlias = tuple[int, int, int, int]), so this is low risk, but addingstrict=Truecosts nothing and guards against future drift if either side's arity changes.🔧 Suggested tweak
- for attr, value in zip(_span_attrs, span): + for attr, value in zip(_span_attrs, span, strict=True):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tilelang/language/eager/ast.py` at line 34, The span unpacking in the AST helper uses zip without enforcing equal lengths; update the loop in the code that iterates over _span_attrs and span to use strict=True. This change should be applied where the span dictionary/object is built, so the existing behavior stays the same while guarding the Span tuple and _span_attrs from drifting out of sync.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tilelang/language/eager/ast.py`:
- Line 34: The span unpacking in the AST helper uses zip without enforcing equal
lengths; update the loop in the code that iterates over _span_attrs and span to
use strict=True. This change should be applied where the span dictionary/object
is built, so the existing behavior stays the same while guarding the Span tuple
and _span_attrs from drifting out of sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0c9f20a-9f74-48ff-a527-eda5ffc2a1e6
📒 Files selected for processing (1)
tilelang/language/eager/ast.py
| arg_stmt = quote1(f'{name} = __tb.arg("{name}", {name})', span=arg) | ||
| else: | ||
| arg_stmt = quote1(f'{name} = __tb.arg("{name}", {name})', span=arg) | ||
| arg_stmt = quote1(f'{name} = __tb.arg("{name}", {name})', span=arg) |
There was a problem hiding this comment.
two branches are identical. The annotation parsing is inside _parse_arg_annot
| is_kernel_ctx = True | ||
| break |
There was a problem hiding this comment.
Early exit as soon as we are sure this is a kernel context
1 participant
Learning tilelang frontend and fixes various typing issues by the way. This is a purly quality PR. No behavior changes.
SpanandQuoteReplacementfor better readabilityast_set_span(tmp_store, node.targets[0])should beast_set_span(tmp_store, ast_get_span(node.targets[0]))Summary
tilelang/language/eager/ast.pyby addingSpanandQuoteReplacementaliases and updating AST helper/visitor/quote function signatures to be more precise about accepted span inputs and returned node types.BaseBuilder, non-Noneguard inBaseBuilder.boolop, and explicitquote1(...)→ast.AssigninDSLMutator.flush_binds).ast_set_span(tmp_store, node.targets[0])toast_set_span(tmp_store, ast_get_span(node.targets[0]))(copying span metadata from the normalized target).visit_For,visit_Assign,visit_FunctionDef,visit_IfExp, andvisit_With(including early break once the kernel context is found).C++ style / lint notes
tilelang/language/eager/ast.py, so it does not touch C++, CI/lint tooling, ordocs/developer_guide/cpp_style.md.