Skip to content

feat(syntax): Phase 1 — Edict lexer + minimal-v1 parser - #9

Merged
flyingrobots merged 20 commits into
mainfrom
feat/phase1-parser
Jun 20, 2026
Merged

feat(syntax): Phase 1 — Edict lexer + minimal-v1 parser#9
flyingrobots merged 20 commits into
mainfrom
feat/phase1-parser

Conversation

@flyingrobots

Copy link
Copy Markdown
Owner

What

First executable slice of Edict: a standalone Rust Cargo workspace with edict-syntax (lexer + AST + recursive-descent parser) for the edict.implementation/minimal-v1 surface. Turns the Phase 0 design baseline into something that actually parses.

Highlights

  • Workspace mirrors Wesley conventions (edition 2021, Apache-2.0), strict lints: forbid unsafe, deny clippy::all, warn clippy::pedantic.
  • Lexer (token.rs): hand-written, std-only, deterministic. Contextual-keyword identifiers (keywords usable after .), typed integer literals with suffixes, string escapes, refined-scalar/operator punctuation, line/block comments.
  • AST (ast.rs): minimal-v1 — module/imports/type decls (records + refined scalars)/intent + clauses/statements/expressions.
  • Parser (parser.rs): recursive descent with full operator precedence; package refs, imports (with digest), type decls, intents with profile/implements/basis/footprint/budget/where clauses, let/return, record shorthand.
  • First conformance fixture: fixtures/lang/bounds/bounded-hello.edict (positive fixture for EDICT-LANG-BOUNDS-001, valid dummy digest per the fixtures convention) parses to the expected tree.

Tests

8 green (6 lexer unit + 2 parser integration), zero warnings, cargo clippy clean under deny-all + pedantic.

