Conversation
|
Good catch, yeah that's a vestigial remnant of what I was mentioning during
the meeting, about originally trying to encode whether or not the sequence
length was fixed via the type system. I'll rework it.
… Message ID: ***@***.***>
|
|
Still polishing a few details here |
328b00c to
67f9eec
Compare
|
@Turupawn I just did another push, hopefully this is pretty close to what gets merged |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67f9eec23d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Add end field initialization | ||
| if let Some(info) = self.field_access_info(range_ty, FieldIndex::Ident(end_ident)) { | ||
| inits.push(( | ||
| MirProjectionPath::from_projection(Projection::Field(info.field_idx)), | ||
| end_value, |
There was a problem hiding this comment.
Avoid storing Known range bounds into ZST fields
In lower_range_expr you always initialize the end field with end_value, but Range bounds are zero-sized when they’re Known (Known’s Repr is () in library/core/src/range.fe). For a range like n..10, the type is Range<Unknown, Known<10>>, whose runtime layout only includes start; initializing end still emits a usize store at the end-field offset, which is beyond the allocated size and can clobber adjacent memory. This should skip initialization when the bound field type is zero-sized (or synthesize a unit) so Known bounds don’t emit stores.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec06f01b77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
crates/mir/src/lower/expr.rs
Outdated
| origin: ValueOrigin::PlaceRef(place.clone()), | ||
| repr: ValueRepr::Word, | ||
| }); | ||
| // Emit a load instruction for the start value | ||
| self.assign(None, None, Rvalue::Load { place }); |
There was a problem hiding this comment.
Avoid emitting load without a destination
The range fallback currently emits Rvalue::Load with dest = None, but the Yul emitter rejects this (emit_assign_inst errors on “load without destination”), so any for loop that hits lower_for_range (e.g., when Seq resolution fails or core Seq isn’t available during error recovery) will crash during codegen. Please load into a temp local (as done elsewhere) or otherwise avoid generating load-only assignments.
Useful? React with 👍 / 👎.
crates/mir/src/lower/expr.rs
Outdated
|
|
||
| // Extract the end value from the range | ||
| let end_value = | ||
| if let Some(info) = self.field_access_info(range_ty, FieldIndex::Ident(end_ident)) { |
There was a problem hiding this comment.
Better to unwrap here and remove the fallback. Same goes for the 'start' case above.
| fn len(self) -> usize { | ||
| E - S | ||
| } |
There was a problem hiding this comment.
This should use saturating_sub (though sadly that doesn't exist yet), or we should add a hir check to ensure E >= S at least for the sugar syntax. (or we just make an issue to modify this when saturating_sub is available).
crates/mir/src/lower/expr.rs
Outdated
| if let Some(elem_local) = elem_local { | ||
| let idx_for_get = self.builder.body.alloc_value(ValueData { | ||
| ty: usize_ty, | ||
| origin: ValueOrigin::Local(idx_local), | ||
| repr: ValueRepr::Word, | ||
| }); | ||
| let elem_value = self.emit_seq_get_call( | ||
| iterable_value, | ||
| idx_for_get, | ||
| elem_ty, | ||
| &seq_info.get_callable, | ||
| &seq_info.get_effect_args, | ||
| ); | ||
|
|
||
| self.assign(None, Some(elem_local), Rvalue::Value(elem_value)); |
There was a problem hiding this comment.
This doesn't work for pattern bindings, eg for Point { x, y } in points.
I think all you need to do is:
// remove the `if let Some(elem_local)` guard
let idx_for_get = ...
let elem_value = ...
self.bind_pat_value(pat, elem_value)
// carry on with the `// Execute the body` code below.
sbillig
left a comment
There was a problem hiding this comment.
Looks good, pending minor issues above.
Update check_for to resolve element types via Seq<T> trait lookup rather than the placeholder Iterator reference. Arrays remain special cased since const generics in array type positions aren't fully supported yet.
Range expressions (a..b) now type check to DynRange. Both operands are verified to be usize, and the result is resolved from core::range::DynRange.
Desugar range expressions `start..end` in MIR lowering to allocate and initialize a DynRange struct with start and end fields, rather than leaving them as Range binops that panic at codegen.
Desugar for-loops into while loops: - For ranges (DynRange): initialize loop var to start, loop while < end - For arrays: create hidden index, bind element each iteration Both cases increment by 1 and use standard loop control flow with proper continue/break targets registered.
Remove the bifurcated Const<N>/Dyn type system in favor of a simple approach that matches issue argotorg#1207: - Remove Const<N> and Dyn marker types from seq.fe - Remove Range<N> (const-length), keep only Range with start/end - Simplify Seq trait to return usize directly (no Len associated type) - Update type checker and MIR comments from DynRange to Range This defers const-vs-dynamic optimization to the backend rather than encoding it in the type system, reducing complexity significantly.
Previously, `impl<const N: usize> Trait for [T; N]` failed because the target type was lowered before entering the impl scope, so const generic params weren't available for array length expressions. Fix by entering the impl scope with a preliminary ID first, lowering generic params, then lowering the target type. The final ID is swapped in before lowering nested items like methods. This enables the array Seq implementation in core library.
Resolve Seq::len and Seq::get at type-check time and store in ForLoopSeq for MIR lowering. Desugars `for x in seq` to an index-based while loop calling the resolved trait methods. - snapshot/rollback when probing Seq impls to prevent inference pollution - route `continue` to increment block to avoid infinite loops - preserve effect args on synthesized Seq method calls
- Prevent unification-table panics when resolving associated types via impls - Constrain record init generics from expected type without inference pollution - Add uitests for assoc-type struct fields and for-loop over array literal
Replace the simple Range struct with a generic Range<S: Bound, E: Bound> that encodes bound constness in the type system: - Range<Known<0>, Known<10>> for fully const ranges (0 words) - Range<Known<0>, Unknown> for const start, runtime end (1 word) - Range<Unknown, Unknown> for fully dynamic ranges (2 words) The type checker now detects integer literals in range expressions and constructs the appropriate Range type, enabling zero-cost abstractions when bounds are compile-time known.
ec06f01 to
d0e84a8
Compare
|
@Turupawn here's some info on where things landed and some ideas for integrating #1211: Seq-Based Iteration: Architecture Report & Future Integration GuideSummaryThe This report covers the current architecture, considerations for integrating codegen-focused features, and some longer-term design explorations. Part 1: Current Architecture1.1 Unified Iteration via Seq + ForLoopSeqThe central design decision: all There's no range-specific for-loop lowering — Range implements Seq like any other sequence type. This simplifies the compiler and means new Seq implementors automatically work with for-loops. Pattern binding is handled uniformly: the loop element always flows through Reference: 1.2 Range Type SystemThe Range type uses type-level encoding for memory optimization:
Layout invariants around ZST fields are enforced in MIR lowering and transforms, preventing out-of-bounds writes. Reference: 1.3 Range::len() Semantics
This logic is implemented across the four Seq impls (const/const, const/runtime, runtime/const, runtime/runtime), each accessing bounds appropriately for its type signature. Reference: 1.4 Core Type ResolutionRange type discovery is centralized via Reference: 1.5 TargetDataLayoutCodegen now receives a // Query the layout
let word_size = layout.word_size_bytes;
// Rather than hardcoding
// if size > 32 { ... }Reference: Part 2: Integrating Codegen Features2.1 Loop UnrollingThe natural integration point is A subtlety worth noting: Semantic considerations: Unrolled loops need to preserve An alternative structure: Implementing unrolling as a MIR transform pass (post-lowering) rather than during lowering has some appeal — it separates semantic lowering from optimization, and makes it easier to gate by optimization level. 2.2 Data RegionsThe concept of storing large constant arrays/strings in data sections (rather than inlining them) is valuable for EVM targets. Backend neutrality: The MIR representation benefits from being backend-agnostic. A Existing infrastructure: Fe already has code region plumbing ( Reference: Design Note: Core/Std RelevanceData regions are the simplest instance of a broader "data lives somewhere" story (code section, calldata, memory, storage, etc.). Even if the immediate work is EVM/Yul-focused, it's worth shaping the implementation so it doesn't pre-commit Fe's future core/std interfaces. Concretely: keep the compiler's representation backend-neutral, keep size/threshold policy layout-driven, and avoid encoding backend naming/labeling decisions into MIR. This keeps the door open for future library-level "region/handle" traits without forcing the design today. Future-proofing checklist (for any data-region PR):
Part 3: Design Exploration — DataLoc/StaticSeq
3.1 The PatternFollowing the 3.2 StaticSeq SketchThe compiler could automatically select 3.3 ConsiderationsThis design implies compiler intrinsics for loading elements from code/data sections, and ties into backend capabilities that vary across targets. It also intersects with ongoing discussions about core/std "memory region" interfaces and contract desugaring plumbing; treat Part 4: Integration SummaryThe A few patterns emerge from the current design:
Part 5: Key Files Reference
|
Implements generic iteration via a
Seq<T>trait, following Sean's proposal in #1207. Also adds..range operator syntax and fixes const generics in impl target types.Changes
Parser/HIR
..asArithBinOp::Range, parsed and lowered through HIRCore library (
library/core/)Seq<T>trait -fn len(self) -> usizeandfn get(self, i: usize) -> TRange/DynRange- range types withSeqimplementationsSeqimpl -impl<T, const N: usize> Seq<T> for [T; N]Type checking (
hir/analysis/ty/)Seqtrait lookup instead of hardcoded array/range casesForLoopSeq- stores pre-resolvedCallableforlen/getso MIR can emit direct callsMIR lowering
for x in seqbecomes index loop using resolvedSeq::len/Seq::getcontinuenow targets increment block, not condition (fixes infinite loop bug)CallOrigin.expr- nowOption<ExprId>to represent synthesized calls without fake expr IDsConst generics fix
Impl::lower_astandImplTrait::lower_astnow enter preliminary scope first, lower generics, then lower target typeimpl<T, const N: usize> ... for [T; N]patterns that previously failed resolutionTests
const_generic_impl_target_array.fe/const_generic_impl_regression.fe- uitest for const generic fixfor_continue.fe/for_continue.mir.snap- MIR snapshot for continue-in-for correctnessQuestions / follow-up
RangevsDynRange? Do we need this distinction?