apollo_compile_to_casm: test that the bundled libfunc list rejects what it excludes - #15141
Yoni-Starkware wants to merge 1 commit into
Conversation
PR SummaryLow Risk Overview A new integration test Test hygiene:
Reviewed by Cursor Bugbot for commit d46a670. Bugbot is set up for automated code reviews on this repo. Configure here. |
avi-starkware
left a comment
There was a problem hiding this comment.
@avi-starkware+AGNT made 6 comments.
Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on avi-starkware and Yoni-Starkware).
crates/apollo_compile_to_casm/src/compiler_test.rs line 6 at r1 (raw file):
use crate::constants::BUNDLED_ALLOWED_LIBFUNCS_PATH; #[test]
built_in_libfunc_lists_are_selected_by_name asserts the same string literals that libfunc_list_arg is written with, so it pins the code against itself rather than against cairo-lang.
The match arms in compiler.rs spell "audited" and "all" on lines 75-76, and the test spells them again. If cairo-lang renamed a built-in list, the compiler invocation would break and this test would stay green.
cairo_lang_starknet_classes::allowed_libfuncs::{BUILTIN_AUDITED_LIBFUNCS_LIST, BUILTIN_ALL_LIBFUNCS_LIST} are both pub const in the pinned 2.19.4 and would pin the actual contract. Neither is imported anywhere in the repo today, so this needs a new use here.
Dropping the test is a fair answer too, given what it covers as written.
Nonblocking.
crates/apollo_compile_to_casm/src/compile_test.rs line 38 at r1 (raw file):
}; // Libfuncs in allowed_libfuncs.json but not yet in Cairo's audited list.
PENDING_LIBFUNCS is empty now, so the pending_set filter and the new staleness assert are both no-ops. Keep the empty hook, or delete the three pieces and re-add them the next time allowed_libfuncs.json runs ahead of the audited list?
Asking which you intended, not reporting a bug. The guard works. Populating it fires the assert as designed:
PENDING_LIBFUNCS = &["array_append"]
-> panics at compile_test.rs:188
And there is a real argument for keeping it: it is the only assert that catches a pending entry that has since been audited. Such an entry sits in both maps, so the earlier missing/extra check stops seeing it, and this one takes over. If that is the intent, saying so in the comment above the constant would make the empty slice read as deliberate rather than leftover.
Nonblocking.
crates/apollo_compile_to_casm/src/compile_test.rs line 49 at r1 (raw file):
const EXCLUDED_LIBFUNCS: &[&str] = &["coupon_buy", "coupon_call", "coupon_refund"]; // A class using an excluded libfunc. Regenerate from the sibling .cairo with:
The regeneration recipe on EXCLUDED_LIBFUNC_CLASS_PATH does not regenerate coupon_contract.sierra.json.
coupon_buy sits behind an experimental Cairo feature that has to be enabled in a crate config, so the fixture cannot be produced from a single .cairo file at all. I ran the documented command four ways against the cairo-package starknet-compile:
# as written, pointed at the sibling .cairo
Error: The given path is a file, but --single-file was not supplied.
# adding -s
error[E2167]: Coupons are disabled in the current crate. You can enable them by
enabling the coupons experimental feature in the crate config.
error[E2030]: Wrong number of arguments. Expected 1, found: 2
# pointed at resources/ as a crate
Failed to load project config: No such file or directory
# a real crate dir whose cairo_project.toml carries
# [config.global] edition = "2024_07"
# [config.global.experimental_features] coupons = true
exit 0
Only the last one produces the fixture. Worth noting -s alone looks like the fix and is not.
scripts/install_compiler_binaries.sh also never installs starknet-compile. It installs starknet-sierra-compile and starknet-native-compile, so whoever picks this up next has to find the binary under target/bin/cairo_package__<CAIRO1_COMPILER_VERSION>/cairo/bin/. Worth naming in the comment.
Committing the crate layout the fixture was built from is the durable fix. Spelling the full recipe out in the comment works too.
I'd fix it rather than leave it, because nothing in CI compiles the .cairo, so drift between it and the committed .sierra.json stays invisible.
On the good news side, the committed fixture is genuine: regenerating it that last way reproduced the file byte for byte except sierra_program felts 2 and 5, which carry the sierra and compiler versions.
Caveat on all of the above: measured with starknet-compile 2.19.0-rc.3. No 2.19.4 build exists on my machine, though the repo pins 2.19.4.
Nonblocking.
crates/apollo_compile_to_casm/src/compile_test.rs line 213 at r1 (raw file):
} /// The default selects the bundled list, which has to be resolved from disk; a node that ships
The doc comment on bundled_libfuncs_list_rejects_an_excluded_libfunc opens with two lines written for a different test.
Lines 213-214 documented compile_with_the_default_config before this change. The new test landed between them and that function, and Rust attaches a contiguous /// block downward, so all four lines now document the new test:
/// The default selects the bundled list, which has to be resolved from disk; a node that ships
/// without it would panic on startup rather than fail a compilation.
/// The built-in lists both permit [`EXCLUDED_LIBFUNCS`], so rejecting this class is the only
/// behaviour that tells the bundled list apart from `Audited` and `All`.
#[test]
fn bundled_libfuncs_list_rejects_an_excluded_libfunc() {The new test never builds SierraCompilationConfig::default(), so the first sentence misdescribes it. And compile_with_the_default_config on line 243 is left with no doc at all.
Moving lines 213-214 back down onto compile_with_the_default_config fixes it.
One thing that does not fix it, in case it looks like it would: putting a blank line between the two halves. rustc reads a blank line inside a doc block as a paragraph break and still attaches both halves to the item below. I checked that before writing this.
Nonblocking.
crates/apollo_compile_to_casm/src/compile_test.rs line 226 at r1 (raw file):
..SIERRA_COMPILATION_CONFIG }); let bundled_list_compiler = SierraToCasmCompiler::new(SierraCompilationConfig {
The bundled-compiler construction is written three times in this file now. Lines 226-229 are byte-identical to lines 204-207 in compile_against_the_bundled_libfuncs_list, and 222-225 differ only in the variant:
md5(204-207) = 78ed5c9284336cc83a30167fc24499c3
md5(226-229) = 78ed5c9284336cc83a30167fc24499c3
A helper next to compiler() collapses each site to one line:
fn compiler_with_libfuncs_list(allowed_libfuncs_list: AllowedLibfuncsList) -> SierraToCasmCompiler {
SierraToCasmCompiler::new(SierraCompilationConfig {
allowed_libfuncs_list,
..SIERRA_COMPILATION_CONFIG
})
}compiler() can then delegate to it with AllowedLibfuncsList::All, since SIERRA_COMPILATION_CONFIG already sets that on line 35.
Nonblocking, and easy to drop if you'd rather keep each construction spelled out.
crates/apollo_compile_to_casm/src/compile_test.rs line 231 at r1 (raw file):
}); compiler().compile(excluded_libfunc_class.clone()).expect("`All` must accept the class.");
The All compile on this line cannot fail while the Audited compile just below it passes.
audited is a subset of all in cairo-lang-starknet-classes 2.19.4, which is what Cargo.lock resolves at this commit, and upstream has a test enforcing the relation, so it holds by invariant rather than by luck:
audited_not_in_all == []
So the line spawns a compiler subprocess that carries no signal the Audited line does not already carry.
Before you act on that, there is a reason to keep it. The doc comment above the test claims "The built-in lists both permit [EXCLUDED_LIBFUNCS]", and this line is what demonstrates the "both" half. If you want that claim in the doc, keeping the line that backs it is a fair trade. The leaner version drops the line and states the subset relation in the comment instead.
Your call, and I'm happy either way. Nonblocking.
e74a7c1 to
c109f2d
Compare
…at it excludes Add a class using a coupon libfunc as a fixture, with the crate it is built from, since coupons are behind an experimental feature that a single source file cannot enable. It compiles under both built-in lists and is rejected by the bundled one, which is the only behaviour that tells the three apart. Follow-ups from the #15133 review: - Guard `PENDING_LIBFUNCS` against entries that have since been audited, mirroring the `EXCLUDED_LIBFUNCS` check, and drop the three entries it flags. - Move the libfunc argument assertions into `compiler_test.rs`, where they run without the compiler binary, and pin the bundled path rather than only the flag. - Give the positive flow in `test_max_memory_usage` a limit that differs from the default, so it no longer compares a value with itself. Take the built-in list names from `cairo_lang_starknet_classes` rather than spelling them, so the compiler invocation cannot drift from the names cairo-lang accepts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c109f2d to
d46a670
Compare
Follow-ups deferred from the #15133 review.
The test
Nothing so far distinguished
Bundledfrom the built-in lists: the list selection gates acceptance but does not change the emitted CASM, so comparing outputs can't tell them apart. A class using an excluded libfunc can.resources/coupon_contract.sierra.jsonis such a class, with its Cairo source committed beside it. Run against the three selections directly:allauditedbundledLibfunc coupon_buy is not allowedBoth built-in lists permit coupons, so rejecting this class is the only behaviour that separates our list from either of them.
bundled_libfuncs_list_rejects_an_excluded_libfuncasserts all three.This only became possible once #15136 landed — on #15133 alone the bundled list held exactly the same entries as
audited.Other review follow-ups
PENDING_LIBFUNCShad no staleness guard. It is only used to permit extras, so an entry that has since been audited is neither missing nor extra and sits there indefinitely. Added the mirror of theEXCLUDED_LIBFUNCScheck; it flagged all threesha512_*entries, which are now in the audited list at the pinned 2.19.4, so the const is emptied.compiler_test.rs. They sat inside a test that constructsSierraToCasmCompiler, so they only ran where the pinnedstarknet-sierra-compileis installed, though the function needs nothing but a resolvable path. They now also pin the bundled path, not just the flag — pointingBundledat the wrong file previously still passed.test_max_memory_usagepositive flow overrodemax_memory_usagewith the valueSIERRA_COMPILATION_CONFIGalready sets, so it rebuilt the same compiler and compared a value with itself. Now uses a limit that differs from the default.Testing
cargo test -p apollo_compile_to_casm— 13 passed.apollo_sierra_compilation_config,apollo_node_config,apollo_deployments— 30 passed. fmt +clippy --all-targets -- -D warningsclean.🤖 Generated with Claude Code