Repository navigation
parser: preserve named proto contracts in the AST - #1075
hiyouwhatsapp wants to merge 12 commits into
Conversation
Parse the named contract form without changing inherent proto attachments Keep target arguments, visibility and receiver types in the syntax tree with focused diagnostic tests Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
Signed-off-by: hiyouwhatsapp <89997383+hiyouwhatsapp@users.noreply.github.com>
LunaStev
left a comment
There was a problem hiding this comment.
Reviewed commit e94f788355c8a8bdb509c3131060950ff092a969 with Rust 1.89. The parser-only scope is appropriate, but the new positive tests currently fail and newly accepted named blocks can leak into the existing inherent-method pipeline.
1. Two new positive tests fail before their assertions
Reproduction:
cargo +1.89.0 test --locked --jobs 2 -p parser --test proto_contracts -- --nocaptureObserved: 1 passed, 2 failed. Both named_and_inherent_proto_forms_remain_distinct and public_named_contract_preserves_target_generic_arguments_and_visibility fail with:
expected identifier in function name
found: Print (`print`)
print is a reserved lexer token, so the fun print(self) fixtures never reach their AST assertions. Use an ordinary valid method name in the positive cases, or obtain an explicit language decision before broadening keyword/name rules. Also make the missing-body negative case reach body parsing rather than succeeding as a negative test only because its function name is already invalid.
2. Keep parser-only named contracts out of inherent method lowering
The new name field is preserved in the AST, but existing ProgramTypes::collect, method lowering and LLVM function collection key implementations by target and method name only. They do not inspect the new contract name.
I exercised both strings through Lexer -> parse_syntax_with_spans -> TypedProgram::lower on this commit:
struct Buffer {}
proto MissingContract: Buffer {
fun value(self) -> i32 { return 7; }
}
fun main() {}
This is accepted by HIR lowering despite there being no implementation of named-contract semantics.
struct Buffer {}
proto First: Buffer {
fun value(self) -> i32 { return 7; }
}
proto Second: Buffer {
fun value(self) -> i32 { return 8; }
}
fun main() {}
This is rejected as duplicate method Buffer.value: the two contract names have already been erased from the semantic lookup key. These probes use valid method identifiers, independently of the fixture problem above.
To reproduce that frontend boundary without building LLVM, create a temporary HIR example that passes either string to:
let tokens = lexer::Lexer::new(source).tokenize().unwrap();
let syntax = parser::parse_syntax_with_spans(&tokens).unwrap();
match hir::TypedProgram::lower(syntax) {
Ok(_) => println!("accepted"),
Err(error) => println!("{}", error.diagnostic().message),
}Build/run it with cargo +1.89.0 run --locked --jobs 2 -p hir --example <example-name>.
This is not a request to implement coherence, constraints or contract method resolution in #365. Keep the parser/AST representation, but add a located unsupported-feature diagnostic at the semantic boundary until those later stages explicitly support named contracts. Named blocks must not silently acquire inherent-extension semantics. Preserve the old colon-free proto path and add tests for both outcomes.
Validation and formatting
The existing six generic_methods HIR regressions passed locally. The new tests above did not. The changed files also fail cargo +1.89.0 fmt --all -- --check; format the touched files, then rerun the new parser suite, existing inherent/generic method coverage and the parser-only AST serialization/clone cases.
LunaStev
left a comment
There was a problem hiding this comment.
Re-reviewed updated head 55edcb9.
The prior functional requests are resolved: the positive fixtures use valid method names, and named contracts now receive a located unsupported-feature diagnostic at the semantic boundary instead of acquiring inherent-method semantics. I ran the three proto parser tests, both named-boundary HIR tests and six existing generic-method tests successfully.
One required gate remains: the touched parser source and tests do not pass pinned rustfmt. Reproduce:
rustfmt +1.89.0 --edition 2021 --check front/parser/src/parser/items.rs front/parser/tests/proto_contracts.rs
cargo +1.89.0 fmt --all -- --checkObserved differences include the ProtoImpl construction around items.rs:170 and the long assertion/let-else blocks in proto_contracts.rs. Format these touched files, rerun the two focused integration suites and the existing inherent/generic method coverage, then obtain successful checks on the updated head.
This request replaces the two earlier functional blockers with the remaining formatting correction. It does not ask for named-contract coherence or method resolution in this parser/AST PR.
LunaStev
left a comment
There was a problem hiding this comment.
Re-reviewed updated head b5acb66.
The parser/named-contract corrections remain sound in the focused checks: three proto parser tests, two named-contract HIR boundary tests and six generic-method tests passed. I found no new functional blocker in this formatting follow-up.
The pinned formatting gate still fails on this exact head, both locally and in CI 41823784281. Its Linux amd64 job fails the cargo +1.89.0 fmt --all -- --check command.
Reproduce from the repository root:
cargo +1.89.0 fmt --all -- --check
rustfmt +1.89.0 --edition 2021 --check front/parser/src/parser/items.rs front/parser/tests/proto_contracts.rsRemaining locations include items.rs:189 and proto_contracts.rs:27,36,61,73. One exact formatter expectation is:
- let ASTNode::Function(mut method) = parse_function(tokens)? else {
- unreachable!()
- };
+ let ASTNode::Function(mut method) = parse_function(tokens)? else { unreachable!() };Generate the touched-file formatting with the pinned Rust 1.89.0 formatter and repository rustfmt.toml, then run the complete check again. The current expanded let-else edits still differ from that formatter's output.
Retain the parser-only scope and semantic unsupported-feature boundary. Rerun the focused tests after formatting and obtain successful required checks on the resulting head.
|
Remaining changes before merge — reviewed head The earlier functional requests are addressed and 11 focused tests passed. Please finish the remaining formatting correction. The current pinned check still reports differences in front/parser/src/parser/items.rs around 189 and proto_contracts.rs around 27, 36, 61 and 73, including let-else layout and assertion wrapping. Generate the edits with Rust 1.89.0 and the repository configuration: rustfmt +1.89.0 --edition 2021 --config-path rustfmt.toml front/parser/src/parser/items.rs front/parser/tests/proto_contracts.rs
cargo +1.89.0 fmt --all -- --check
cargo +1.89.0 test --locked --jobs 2 -p parser --test proto_contracts
cargo +1.89.0 test --locked --jobs 2 -p hir --test named_proto_boundary --test generic_methodsKeep the parser/AST scope and located unsupported-feature boundary intact. The formatter's generated output is needed; the current manual layout still differs from the pinned check. Detailed findings and reproduction: review. |
LunaStev
left a comment
There was a problem hiding this comment.
Reviewed head 3a4ae11e79b93ac966624093be2734e99959ffef.
The previous parser/AST requests are addressed: legal positive fixtures, a located unsupported boundary for named contracts, and pinned formatting. The additional incomplete inherent-proto diagnostic preserves the existing message. Local pinned formatting passed, and all 12 focused parser/HIR tests passed (4 proto contracts, 2 named-boundary, 6 generic-method tests).
Code review approved. CI is still not fully successful: run 42059955925 has 49 successes and one failed FreeBSD/arm64 compile shard. Job 127385450855 fails before compiler execution while downloading LLVM packages from apt.llvm.org (IPv6 network unreachable / IPv4 connection timeout). This log is not evidence of a parser defect.
Keep the PR open until the required check succeeds through the normal CI process; this approval does not waive or replace that check. No workflow rerun or full-suite repeat was started during this review.
Fixes #365
The parser only understood inherent
proto Target { ... }blocks, so named contracts failed before the AST could represent themI added
proto Name : Target { ... }parsing, keeping it distinct from the old form and preserving the name, generic target, visibility and method bodiesNamed proto methods can use the shorthand
selfreceiver from the target type without changing ordinary function parametersAdded parser regressions for both forms, public declarations, generic targets, cloning and malformed syntax with source locations
Only covers parsing and the source AST, contract coherence and method resolution stay out of scope
Haven't run the Rust tests locally, CI still needs to verify the change