Skip to content

fix(sema): reject target gated builtin methods on wrong target#1890

Open
aryanbaranwal001 wants to merge 2 commits into
hyperledger-solang:mainfrom
aryanbaranwal001:fix/builtin-method-target-check
Open

fix(sema): reject target gated builtin methods on wrong target#1890
aryanbaranwal001 wants to merge 2 commits into
hyperledger-solang:mainfrom
aryanbaranwal001:fix/builtin-method-target-check

Conversation

@aryanbaranwal001
Copy link
Copy Markdown

This PR fixes compiler panics caused by calling target gated builtin methods on a target they are not valid for

Each builtin in BUILTIN_METHODS carries a target field, empty means available on all targets, non empty means restricted to the listed targets only. The is_builtin_call at src/sema/builtin.rs:898 already enforces this with p.target.is_empty() || p.target.contains(&ns.target). The method resolver at src/sema/builtin.rs:1446 filters only by name and receiver type and never checks func.target. So any target gated builtin method passes sema on every target and reaches codegen, where the target specific host function lookup fails and the compiler panics.

extendTtl is gated to Soroban but sema accepted it on Solana, Polkadot, and EVM, causing .unwrap() to panic at codegen/expression.rs:2892.

Fix

Add the target check to the filter at src/sema/builtin.rs:1446:

let funcs: Vec<_> = BUILTIN_METHODS
    .iter()
    .filter(|func| {
        func.name == id.name
            && func.method.contains(deref_ty)
            && (func.target.is_empty() || func.target.contains(&ns.target))
    })
    .collect();

And improve the funcs.is_empty() branch at src/sema/builtin.rs:1525 to emit a specific diagnostic when the method exists but is gated to a different target:

if funcs.is_empty() {
    if BUILTIN_METHODS
        .iter()
        .any(|func| func.name == id.name && func.method.contains(deref_ty))
    {
        diagnostics.push(Diagnostic::error(
            id.loc,
            format!(
                "method '{}' is not available on target {}",
                id.name, ns.target
            ),
        ));
        return Err(());
    }
    Ok(None)
} else {
    diagnostics.extend(call_diagnostics);
    Err(())
}

Now any target gated builtin method called on the wrong target is rejected in sema with a clear diagnostic instead of panicking in codegen. Methods with no target restriction are unaffected.

Happy to make changes if the approach needs adjustment.

Signed-off-by: Aryan Baranwal <aryanbaranwal131214@gmail.com>
@aryanbaranwal001
Copy link
Copy Markdown
Author

The CI failures are unrelated to this change.

ruint@1.18.0 was published to crates.io recently and bumped its minimum supported Rust version to rustc >= 1.90. The CI workflow installs 1.88.0 (hardcoded via dtolnay/rust-toolchain@1.88.0 in the workflow YAMLs)

And runs all cargo invocations (cargo build, cargo test, etc.) without --locked, so Cargo re-resolves dependencies against crates.io and picks up ruint@1.18.0 instead of the 1.17.2 pinned in Cargo.lock. This causes every cargo job to abort immediately with the following error.

#15 6.059 error: rustc 1.88.0 is not supported by the following package:
#15 6.059   ruint@1.18.0 requires rustc 1.90
#15 6.059 Either upgrade rustc or select compatible dependency versions with

The full transitive chain that pulls ruint is (cargo tree --invert --package ruint):

❯ cargo tree --invert --package ruint
ruint v1.17.2
└── alloy-primitives v0.7.7
    ├── solang-forge-fmt v0.2.0 (/solang/fmt)
    └── solang-forge-fmt v0.2.0
        └── solang v0.3.4 (/solang)

This PR makes no changes to any dependency. The same failure will affect the next scheduled CI run on main.

Possible fixes:

  • Bump rust-toolchain in the workflow files to 1.90.0
  • Add --locked to the cargo invocations in CI so it respects Cargo.lock

Happy to open a separate PR for whichever fix is preferred. Though I suggest bumping up the toolchain to 1.90.0 or even the latest version 1.95.0, since ruint won't be the last transitive dep to raise its MSRV.

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.

Compiler panic in codegen: Option::unwrap() on extendTtl when compiled for a non-Soroban target

2 participants