feat: add initial compiler spine - #31
Conversation
|
Warning Review limit reached
More reviews will be available in 38 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (16)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6071fb9ed
ℹ️ 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".
| ) -> Option<CorePredicate> { | ||
| let left = self.check_expr(lhs, env)?; | ||
| let right = self.check_expr(rhs, env)?; | ||
| if comparable(&left.ty, &right.ty) { |
There was a problem hiding this comment.
When a where clause uses <, <=, >, or >= (for example where input.name < "z" over String<max=256>), this accepts the predicate as long as the operand shapes are compatible. The compiler-spine contract added here only includes equality predicates, and raw Core strings do not have an ordering policy, so this lowers source the initial subset should reject instead of reporting UnsupportedSourceShape/ExpectedPredicate.
Useful? React with 👍 / 👎.
| fields.insert(name.clone(), value.expr); | ||
| field_types.insert(name.clone(), value.ty); |
There was a problem hiding this comment.
For a record literal with a repeated field such as return { message: a, message: b };, these BTreeMap::insert calls silently overwrite the earlier value and type, so compile_to_core accepts and lowers invalid source instead of rejecting the duplicate field. This can hide author mistakes and produce Core that does not faithfully represent the source literal.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn string_value(value: &str) -> TypedValue { | ||
| let max = value.len() as u64; |
There was a problem hiding this comment.
For non-ASCII literals like "é", str::len() counts UTF-8 bytes rather than Unicode scalar values, while String<max=N> bounds are defined in scalar values. This makes a valid literal for String<max=1> look like max=2 and can reject valid programs or inflate concatenation bounds; use a scalar count such as value.chars().count() for the literal bound.
Useful? React with 👍 / 👎.
| let Some(shape) = self.type_decl_shape(&decl.source) else { | ||
| continue; | ||
| }; | ||
| self.core_types.insert(decl.name.clone(), shape.core_type()); |
There was a problem hiding this comment.
This stores each lowered type under the source-local name (HelloInput), but TypeShape::coord is the fully qualified Core coordinate (examples.hello@1.HelloInput) and that is what intents and record fields use as their type references. The resulting CoreModule contains type refs that cannot be resolved through core.types, so even the newly compiled fixture is internally inconsistent for consumers that walk Core type references.
Useful? React with 👍 / 👎.
| let input_shape = self.type_ref_shape(¶m.ty, param.span, None)?; | ||
| let output_shape = self.type_ref_shape(&source.returns, source.span, None)?; | ||
| let input_binding = LocalRef { | ||
| id: param.name.clone(), |
There was a problem hiding this comment.
Using param.name as the Core LocalRef.id makes the lowered Core change when an author only renames the intent parameter, even though alpha_name remains $arg0. The Core topic says local ids are compiler-owned and source binder spelling is not identity, so intent t(request: HelloInput) and intent t(input: HelloInput) should not produce different semantic local ids.
Useful? React with 👍 / 👎.
| let canonical = refine | ||
| .canonical | ||
| .clone() | ||
| .unwrap_or_else(|| "raw-utf8".to_owned()); |
There was a problem hiding this comment.
For a source type like String<max=128, canonical=nfc>, this copies the source spelling nfc directly into the Core string type. The Core schema only permits canonical values such as unicode-scalar-nfc and raw-utf8, so the compiler can emit an in-memory Core type that does not match the Core contract instead of translating or rejecting unsupported policies.
Useful? React with 👍 / 👎.
| )); | ||
| return None; | ||
| }; | ||
| let max = lmax + rmax; |
There was a problem hiding this comment.
With very large declared string bounds, e.g. concatenating two String<max=u64::MAX> values, lmax + rmax can overflow: debug builds panic during compilation and release builds wrap to a much smaller maximum. Since Core bounds are used as safety facts, this should reject overflow rather than producing an unsound bound.
Useful? React with 👍 / 👎.
| fn type_decl_shape(&mut self, decl: &TypeDecl) -> Option<TypeShape> { | ||
| let coord = format!("{}.{}", self.resolved.coordinate, decl.name); | ||
| match &decl.body { | ||
| TypeExpr::Record(fields) => self.record_shape(&coord, &decl.name, fields, decl.span), |
There was a problem hiding this comment.
A declaration like type Box<T> = { value: String<max=8>, }; enters this record path and is lowered as a concrete Box, silently ignoring its type parameters. Generics are not part of the initial lowerable subset, so accepting them here can make later Box references mean a monomorphic Core type that the source did not declare.
Useful? React with 👍 / 👎.
| coord_hint: Option<String>, | ||
| ) -> Option<TypeShape> { | ||
| match ty { | ||
| TypeRef::Named { path, args } if args.is_empty() && path.len() == 1 => { |
There was a problem hiding this comment.
The source language includes scalar types such as Bool, I32, I64, U32, and U64, but they parse as TypeRef::Named and this branch only looks in user-declared named_types. A local record like type Input = { ok: Bool, }; is therefore rejected as UnresolvedType even though the new Core model and advertised subset include booleans and integers.
Useful? React with 👍 / 👎.
| }), | ||
| Expr::Int { value, .. } => Some(TypedValue { | ||
| expr: CoreExpr::Const(CoreValue::Int { | ||
| width: "I64".to_owned(), |
There was a problem hiding this comment.
For suffixed literals in otherwise lowerable predicates, such as where 1u32 == 1u32, the AST preserves the suffix but this lowering hardcodes every integer literal to I64. That changes the source's explicit integer width/sign in Core and can make typed integer fixtures compile to the wrong semantic value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6071fb9ed
ℹ️ 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".
| ) -> Option<CorePredicate> { | ||
| let left = self.check_expr(lhs, env)?; | ||
| let right = self.check_expr(rhs, env)?; | ||
| if comparable(&left.ty, &right.ty) { |
There was a problem hiding this comment.
When a where clause uses <, <=, >, or >= (for example where input.name < "z" over String<max=256>), this accepts the predicate as long as the operand shapes are compatible. The compiler-spine contract added here only includes equality predicates, and raw Core strings do not have an ordering policy, so this lowers source the initial subset should reject instead of reporting UnsupportedSourceShape/ExpectedPredicate.
Useful? React with 👍 / 👎.
| fields.insert(name.clone(), value.expr); | ||
| field_types.insert(name.clone(), value.ty); |
There was a problem hiding this comment.
For a record literal with a repeated field such as return { message: a, message: b };, these BTreeMap::insert calls silently overwrite the earlier value and type, so compile_to_core accepts and lowers invalid source instead of rejecting the duplicate field. This can hide author mistakes and produce Core that does not faithfully represent the source literal.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn string_value(value: &str) -> TypedValue { | ||
| let max = value.len() as u64; |
There was a problem hiding this comment.
For non-ASCII literals like "é", str::len() counts UTF-8 bytes rather than Unicode scalar values, while String<max=N> bounds are defined in scalar values. This makes a valid literal for String<max=1> look like max=2 and can reject valid programs or inflate concatenation bounds; use a scalar count such as value.chars().count() for the literal bound.
Useful? React with 👍 / 👎.
| let Some(shape) = self.type_decl_shape(&decl.source) else { | ||
| continue; | ||
| }; | ||
| self.core_types.insert(decl.name.clone(), shape.core_type()); |
There was a problem hiding this comment.
This stores each lowered type under the source-local name (HelloInput), but TypeShape::coord is the fully qualified Core coordinate (examples.hello@1.HelloInput) and that is what intents and record fields use as their type references. The resulting CoreModule contains type refs that cannot be resolved through core.types, so even the newly compiled fixture is internally inconsistent for consumers that walk Core type references.
Useful? React with 👍 / 👎.
| let input_shape = self.type_ref_shape(¶m.ty, param.span, None)?; | ||
| let output_shape = self.type_ref_shape(&source.returns, source.span, None)?; | ||
| let input_binding = LocalRef { | ||
| id: param.name.clone(), |
There was a problem hiding this comment.
Using param.name as the Core LocalRef.id makes the lowered Core change when an author only renames the intent parameter, even though alpha_name remains $arg0. The Core topic says local ids are compiler-owned and source binder spelling is not identity, so intent t(request: HelloInput) and intent t(input: HelloInput) should not produce different semantic local ids.
Useful? React with 👍 / 👎.
| let canonical = refine | ||
| .canonical | ||
| .clone() | ||
| .unwrap_or_else(|| "raw-utf8".to_owned()); |
There was a problem hiding this comment.
For a source type like String<max=128, canonical=nfc>, this copies the source spelling nfc directly into the Core string type. The Core schema only permits canonical values such as unicode-scalar-nfc and raw-utf8, so the compiler can emit an in-memory Core type that does not match the Core contract instead of translating or rejecting unsupported policies.
Useful? React with 👍 / 👎.
| )); | ||
| return None; | ||
| }; | ||
| let max = lmax + rmax; |
There was a problem hiding this comment.
With very large declared string bounds, e.g. concatenating two String<max=u64::MAX> values, lmax + rmax can overflow: debug builds panic during compilation and release builds wrap to a much smaller maximum. Since Core bounds are used as safety facts, this should reject overflow rather than producing an unsound bound.
Useful? React with 👍 / 👎.
| fn type_decl_shape(&mut self, decl: &TypeDecl) -> Option<TypeShape> { | ||
| let coord = format!("{}.{}", self.resolved.coordinate, decl.name); | ||
| match &decl.body { | ||
| TypeExpr::Record(fields) => self.record_shape(&coord, &decl.name, fields, decl.span), |
There was a problem hiding this comment.
A declaration like type Box<T> = { value: String<max=8>, }; enters this record path and is lowered as a concrete Box, silently ignoring its type parameters. Generics are not part of the initial lowerable subset, so accepting them here can make later Box references mean a monomorphic Core type that the source did not declare.
Useful? React with 👍 / 👎.
| coord_hint: Option<String>, | ||
| ) -> Option<TypeShape> { | ||
| match ty { | ||
| TypeRef::Named { path, args } if args.is_empty() && path.len() == 1 => { |
There was a problem hiding this comment.
The source language includes scalar types such as Bool, I32, I64, U32, and U64, but they parse as TypeRef::Named and this branch only looks in user-declared named_types. A local record like type Input = { ok: Bool, }; is therefore rejected as UnresolvedType even though the new Core model and advertised subset include booleans and integers.
Useful? React with 👍 / 👎.
| }), | ||
| Expr::Int { value, .. } => Some(TypedValue { | ||
| expr: CoreExpr::Const(CoreValue::Int { | ||
| width: "I64".to_owned(), |
There was a problem hiding this comment.
For suffixed literals in otherwise lowerable predicates, such as where 1u32 == 1u32, the AST preserves the suffix but this lowering hardcodes every integer literal to I64. That changes the source's explicit integer width/sign in Core and can make typed integer fixtures compile to the wrong semantic value.
Useful? React with 👍 / 👎.
1 participant
Summary
resolve_module,type_check,lower_core, andcompile_to_coreCompilerContextprofile/budget factsbounded-hello) to structured Core while keeping canonical bytes, exact digests, target lowering, and admission out of scopedocs/topics/compiler-spine/plus aligned topic-shelf updatesCloses #20
Verification
cargo xtask verifynpx markdownlint-cli2 CHANGELOG.md docs/README.md docs/topics/README.md docs/topics/compiler-spine/README.md docs/topics/compiler-spine/test-plan.md docs/topics/core-ir/README.md docs/topics/core-ir/test-plan.md docs/topics/semantic-validation/README.md docs/topics/semantic-validation/test-plan.md docs/topics/syntax/README.md docs/topics/syntax/test-plan.md