Make lookup_variable return ValueRef

[jacinta-stacks]

Closes https://github.com/stx-labs/core-epics/issues/151

Lookup_variable now returns a ref where possible. In theory we might be able to expand eval and apply to also return a value_ref...not positive. But for now this is just taking advantage in the lookup_variable instance.

The change seems bigger than it actually is. I had to seperate the Environment into the invocation context and execution context since the invocation context is immutable and what we are returning a ref to. otherwise have lifetime issues. using RC is not as clean/a bit hackier/made the cost tracking a bit trickier. So this is the result. Rip it to shreds and I will try my best to improve.

[aaronb-stacks]

This looks like a great overall approach to me. I have a handful of mostly light comments on the code, but one higher-level comment on the treatment of errors.

For things like RuntimeCheckErrorKind::TypeValueError, I'm kind of wary of charging for the clone of the value. The actual contents of this error are not consensus critical, which means it would be nice to have the freedom to kind of "drop" these inner values. Now its true that these errors want ownership today, so it's fair to charge for the clone, but my current thinking is something along the lines of the following:

1. We make the contents of these errors strings.
2. We have a default max length of the string serialization
3. We do something like format!("{:value.MAX_LEN}") to write the contents.

That way, for the error cases, we don't have to worry about potentially cloning very large values.

[jacinta-stacks]

This looks like a great overall approach to me. I have a handful of mostly light comments on the code, but one higher-level comment on the treatment of errors.

Done in 64a036c. Wasn't sure what a reasonable value would be for to_error_string() (chose 512)

[brice-stacks]

I'm still working through this review, but posting a couple comments for today. It looks great so far btw!

[jacinta-stacks]

@brice-stacks I did some benching. (64d35cb):

fold_buf_cmp (128-byte buffer, 2 lookups/step):

Steps	Epoch33	Epoch34	Speedup
50	45.1 µs	38.4 µs	~15%
200	163.8 µs	137.2 µs	~16%

fold_ascii_cmp (128-char string, 2 lookups/step):

Steps	Epoch33	Epoch34	Speedup
50	46.4 µs	39.1 µs	~16%
200	168.9 µs	140.9 µs	~17%

let_local_refs (128-byte buffer, N body lookups):

Refs	Epoch33	Epoch34	Speedup
10	6.66 µs	6.60 µs	~1%
50	16.4 µs	13.8 µs	~16%

The >= / <= / > / < comparison operators call eval directly and pattern-match on value.as_ref() — they never call clone_with_cost at all. So for a fold where each step compares a constant, Epoch33 allocates two 128-byte copies per iteration and Epoch34 allocates zero. At 200 steps that's ~50KB of heap traffic eliminated per eval_all call.

feel free to rerun with cargo bench --bench value_ref -p clarity 2>&1 | grep -E "value_ref|time:"

[brice-stacks]

LGTM!

[aaronb-stacks]

LGTM!

[codecov]

Codecov Report

❌ Patch coverage is 94.63722% with 102 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.01%. Comparing base (962e7ab) to head (f380ceb).
⚠️ Report is 25 commits behind head on develop.

Files with missing lines	Patch %	Lines
clarity/src/vm/functions/assets.rs	93.38%	24 Missing ⚠️
clarity/src/vm/functions/arithmetic.rs	87.23%	12 Missing ⚠️
clarity/src/vm/mod.rs	93.19%	10 Missing ⚠️
clarity/src/vm/functions/database.rs	98.30%	6 Missing ⚠️
pox-locking/src/events.rs	53.84%	6 Missing ⚠️
clarity-types/src/types/mod.rs	50.00%	5 Missing ⚠️
clarity/src/vm/database/clarity_db.rs	50.00%	5 Missing ⚠️
clarity/src/vm/functions/sequences.rs	95.32%	5 Missing ⚠️
stackslib/src/chainstate/stacks/db/mod.rs	0.00%	5 Missing ⚠️
clarity/src/vm/functions/crypto.rs	92.59%	4 Missing ⚠️
... and 9 more

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #6918       +/-   ##
============================================
+ Coverage    60.61%   85.01%   +24.40%     
============================================
  Files          412      412               
  Lines       219362   219782      +420     
  Branches       338      338               
============================================
+ Hits        132974   186857    +53883     
+ Misses       86388    32925    -53463     

Files with missing lines	Coverage Δ
clarity-types/src/errors/analysis.rs	79.95% <100.00%> (+8.97%)	⬆️
clarity/src/vm/clarity.rs	87.28% <100.00%> (+1.69%)	⬆️
clarity/src/vm/contracts.rs	100.00% <100.00%> (ø)
clarity/src/vm/docs/mod.rs	90.81% <100.00%> (+86.05%)	⬆️
clarity/src/vm/functions/define.rs	99.36% <100.00%> (+0.37%)	⬆️
clarity/src/vm/functions/options.rs	91.12% <100.00%> (+8.72%)	⬆️
clarity/src/vm/functions/tuples.rs	84.84% <100.00%> (+2.08%)	⬆️
clarity/src/vm/variables.rs	96.74% <100.00%> (+24.63%)	⬆️
pox-locking/src/pox_2.rs	86.96% <100.00%> (+2.77%)	⬆️
pox-locking/src/pox_3.rs	78.80% <100.00%> (+15.34%)	⬆️
... and 29 more

... and 267 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 962e7ab...f380ceb. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.