Architecture notes

  • Edict is standalone (Wesley already extracted it + guards the boundary); not a GraphQL superset.
  • HOLMES/Watson/Moriarty are reused downstream (Wesley's wesley-holmes) at the assurance phase — a documented seam, not a parser dependency.

Scope / not in this PR

minimal-v1 only and intentionally partial: effect statements + else obstruction maps, if/branch-yield, bounded for, require/guarantee/assert, and the negative/relapse fixtures are follow-ups. Core IR lowering is gated on the edict.core/v1 CDDL (#3).

Grammar and Core schema remain unfrozen.

Standalone edict-syntax crate (Apache-2.0, edition 2021, strict lints:
forbid unsafe, deny clippy::all, warn pedantic). Hand-written std-only lexer
covering minimal-v1 tokens: contextual-keyword idents, typed integer literals
with suffixes, strings with escapes, refined-scalar/op punctuation, line/block
comments. 6 lexer unit tests green; clippy clean.

Edict is built in its own repo (Wesley already extracted it and guards the
boundary); it will reuse Wesley's HOLMES/Watson/Moriarty downstream at the
assurance phase, not as a parser dependency.
…o parses

Adds ast.rs (module/imports/type decls/intent/clauses/stmts/exprs for the
minimal-v1 subset) and parser.rs (recursive descent with full operator
precedence). The bounded hello example parses to the expected AST, driven by
fixtures/lang/bounds/bounded-hello.edict (positive fixture for
EDICT-LANG-BOUNDS-001, valid dummy digest per the fixtures convention).

8 tests green (6 lexer + 2 parser); clippy clean under deny(clippy::all) +
warn(pedantic).

Cool idea filed: editor syntax highlighting / Tree-sitter grammar (#7).
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@flyingrobots, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 54 minutes and 2 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 27a1bef3-7dcc-413c-b3bc-a408905656c3

📥 Commits

Reviewing files that changed from the base of the PR and between 9e7c1e9 and 1250ea2.

📒 Files selected for processing (19)
  • .github/workflows/ci.yml
  • CHANGELOG.md
  • README.md
  • crates/edict-syntax/src/ast.rs
  • crates/edict-syntax/src/lib.rs
  • crates/edict-syntax/src/parser.rs
  • crates/edict-syntax/src/token.rs
  • crates/edict-syntax/tests/common/mod.rs
  • crates/edict-syntax/tests/parse_control_flow.rs
  • crates/edict-syntax/tests/parse_hello.rs
  • crates/edict-syntax/tests/parse_keywords.rs
  • crates/edict-syntax/tests/parse_loops.rs
  • crates/edict-syntax/tests/parse_negative.rs
  • crates/edict-syntax/tests/parse_review_regressions.rs
  • crates/edict-syntax/tests/parse_variants.rs
  • docs/RETRO_phase1-parser.md
  • docs/TECHNICAL_EXPLANATION.md
  • fixtures/lang/effects/conditional-blob.edict
  • fixtures/lang/types/color-match.edict

Walkthrough

Introduces the edict-syntax Rust crate from scratch inside a new Cargo workspace. The crate provides a hand-written lexer (token.rs), a complete minimal-v1 surface AST (ast.rs), and a recursive-descent parser with precedence-climbing expression support (parser.rs). Integration tests, two .edict language fixtures, a GitHub Actions CI workflow, and a full technical explanation document are also added.

Changes

edict-syntax: new Rust crate — lexer, AST, parser, tests, fixtures, docs, CI

Layer / File(s) Summary
Workspace and CI scaffolding
Cargo.toml, crates/edict-syntax/Cargo.toml, .github/workflows/ci.yml
New Cargo workspace with shared lint rules (forbid unsafe, deny clippy pedantic/all) and a CI workflow running cargo fmt, cargo clippy --deny warnings, and cargo test --all-features.
Lexer: Span, tokens, LexError, lex()
crates/edict-syntax/src/token.rs
Defines Span, IntSuffix, TokenKind, Token, LexError, and lex(). Hand-written Lexer dispatches on byte class to lex identifiers, integers (with typed suffix and underscore placement rules), strings (with escape decoding), comments, and all operator/punctuation sequences; includes unit tests.
AST node definitions
crates/edict-syntax/src/ast.rs
Defines the complete surface AST: Module, PackageRef, Import/ImportKind, Decl, TypeDecl, TypeExpr, FieldDecl/FieldConstraint, BoundRef, TypeRef, ScalarRefine, IntentDecl, Param, IntentClause, Block, Stmt, ObstructionHandler/Arm/Target, BinOp, UnOp, Expr, and RecordEntry.
Crate entry point
crates/edict-syntax/src/lib.rs
Declares ast, parser, token modules and re-exports parse_module, ParseError, lex, IntSuffix, LexError, Span, Token, TokenKind.
Recursive-descent parser
crates/edict-syntax/src/parser.rs
Implements ParseError and parse_module. Parser cursor drives full grammar: module/package/import forms, type/intent declarations with field constraints and type refs, statement blocks with obstruction handlers, and expression precedence climbing including cursor-backtracking disambiguation between <type-args>(...) calls and < binary comparisons.
Language fixtures
fixtures/lang/bounds/bounded-hello.edict, fixtures/lang/effects/read-greeting.edict
bounded-hello.edict defines HelloInput/HelloReading types and a sayHello intent with bounded strings, where-constraint, and read-only profile. read-greeting.edict defines readGreeting with shape/lawpack/target imports, basis, budget, effect-else obstruction, and record return.
Integration tests
crates/edict-syntax/tests/parse_hello.rs, tests/parse_greeting.rs, tests/parse_negative.rs
parse_hello.rs asserts full AST conformance for the bounded-hello fixture, missing-semicolon rejection, and multi-part version acceptance. parse_greeting.rs asserts fixture parsing, generic-call vs comparison disambiguation, and obstruction-map with binders/payloads. parse_negative.rs asserts rejection of reserved declarations, missing package, invalid Bytes syntax, and unterminated strings.
Technical documentation
docs/TECHNICAL_EXPLANATION.md
Covers Edict glossary, compilation lifecycle, lexer internals, parser architecture, end-to-end AST example, error handling, security boundaries, concurrency constraints, dependency boundaries, and design trade-offs.

Sequence Diagram(s)

sequenceDiagram
    participant Dev as Developer
    participant parse_module
    participant lex as lex()
    participant Lexer
    participant Parser

    rect rgba(70, 130, 180, 0.5)
        note over Dev, parse_module: Lexing phase
        Dev->>parse_module: parse_module(src: &str)
        parse_module->>lex: lex(src)
        lex->>Lexer: run()
        Lexer->>Lexer: skip trivia (whitespace/comments)
        Lexer->>Lexer: dispatch on byte class (ident/int/string/punct)
        Lexer-->>lex: Vec<Token> + Eof
        lex-->>parse_module: Ok(Vec<Token>) or Err(LexError)
    end

    rect rgba(34, 139, 34, 0.5)
        note over parse_module, Parser: Parsing phase
        parse_module->>Parser: Parser::new(tokens)
        Parser->>Parser: parse_package_decl()
        Parser->>Parser: parse_imports() loop
        Parser->>Parser: parse_decl() → type | intent
        Parser->>Parser: parse_intent_clauses() + parse_block()
        Parser->>Parser: parse_stmt() → let/effect/require/guarantee/assert/return
        Parser->>Parser: parse_expr() via precedence climbing
        Parser->>Parser: try_type_call_args() backtrack on < vs generic
        Parser-->>parse_module: ast::Module or ParseError
    end

    parse_module-->>Dev: Result<Module, ParseError>
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

🦀 A language is born from the void today,
tokens align, and AST nodes hold sway.
The parser climbs precedence, left to right,
<T>() vs < — disambiguated right.
Obstructions are handled, spans never lie,
unsafe forbidden beneath the compiler's eye. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly describes the main change: introducing the Edict lexer and minimal-v1 parser as Phase 1 implementation.
Description check ✅ Passed The description comprehensively explains the changeset, covering workspace structure, lexer/AST/parser components, test coverage, and architecture notes with explicit scope boundaries.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@flyingrobots

Copy link
Copy Markdown
Owner Author

🧑‍⚖️ Self-review (Code Lawyer) — PR #9

Reviewed origin/main...HEAD. Intent (a minimal-v1 lexer+parser that parses the hello example) is met; 8 tests green, clippy clean. But a strict pass finds 1 real correctness bug and process/coverage gaps.

0 Critical · 2 Major · 4 Minor · 1 Nit

# Sev File:line Type Issue Mitigation
M1 🟠 Major crates/edict-syntax/src/token.rs:293 Correctness String lexing does char::from(self.bump()) — that's a byte→Latin-1 conversion, not a UTF-8 decode. A non-ASCII string ("café") is corrupted into mojibake, violating "String decodes to Unicode scalar values." Proven: 0xC3Ã (U+00C3). Accumulate string bytes into a Vec<u8> (escapes push their bytes too) and String::from_utf8(...) at the close quote; add a fixture with non-ASCII content.
M2 🟠 Major .github/workflows/ (absent) Process No CI runs cargo test/cargo clippy/fmt. Nothing guards the green state (house rule: CI must cover all suites). Add a GitHub Actions workflow: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test --workspace.
m1 🟡 Minor token.rs:225 Spec fidelity Integer underscore placement unchecked — lexer silently accepts trailing/edge _ (e.g. 1_); spec says underscores only between digits. Reject _ at start/end/adjacent; add a negative fixture.
m2 🟡 Minor parser.rs:159-167 Coverage Package version parses a single token only; grammar allows version = digit {digit|.|-|_|letter} so @1.2.3-beta fails. Lex/parse a full version token; fixture a multi-part version.
m3 🟡 Minor Cargo.toml:18 House rule clippy::pedantic is warn, but the house rule is warnings-as-errors. Set pedantic to deny (or -D warnings in CI) once the surface is pedantic-clean.
m4 🟡 Minor fixtures/, tests/ Coverage No negative/relapse fixtures or semantic-validation layer yet (reject migration/projection, naked String, clause-requiredness, etc.). Scoped out, but the spec leans on negative fixtures early. Track as a follow-up; add the relapse-zoo parse-reject corpus next.
n1 🔵 Nit ast.rs Consistency Eq derivation is inconsistent: Expr/Stmt/Module/Decl/IntentClause/etc. are PartialEq-only though all fields are Eq-able. Derive Eq uniformly (no floats in the AST).

@codex — please confirm this triage and flag any Major I've missed in the diff.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a5bf84067

ℹ️ 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".

Comment thread crates/edict-syntax/src/token.rs Outdated
}
Some(_) => {
// copy one UTF-8 byte; multi-byte chars pass through intact
out.push(char::from(self.bump()));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Decode string literals as UTF-8

For any string literal containing non-ASCII UTF-8, this pushes each raw byte as its own Unicode scalar, so "café" becomes "café"; the AST no longer contains the source string required by the spec, and later length/hash checks over string values will be wrong. Decode a UTF-8 character or accumulate bytes and validate with String::from_utf8 instead of converting each byte with char::from.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +190 to +191
} else if self.eat_kw("capability") {
ImportKind::Capability

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject capability imports in minimal-v1

When the source contains use capability ..., this branch accepts it as an ImportKind::Capability, but the v1 import grammar only includes shape/lawpack/target/core and the spec calls use capability rejected v1 syntax. Because it is parsed into a normal module import here, a minimal-v1 compile can silently admit a non-v1 capability surface form that should fail during parsing.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
(Some(self.package_ref()?), None)
};

let digest = if self.eat_kw("digest") { Some(self.string()?) } else { 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.

P1 Badge Validate digest literals in import clauses

For any digest clause, this accepts arbitrary strings such as digest "not-a-digest" or the prose "sha256:...", even though the grammar's digest-lit is sha256: plus exactly 64 hex characters and only real digest resolution is deferred. That lets imports appear digest-locked in the AST while lacking a syntactically valid lock value.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +159 to +163
TokenKind::Int { value, .. } => {
self.idx += 1;
value
}
TokenKind::Ident(s) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Parse full package versions

Package refs can use versions like 1.2.3-beta, but this match consumes exactly one token, so the parser stops after 1 and errors on the leftover .; conversely @beta is accepted even though versions must start with a digit. This rejects valid imports/packages and admits invalid coordinates before dependency resolution.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +380 to +384
let max = match self.bound_ref()? {
BoundRef::Int(n) => n,
BoundRef::Coord(_) => {
return self.err("Bytes max must be an integer or digest-locked bound ref");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Allow digest-locked bound refs for Bytes max

The grammar uses bound-ref for Bytes<max=...> just like String/List, and the implementation plan says bounds may be literal or digest-locked refs, but this branch rejects every coordinate bound. Any valid type such as Bytes<max=limits.payloadBytes> fails to parse while the equivalent string/list bounds parse.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +565 to +568
while *self.peek() == TokenKind::Dot {
self.idx += 1;
let field = self.ident()?;
e = Expr::Field { base: Box::new(e), field, span: Span::new(start, self.prev_end()) };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Parse postfix call suffixes

For valid expressions such as len(input.name) or prelude calls used in guards, postfix only consumes field access and then returns, leaving the ( to trigger a parse error at the surrounding statement. The v1/minimal-v1 grammar includes call suffixes on postfix expressions, so this rejects any program that uses the closed prelude or pure helper calls.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
fn type_decl(&mut self) -> Result<TypeDecl, ParseError> {
let start = self.peek_span().start;
self.expect_kw("type")?;
let name = self.ident()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Enforce upper-case type declaration names

type declarations are defined with upper-ident, but this accepts any identifier, so type foo = String<max=1>; parses and enters the AST as a type declaration. That lets syntactically invalid type coordinates proceed to later phases where they can collide with value/import namespaces rather than being rejected by the parser.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
…ardening)

- M1: lex strings as raw bytes + decode UTF-8 at close quote (was byte->Latin-1,
  corrupting non-ASCII). Adds a multibyte fixture.
- M2: add GitHub Actions CI (fmt --check, clippy -D warnings, test --workspace).
- m1: enforce integer underscores only between digits (reject 1_, 1__0).
- m2: parse multi-part package versions (@1.2.3-beta) via span adjacency.
- m3: promote clippy::pedantic to deny; clippy clean under deny-all + pedantic.
- m4: negative parse-reject fixtures (parse_negative.rs); full semantic
  validation + relapse zoo tracked in #10.
- n1: derive Eq uniformly across the AST.

15 tests green; fmt clean; clippy clean.
@flyingrobots

Copy link
Copy Markdown
Owner Author

✅ Self-review findings resolved (6fdfbe6)

# Sev Fix
M1 Major String lexing now buffers raw bytes and String::from_utf8s at the close quote — non-ASCII no longer corrupted. New multibyte_utf8_string_is_not_corrupted test (café — naïve 🦀).
M2 Major Added .github/workflows/ci.yml: cargo fmt --check · cargo clippy --workspace --all-targets -- -D warnings · cargo test --workspace.
m1 Minor Integer underscores enforced between digits only; 1_/1__0 reject.
m2 Minor Multi-part package versions (@1.2.3-beta) parse via span-adjacency.
m3 Minor clippy::pedantic promoted to deny; clippy clean under deny-all + pedantic.
m4 Minor Negative parse-reject fixtures added (tests/parse_negative.rs); full semantic-validation layer + relapse zoo tracked in #10.
n1 Nit Eq derived uniformly across the AST.

Post-fix: 15 tests green (was 8), cargo fmt --check clean, cargo clippy clean under deny-all + deny-pedantic.

@codex — addressed; please re-confirm. CI will run on this push once the workflow is on the branch.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6fdfbe6293

ℹ️ 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".

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +234 to +235
} else if self.eat_kw("capability") {
ImportKind::Capability

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject capability imports in v1 parser

When source contains use capability ..., this branch accepts it as an import even though the v1 import grammar only includes shape/lawpack/target/core and the AST comment says capability imports are product sketches rejected by v1. Because this crate has no later semantic-validation pass to reject it, invalid v1 modules can parse successfully and be treated as supported syntax.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +246 to +247
let digest = if self.eat_kw("digest") {
Some(self.string()?)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Validate digest clauses as digest literals

In a digest clause this accepts any string literal, so inputs like digest "not-a-digest" or digest "sha256:dead" parse even though the language grammar and fixture README require sha256: followed by exactly 64 hex characters; digest-lock validation is only about matching a real artifact, not accepting malformed source. That lets malformed lock coordinates reach downstream identity/bundle handling as parsed imports.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +460 to +463
let max = match self.bound_ref()? {
BoundRef::Int(n) => n,
BoundRef::Coord(_) => {
return self.err("Bytes max must be an integer or digest-locked bound ref");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve coordinate bounds for Bytes max

For valid declarations such as type T = { b: Bytes<max=limits.payloadBytes>, };, bound_ref() has already parsed the qualified bound but this arm immediately rejects it and TypeRef::BytesTy can only store a u64. The grammar defines bytes-refine in terms of bound-ref, so digest-locked/named byte limits cannot be parsed even though the same bound form works for String/List/Map.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +199 to +200
TokenKind::Int { value, suffix } => {
s.push_str(&value);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve underscores in package versions

For package refs with numeric underscore segments, for example package a.b@1_2;, this appends the normalized integer token value and the AST records version 12 instead of the source version 1_2. Since package refs are identity/version coordinates and the grammar allows _ in version, this can silently alias distinct package versions before resolution.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
fn postfix(&mut self) -> Result<Expr, ParseError> {
let start = self.peek_span().start;
let mut e = self.primary()?;
while *self.peek() == TokenKind::Dot {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Support call suffixes in postfix expressions

Postfix parsing only consumes field access, so valid call forms such as len(input.name), basisRef.read(), or echo.ref<T>(id) leave the ( or type-arg call suffix unconsumed and then fail when the enclosing statement/where clause expects a delimiter. The language grammar and crate scope include call and type-call postfixes, so pure helper calls and target/lawpack calls cannot be parsed.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated
fn type_decl(&mut self) -> Result<TypeDecl, ParseError> {
let start = self.peek_span().start;
self.expect_kw("type")?;
let name = self.ident()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Enforce upper-ident for type declarations

This accepts any identifier as a type name, so a declaration like type foo = { }; parses even though the v1 grammar requires upper-ident for type-decl. Since the invalid spelling is stored unchanged in the AST and there is no syntactic follow-up pass here, malformed type declarations can enter later compilation stages as if they were valid.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
…ee/assert

Next minimal-v1 parser slice:
- Expr::Call with call-suffix and type-call-suffix; the leading angle bracket is
  disambiguated from comparison by backtracking (type-call only when the type
  args are immediately followed by an open paren).
- Effect-failure mapping: obstruction target (coordinate + optional payload),
  single-shorthand and full map form with per-failure binders; on let rhs and
  bare effect statements.
- require (mandatory else), guarantee (optional else), assert (no else).
- Fixture fixtures/lang/effects/read-greeting.edict parses (calls, type args,
  effect-else, multi-import). Tests cover generics-vs-comparison disambiguation
  and obstruction maps with binders + payloads.

18 tests green; fmt clean; clippy clean under deny-all + pedantic.

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

Actionable comments posted: 9

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

Inline comments:
In @.github/workflows/ci.yml:
- Around line 20-24: Replace all floating tag references with immutable
full-length commit SHAs for the three actions: actions/checkout,
dtolnay/rust-toolchain, and Swatinem/rust-cache. Additionally, add
persist-credentials: false to the actions/checkout action configuration to
prevent the GITHUB_TOKEN from being persisted in .git/config. You will need to
look up the corresponding commit SHAs for each action's version tag (`@v4`,
`@stable`, `@v2`) from their respective GitHub repositories and update all three
uses statements accordingly.
- Around line 21-30: The CI workflow only tests against the stable Rust
toolchain, but Cargo.toml declares an MSRV of 1.85.0, creating a gap where code
could pass stable's checks while breaking on the MSRV floor. Add a matrix
strategy to the workflow that includes both rust-version 1.85.0 and stable,
ensuring all checks (Format, Clippy, and Test) run against both versions. Update
the dtolnay/rust-toolchain step to reference the matrix variable instead of
hardcoding stable.

In `@crates/edict-syntax/src/lib.rs`:
- Around line 3-8: The crate-level documentation comment in lib.rs starting at
line 3 currently advertises features like enums and if expressions that are not
yet implemented by the parser/AST in this PR. Review the actual parser and AST
implementations in this crate to determine which features are truly supported
today, then update the scope documentation to only list the implemented
features. Move any unimplemented features (such as enums, if expressions,
variants, maps, regex-lite constraints, etc.) to a separate section clearly
marked as coming later or planned, so the documentation accurately reflects the
current state of the parser rather than future aspirational features.

In `@crates/edict-syntax/src/parser.rs`:
- Around line 353-361: In the integer token parsing block where TokenKind::Int
is matched, the error span is captured using self.peek_span() after incrementing
self.idx, which causes the diagnostic to highlight the wrong token location.
Move the span capture to before the self.idx increment (store it in a variable),
then use the stored span value in the ParseError within the map_err closure
instead of calling self.peek_span() again. This ensures the error diagnostic
points to the invalid integer token itself, not the next token.
- Around line 556-623: The stmt() function currently accepts any expression for
Stmt::Effect in the final else branch and allows maybe_else_handler() for any
let value expression, but both should only permit call expressions. After
parsing the value expression in the let statement branch, verify it's a call
expression before calling maybe_else_handler(), rejecting non-call expressions
with an appropriate error. Similarly, in the final else branch where you parse
the effect statement, validate that the parsed expression is a call expression
rather than accepting arbitrary expressions, and reject non-call expressions
accordingly.

In `@crates/edict-syntax/tests/parse_hello.rs`:
- Around line 90-93: The missing_semicolon_is_a_parse_error test function
currently only asserts that parse_module returns any error via is_err(), which
means it could pass for unrelated parse failures. Instead of just checking
is_err(), inspect the actual error returned to verify that it is specifically
related to the missing semicolon. Examine what error information parse_module
returns (such as error message or error kind) and add an assertion that confirms
the error is specifically about the missing semicolon, not some other parsing
issue.

In `@docs/TECHNICAL_EXPLANATION.md`:
- Line 87: The markdown file has markdownlint MD022/MD031 violations where
headings and fenced code blocks are not properly surrounded by blank lines. Add
a blank line before and after each heading (such as "The Bootstrap Phase (Static
Assembly)") and each fenced code block throughout the document to comply with
the markdown lint rules. Apply this formatting to all the affected lines
mentioned in the comment: lines 87, 93, 116, 149, 151, 156, 159, 207, 216, 217,
263, 310, 311, 336, 477, 479, 484, 489, 491, 496, 535, 540, and 545.
- Line 103: Replace all absolute machine-local file:// URLs in the documentation
with repository-relative links that work for everyone. Specifically, convert
links like file:///Users/james/git/edict/crates/edict-syntax/src/lib.rs to
relative paths (e.g., ../crates/edict-syntax/src/lib.rs or using markdown
relative link syntax appropriate for the documentation structure). This applies
to all occurrences at lines 103, 150, and 264 in the TECHNICAL_EXPLANATION.md
file where links to files like lib.rs and the parse_module function are
referenced.
- Around line 294-302: The operator precedence explanation in the numbered list
is inverted from the actual parser implementation. The current text states that
logic_or binds tightest at the top, but logic_or actually has the loosest
binding (lowest precedence). Remove the phrase "binds tightest at the top of the
expression hierarchy" and instead clarify that logic_or is the loosest binding
level, with each subsequent level having progressively higher precedence down to
primary expressions which bind tightest. This correction aligns the
documentation with the actual parser's operator precedence handling.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b06f5173-edbc-4542-9163-fdb75f9188eb

📥 Commits

Reviewing files that changed from the base of the PR and between 3fd9938 and 9e7c1e9.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • .github/workflows/ci.yml
  • Cargo.toml
  • crates/edict-syntax/Cargo.toml
  • crates/edict-syntax/src/ast.rs
  • crates/edict-syntax/src/lib.rs
  • crates/edict-syntax/src/parser.rs
  • crates/edict-syntax/src/token.rs
  • crates/edict-syntax/tests/parse_greeting.rs
  • crates/edict-syntax/tests/parse_hello.rs
  • crates/edict-syntax/tests/parse_negative.rs
  • docs/TECHNICAL_EXPLANATION.md
  • fixtures/lang/bounds/bounded-hello.edict
  • fixtures/lang/effects/read-greeting.edict
📜 Review details
🧰 Additional context used
🪛 markdownlint-cli2 (0.22.1)
docs/TECHNICAL_EXPLANATION.md

[warning] 87-87: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 116-116: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 149-149: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 151-151: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 156-156: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 159-159: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 207-207: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 216-216: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 217-217: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 263-263: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 310-310: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 311-311: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 336-336: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 477-477: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 479-479: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 484-484: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 489-489: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 491-491: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 496-496: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)


[warning] 535-535: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 540-540: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)


[warning] 545-545: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🪛 zizmor (1.25.2)
.github/workflows/ci.yml

[warning] 20-20: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)


[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)


[error] 21-21: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)


[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)


[info] 21-21: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step

(superfluous-actions)

🔇 Additional comments (8)
Cargo.toml (1)

1-19: LGTM!

crates/edict-syntax/Cargo.toml (1)

1-14: LGTM!

crates/edict-syntax/src/token.rs (1)

1-556: LGTM!

crates/edict-syntax/src/ast.rs (1)

1-320: LGTM!

crates/edict-syntax/tests/parse_greeting.rs (1)

1-132: LGTM!

fixtures/lang/bounds/bounded-hello.edict (1)

1-23: LGTM!

fixtures/lang/effects/read-greeting.edict (1)

1-22: LGTM!

crates/edict-syntax/tests/parse_negative.rs (1)

1-42: LGTM!

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread crates/edict-syntax/src/lib.rs Outdated
Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/tests/parse_hello.rs
Comment thread docs/TECHNICAL_EXPLANATION.md
Comment thread docs/TECHNICAL_EXPLANATION.md Outdated
Comment thread docs/TECHNICAL_EXPLANATION.md Outdated
…ment

Completes the conditional surface of minimal-v1 (SPEC if-expr,
effect-branch-expr, if-stmt):

- Pure ternary `if p then a else b` parses at the top of the expression
  precedence chain (expr = if-expr), so it nests anywhere an expr is legal.
- Effectful branch-yield `if p { ...; yield a; } else { ...; yield b; }` is
  parsed only in let-rhs position; disambiguated from the ternary by what
  follows the predicate (then vs open brace). Each branch must end in yield.
- Statement-level if/else-if/else control flow (Stmt::If + ElseClause),
  with chained else-if modelled as ElseClause::If.

Fixture fixtures/lang/effects/conditional-blob.edict mirrors the spec's
conditional-effect example (branch-yield wrapping a nested effect-else let).
New parse_control_flow.rs covers ternary, nested ternary, branch-yield,
the missing-yield rejection, else-if chaining, and the fixture.

24 tests green; fmt clean; clippy clean under deny-all + pedantic.
Adds the statically bounded loop (SPEC for-stmt):

  for ident in expr bounded bound-ref block

The `bounded` cardinality is mandatory and may be an integer literal or a
digest-locked coordinate (BoundRef::Int | BoundRef::Coord) — every loop must
carry a provable maximum iteration count (EDICT-LANG-BOUNDS).

parse_loops.rs covers the integer bound, the coordinate bound, and rejection
of an unbounded `for` (no `bounded` clause).

27 tests green; fmt clean; clippy clean under deny-all + pedantic.
Adds the sum-type surface of minimal-v1 (SPEC enum-decl, variant-type,
variant-lit, match-expr):

- `enum Name { CASE, ... }` top-level declaration (Decl::Enum); cases are
  payload-free. Enum-case *selection* stays plain field access
  (Qual.Enum.CASE), unchanged.
- `type T = variant { Case, Case(Payload), ... }` (TypeExpr::Variant); each
  case carries an optional payload type.
- Variant constructor literals `Qual.Type::Case` / `Qual.Type::Case(expr)`
  via a `::` postfix suffix; the constructor head must be a pure dotted path.
- `match scrutinee { Case (binder)? => expr, ... }` (Expr::Match), at least
  one arm required; intercepted as a primary so it nests as any expression.

Fixture fixtures/lang/types/color-match.edict combines an enum, a variant
type, a match, and a variant literal. parse_variants.rs covers each form
plus the empty-match rejection.

34 tests green; fmt clean; clippy clean under deny-all + pedantic.
- docs/RETRO_phase1-parser.md: what shipped (parsed surface by grammar
  production), key decisions (contextual keywords, ternary-vs-branch-yield
  disambiguation, backtracking type-calls, `::` variant detection), the UTF-8
  bug found and fixed, deferred scope (literals, const/fn, record-stmt, all
  semantic validation → #10), and next steps (Core IR lowering #3).
- CHANGELOG: record the Phase 1 executable slice under Added.

34 tests green; fmt clean; clippy clean under deny-all + pedantic.
@flyingrobots

Copy link
Copy Markdown
Owner Author

🏁 Phase 1 wrap — minimal-v1 parser complete (to the planned scope)

Carried the branch from the readGreeting slice through the rest of the named
minimal-v1 surface, test-first, one coherent commit each:

Commit Slice
a78eb97 if family — ternary if … then … else, effectful branch-yield (let-rhs only), if/else if/else control flow
dfe4146 bounded forfor … in … bounded <bound> { … }, integer or coordinate bound; unbounded loop rejected
894a693 sum typesenum decls, variant types (with payloads), variant literals (Qual.Type::Case(payload)), match
eac03b8 retro + CHANGELOGdocs/RETRO_phase1-parser.md

State: 34 tests green (was 18), cargo fmt --check clean, clippy clean
under deny-all + pedantic, CI on the branch.

Honestly deferred (documented in the retro, not silently dropped): const/fn
decls, record semantic-effect statements, list/map/bool/unit/digest(...)
literals, and the entire semantic-validation layer (naked unbounded scalars,
clause requiredness, reserved-word rejection, exhaustive match/obstruction
coverage, loop-bound provability) — tracked in #10. Core IR lowering is #3.

The frontend now parses both README worked examples plus conditional effects,
bounded loops, and sum types — enough shape to start lowering. Ready for you to
land #9, or I can keep closing the deferred surface gaps.

@flyingrobots

Copy link
Copy Markdown
Owner Author

🧑‍⚖️ Strict review (Code Lawyer) — origin/main...HEAD

Pre-flight: worktree clean, fetched, 0 behind / 12 ahead. The branch meets its
intent (a minimal-v1 lexer + parser; both README examples plus conditional
effects, bounded loops, and sum types parse; 34 tests green, CI green). A
pedantic pass nonetheless finds a real spec-conformance bug and several quality
gaps.

1 Major · 4 Minor · 3 Nit

# Sev File:line Type Issue Mitigation
M1 🟠 Major crates/edict-syntax/src/parser.rs primary() Spec conformance Keywords are not reserved as bare identifiers. primary() maps any Ident to Expr::Ident; there is no keyword set. Proven: let x = then;, let x = else;, let x = yield; all parse; let x = a + if c then b else d; misparses (if consumed as an identifier → misleading expected Semi, found Ident("c")); let match = a; binds match. Violates SPEC Edict Language v1 §1510 (“Keywords are reserved as bare identifiers but may appear after . as member names”) and undercuts this branch’s own negative-fixture discipline. Add is_keyword(&str) covering structural/clause/reserved words; reject a bare keyword Ident in value/binder position (not reached via .); keep ref.ensure(...) / history.event.record(...) legal. Add negative fixtures.
m1 🟡 Minor crates/edict-syntax/src/lib.rs:5-8 Doc accuracy Crate doc says “Variants … come later behind capability flags” — but this PR adds variant parsing — and lists “pure first-order functions” though fn is not parsed. Update the module doc to match the implemented surface (variants in; const/fn/list/map/record-stmt deferred).
m2 🟡 Minor parser.rs if_ternary / let_rhs Duplication The pure-ternary Expr::If construction is implemented twice (in if_ternary() and the then-branch of let_rhs()). Share one tail helper finish_ternary(pred, start).
m3 🟡 Minor tests/parse_{control_flow,loops,variants}.rs Test duplication body() and the first_intent/first_let_value/first_stmt helpers are re-declared near-identically across three test files. Extract tests/common/mod.rs.
m4 🟡 Minor docs/TECHNICAL_EXPLANATION.md:130,136,561 Hygiene Trailing whitespace; git diff --check fails (would trip a pre-commit whitespace hook). sed -i '' -E 's/[[:space:]]+$//' the file.
n1 🔵 Nit parser.rs match_expr Diagnostics The arms.is_empty() check runs after expect(&RBrace), so the empty-match error span points past }. Capture the brace span / check before consuming.
n2 🔵 Nit fixtures/lang/types/color-match.edict Fixture clarity enum Channel is declared but never referenced (there only for enum-parser coverage); reads as dead. Reference it, or annotate // parser-coverage only.
n3 🔵 Nit CHANGELOG.md (+1 line at 81 cols), docs/TECHNICAL_EXPLANATION.md (107 lines >80) MD style Inconsistent with the repo’s 80-col prose wrap (RETRO table rows >80 are inherent to tables — acceptable). Rewrap the CHANGELOG line; pick a wrap policy for the technical guide.

Headline: M1 is the one that bites — a parser that accepts let x = else; is letting slop through and emitting misleading diagnostics, against an explicit lexical rule. Cheap to fix with a keyword guard + negative fixtures. The rest is quality debt.

@codex — please confirm this triage and flag any Major I’ve missed in the diff.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eac03b8885

ℹ️ 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".

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +200 to +201
TokenKind::Int { value, suffix } => {
s.push_str(&value);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve underscores in package versions

When a package version contains an underscore between digits, this appends TokenKind::Int.value, but the lexer has already stripped underscores for numeric literals, so package a.b@1_2; records version 12 instead of 1_2; versions such as 1_alpha also lex as invalid even though the v1 version grammar allows _. Package refs are identity-bearing, so version parsing needs to preserve the raw adjacent source spelling rather than reuse normalized integer values.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +516 to +517
BoundRef::Coord(_) => {
return self.err("Bytes max must be an integer or digest-locked bound ref");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Accept coordinate bounds on Bytes refinements

The v1 grammar defines bytes-refine as <max=bound-ref> and bound-ref includes qualified identifiers, but this branch rejects every BoundRef::Coord, so a valid shared bound such as type Raw = Bytes<max=limits.maxBytes>; fails to parse. This also contradicts the diagnostic text saying digest-locked bound refs are allowed, so BytesTy needs to carry the same bound-ref shape as String/List/Map rather than only u64.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +247 to +248
let digest = if self.eat_kw("digest") {
Some(self.string()?)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Validate digest literals before accepting locks

When a digest clause is present, this accepts any string literal, so use lawpack c.d@1 digest "not-a-digest" as l; parses as a locked import even though the grammar requires a sha256: digest with 64 hex characters. Import digests feed identity/provenance locking, so malformed or placeholder locks should be rejected at the syntax boundary rather than represented as valid Some(digest).

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +235 to +236
} else if self.eat_kw("capability") {
ImportKind::Capability

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject capability imports in v1

This branch explicitly accepts use capability ..., but the v1 import grammar only allows shape/lawpack/target/core imports and the AST comment itself notes capability imports are product-sketch syntax rejected by v1. As a result package a.b@1; use capability c.d@1 as cap; is accepted as minimal-v1 source instead of being rejected before downstream phases see an unsupported import kind.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +306 to +307
self.expect_kw("type")?;
let name = self.ident()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Enforce upper-ident productions

The v1 grammar uses upper-ident for type declarations and enum/variant/match case names, but these sites call the generic identifier parser, so syntactically invalid sources such as type foo = { x: String<max=1>, }; and enum E { red } parse successfully. If later lowering assumes spec-shaped names, accepting lowercase declarations/cases here lets non-conforming modules through the parser.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs
Resolves self-review findings M1, m2, n1.

M1 (Major, spec conformance — SPEC Edict Language v1 §1510): keywords were
not reserved as bare identifiers, so `let x = then;`, `let x = else;`, and
`let x = yield;` all parsed, and `a + if c then b else d` silently swallowed
`if` as an identifier. Add `is_keyword` and reject a reserved word in value
position (`primary`) and in binder position (new `binder`, used for `let` and
parameter names). Member names after `.` stay legal (§1511), and the import
kinds (`shape`/`lawpack`/`target`/`core`/`capability`) and prelude values
(`none`/`some`/`len`/`hash`) are deliberately *not* reserved so aliases and
`none<T>()` still parse.

m2: the pure-ternary `Expr::If` construction is no longer duplicated between
`if_ternary` and `let_rhs` — both share `ternary_tail`.

n1: the empty-`match` check now runs before consuming `}`, so the diagnostic
points at the empty body.

New tests/parse_keywords.rs covers bare-value and binder rejection, the
non-operand ternary, keyword member names (`history.event.record(...)`), and
prelude constructors. Shared tests/common/mod.rs introduced. 39 tests green;
fmt clean; clippy clean under deny-all + pedantic.
- m1: rewrite the `edict-syntax` crate doc to match the implemented surface
  (variants/enums/match are parsed; `fn`/`const`/`record`/list/map/unit and
  semantic validation are deferred) — it had said variants "come later".
- m4: strip trailing whitespace in docs/TECHNICAL_EXPLANATION.md (was failing
  `git diff --check`).
- n2: reference `enum Channel` from the `color-match` fixture (a `Tinted(Channel)`
  variant case) so it is no longer declared-but-unused.
- n3: rewrap the over-length CHANGELOG line to ≤80 cols and refresh stale test
  counts (34 → 39) in the CHANGELOG and retro.

39 tests green; fmt clean; clippy clean under deny-all + pedantic; whole-PR
`git diff --check` clean.
@flyingrobots

Copy link
Copy Markdown
Owner Author

✅ Self-review findings resolved (3c716fa, ad940f0)

# Sev Resolution
M1 🟠 Major Keywords are now reserved as bare identifiers. New is_keyword + a binder helper reject reserved words in value position (primary) and in let/param binder position. Re-probed: let x = then;, let x = else;, let match = a; now reject; history.event.record(value) (§1511 member access) and none<T>() (prelude) still parse. Import kinds (shape/…) and prelude values (none/some/len/hash) are deliberately excluded so aliases and constructors keep working. Covered by tests/parse_keywords.rs.
m1 🟡 Minor lib.rs crate doc rewritten to match the implemented surface (variants/enums/match in; fn/const/record/list/map/unit + semantic validation deferred).
m2 🟡 Minor Ternary construction de-duplicated via a shared ternary_tail (was duplicated in if_ternary and let_rhs).
m3 🟡 Minor Shared tests/common/mod.rs (body/parse_ok/intent_of); parse_control_flow/parse_loops/parse_variants refactored onto it.
m4 🟡 Minor Trailing whitespace stripped from docs/TECHNICAL_EXPLANATION.md; whole-PR git diff --check clean.
n1 🔵 Nit Empty-match check moved before consuming } — error span now points at the empty body.
n2 🔵 Nit enum Channel referenced from the color-match fixture (Tinted(Channel) case) — no longer dead.
n3 🔵 Nit Over-length CHANGELOG line rewrapped to ≤80; stale test counts refreshed (34 → 39).

Post-fix: 39 tests green (was 34), cargo fmt --check clean, clippy clean under deny-all + pedantic, whole-PR git diff --check clean.

@codex — all eight addressed; please re-confirm.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad940f0fc9

ℹ️ 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".

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +297 to +298
} else if self.eat_kw("capability") {
ImportKind::Capability

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject capability imports in v1 parser

When a source file contains use capability c.d@1 as c;, this branch accepts it and the import completes normally. The v1 grammar only admits shape, lawpack, target, and core imports, and the spec marks use capability as non-v1/rejected, so accepting it here lets out-of-profile source pass the syntax phase.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +576 to +579
let max = match self.bound_ref()? {
BoundRef::Int(n) => n,
BoundRef::Coord(_) => {
return self.err("Bytes max must be an integer or digest-locked bound ref");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve coordinate bounds for Bytes refinements

For a valid bounded byte type such as Bytes<max=limits.maxBytes>, bound_ref() returns BoundRef::Coord(_) and this branch reports a parse error. The grammar defines bytes-refine in terms of bound-ref just like String, List, and Map, so digest-locked/profile-provided byte bounds become unparsable even though the error message says they are allowed.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
let mut arms = Vec::new();
while *self.peek() != TokenKind::RBrace {
let start = self.peek_span().start;
let failure = self.ident()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject reserved failure names in obstruction maps

If an obstruction map arm uses a reserved keyword as the failure coordinate, for example else { else => domain.Oops }, ident() accepts it and the parser returns an ObstructionArm. Failure coordinates are required to be non-keyword identifiers, so this admits an AST that cannot correspond to a valid target failure map and should be rejected at the syntax boundary.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
fn for_stmt(&mut self) -> Result<Stmt, ParseError> {
let start = self.peek_span().start;
self.expect_kw("for")?;
let var = self.ident()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject reserved words as loop binders

When a loop is written with a keyword binder, e.g. for match in input.items bounded 1 { ... }, this ident() call accepts the name even though match is rejected everywhere as a bare value identifier. That leaves a syntactically accepted binding that cannot be referenced and misses the same keyword-reservation check now used for let and parameters.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated
if *self.peek() == TokenKind::RBrace {
return self.err("effect branch block must end with `yield <expr>;`");
}
stmts.push(self.stmt()?);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject return statements inside effect-yield blocks

Inside a branch-yield expression, stmt() allows return, so let x = if p { return input; yield a; } else { yield b; }; parses as an IfYield containing an early intent return. Effect-yield blocks are supposed to terminate by yielding the branch value, and the language disallows return there, so this creates an AST shape later stages should never have to handle.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
};

let digest = if self.eat_kw("digest") {
Some(self.string()?)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Validate import digest literals

An import such as use lawpack x.y@1 digest "not-a-digest" as x; is accepted because the digest clause stores any string literal. The grammar requires a digest-lit with sha256: plus exactly 64 hex characters, so invalid or placeholder locks can enter the AST and make later dependency resolution operate on non-canonical identities.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +1212 to +1214
TokenKind::Ident(name) if is_keyword(&name) => self.err(format!(
"keyword `{name}` is reserved and cannot be used as a bare identifier"
)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Parse digest value literals before keyword rejection

A valid value literal such as digest("sha256:0000000000000000000000000000000000000000000000000000000000000000") reaches the reserved-keyword branch and is rejected before the parser can see the following parentheses. Since digest-value-lit is a primary expression in the v1 grammar, this prevents source from constructing digest values at all.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad940f0fc9

ℹ️ 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".

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +297 to +298
} else if self.eat_kw("capability") {
ImportKind::Capability

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject capability imports in v1 parser

When a source file contains use capability c.d@1 as c;, this branch accepts it and the import completes normally. The v1 grammar only admits shape, lawpack, target, and core imports, and the spec marks use capability as non-v1/rejected, so accepting it here lets out-of-profile source pass the syntax phase.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +576 to +579
let max = match self.bound_ref()? {
BoundRef::Int(n) => n,
BoundRef::Coord(_) => {
return self.err("Bytes max must be an integer or digest-locked bound ref");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve coordinate bounds for Bytes refinements

For a valid bounded byte type such as Bytes<max=limits.maxBytes>, bound_ref() returns BoundRef::Coord(_) and this branch reports a parse error. The grammar defines bytes-refine in terms of bound-ref just like String, List, and Map, so digest-locked/profile-provided byte bounds become unparsable even though the error message says they are allowed.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
let mut arms = Vec::new();
while *self.peek() != TokenKind::RBrace {
let start = self.peek_span().start;
let failure = self.ident()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject reserved failure names in obstruction maps

If an obstruction map arm uses a reserved keyword as the failure coordinate, for example else { else => domain.Oops }, ident() accepts it and the parser returns an ObstructionArm. Failure coordinates are required to be non-keyword identifiers, so this admits an AST that cannot correspond to a valid target failure map and should be rejected at the syntax boundary.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
fn for_stmt(&mut self) -> Result<Stmt, ParseError> {
let start = self.peek_span().start;
self.expect_kw("for")?;
let var = self.ident()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject reserved words as loop binders

When a loop is written with a keyword binder, e.g. for match in input.items bounded 1 { ... }, this ident() call accepts the name even though match is rejected everywhere as a bare value identifier. That leaves a syntactically accepted binding that cannot be referenced and misses the same keyword-reservation check now used for let and parameters.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated
if *self.peek() == TokenKind::RBrace {
return self.err("effect branch block must end with `yield <expr>;`");
}
stmts.push(self.stmt()?);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject return statements inside effect-yield blocks

Inside a branch-yield expression, stmt() allows return, so let x = if p { return input; yield a; } else { yield b; }; parses as an IfYield containing an early intent return. Effect-yield blocks are supposed to terminate by yielding the branch value, and the language disallows return there, so this creates an AST shape later stages should never have to handle.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
};

let digest = if self.eat_kw("digest") {
Some(self.string()?)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Validate import digest literals

An import such as use lawpack x.y@1 digest "not-a-digest" as x; is accepted because the digest clause stores any string literal. The grammar requires a digest-lit with sha256: plus exactly 64 hex characters, so invalid or placeholder locks can enter the AST and make later dependency resolution operate on non-canonical identities.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +1212 to +1214
TokenKind::Ident(name) if is_keyword(&name) => self.err(format!(
"keyword `{name}` is reserved and cannot be used as a bare identifier"
)),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Parse digest value literals before keyword rejection

A valid value literal such as digest("sha256:0000000000000000000000000000000000000000000000000000000000000000") reaches the reserved-keyword branch and is rejected before the parser can see the following parentheses. Since digest-value-lit is a primary expression in the v1 grammar, this prevents source from constructing digest values at all.

Useful? React with 👍 / 👎.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jun 20, 2026
@flyingrobots

Copy link
Copy Markdown
Owner Author

PR #9 review remediation pushed

Addressed the unresolved review clusters in two commits:

Commit Scope
0a8acc0 Parser conformance: stable ParseErrorKind, package-version underscore preservation, digest validation/literals, bool literals, v1 capability-import rejection, coordinate Bytes bounds, upper-ident enforcement, empty enum/obstruction-map rejection, reserved loop/failure binders, call-only effect positions, return rejection inside yield blocks, and type-call adjacency. Added parse_review_regressions.rs with deterministic coverage.
3110b2d Review hygiene: pinned checkout action + disabled credential persistence, added Rust 1.85.0/stable matrix using rustup, fixed machine-local doc links, corrected precedence docs, refreshed parser surface/test counts, and markdownlint-cleaned touched docs.

Local verification, run after commit:

  • cargo fmt --all --check
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace --all-targets — 50 tests
  • cargo +1.85.0 fmt --all --check
  • cargo +1.85.0 clippy --workspace --all-targets --all-features -- -D warnings
  • cargo +1.85.0 test --workspace --all-features — 50 tests
  • git diff --check origin/main...HEAD
  • npx --yes markdownlint-cli2 docs/TECHNICAL_EXPLANATION.md CHANGELOG.md docs/RETRO_phase1-parser.md

One thread was intentionally treated as not actionable: rejecting Map type refs would contradict the v1 grammar in docs/SPEC_edict-language-v1.md, which includes Map<K,V,max=...> in type-ref. Map literals remain deferred.

@codex please re-check the current head 3110b2d.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3110b2d033

ℹ️ 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".

Comment thread crates/edict-syntax/src/token.rs Outdated
Comment on lines +273 to +277
if !matches!(self.peek(), Some(b'0'..=b'9')) {
return Err(LexError {
message: "underscore must be between digits".into(),
span: Span::new(us, self.pos),
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve underscores before version labels

When a package/import version uses an underscore before an alphabetic segment, such as use lawpack pkg.lib@1_beta as lib;, the lexer rejects it before Parser::version can apply the spec's version = digit { digit | "." | "-" | "_" | letter } grammar. The parser already reconstructs adjacent version tokens, but this check forces every _ in a numeric token to be followed by a digit, so valid coordinates with _rc/_beta fail to parse.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
};

self.expect_kw("as")?;
let alias = self.ident()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject reserved keywords as import aliases

Import aliases introduce bare names, but this accepts aliases like as return or as if because it bypasses the reserved-keyword check used for binders. Those modules parse successfully even though the alias cannot be referenced as an import root later (return.foo()/if.foo() are parsed as statement syntax), contradicting the reserved-bare-identifier rule while still putting an unusable alias in the AST.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs Outdated
Comment on lines +619 to +624
TokenKind::Int { value, .. } => {
let span = self.peek_span();
self.idx += 1;
value
.parse::<u64>()
.map(BoundRef::Int)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve integer suffixes on bound literals

When a bound literal carries an explicit suffix, such as String<max=1i32> or List<T, max=1u32>, this branch discards the suffix and stores only BoundRef::Int(1). Integer suffixes are hash-significant and semantic validation needs them to reject suffixes that disagree with the expected cardinality type, so distinct sources become indistinguishable before lowering.

Useful? React with 👍 / 👎.

Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs
Comment thread crates/edict-syntax/src/parser.rs Outdated

/// A dotted coordinate: `a.b.c`.
fn path(&mut self) -> Result<Vec<String>, ParseError> {
let mut parts = vec![self.ident()?];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject reserved keywords as coordinate roots

Coordinates used for bounds, intent clauses, and obstruction targets start with a bare identifier, but this helper accepts reserved words there; for example for x in input.items bounded return {} or require ok else if.Blocked; parse as coordinate references rooted at return/if. Keywords are only supposed to remain legal after ., so accepting them as coordinate roots bypasses the same reserved-name rule applied to value identifiers and produces AST references that cannot resolve to valid imports or constants.

Useful? React with 👍 / 👎.

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.

1 participant