Skip to content

fix[codegen]: remove silent fallback for bool kwargs in venom pipeline#4984

Merged
charles-cooper merged 3 commits into
vyperlang:masterfrom
banteg:feat/venom-folded-bool-kwargs
May 21, 2026
Merged

fix[codegen]: remove silent fallback for bool kwargs in venom pipeline#4984
charles-cooper merged 3 commits into
vyperlang:masterfrom
banteg:feat/venom-folded-bool-kwargs

Conversation

@banteg

@banteg banteg commented May 20, 2026

Copy link
Copy Markdown
Contributor

What I did

Fixed Venom builtin lowering for boolean keyword arguments that semantic analysis already accepted as folded constants.

This covers abi_decode(..., unwrap_tuple=CONST_FALSE) and print(..., hardhat_compat=CONST_TRUE).

How I did it

Updated the Venom bool kwarg extraction helpers in the ABI and debug-print builtins to use kw_node.reduced() before checking for boolean or integer literals.

If a provided bool kwarg still cannot be recognized after reduction, the helper raises a compiler panic instead of silently falling back to the default. That keeps the absent-kwarg default path, but avoids masking unexpected accepted input.

How to verify it

  • uv run --python 3.12 pytest tests/unit/compiler/venom/test_builtin_kwargs.py tests/functional/builtins/codegen/test_abi_decode.py::test_abi_decode_folded_unwrap_tuple_kwarg tests/functional/syntax/test_print.py::test_print_folded_hardhat_compat_kwarg -q
  • uv run --python 3.12 ruff check vyper/codegen_venom/builtins/abi.py vyper/codegen_venom/builtins/misc.py tests/unit/compiler/venom/test_builtin_kwargs.py tests/functional/builtins/codegen/test_abi_decode.py tests/functional/syntax/test_print.py

Commit message

_get_bool_kwarg in abi.py and misc.py silently returned the default
value when the kwarg node was not a NameConstant or Int literal. this
masked two bugs at once: folded constants like UNWRAP=False (a Name, not
NameConstant) were ignored, and any unexpected node type was swallowed
without error.

call kw_node.reduced() before the type checks so folded constants
resolve to their actual literal form. replace the silent fallback
with a CompilerPanic to make future issues immediately visible.

Description for the changelog

Fix Venom builtin lowering for folded constant boolean keyword arguments.

@charles-cooper

Copy link
Copy Markdown
Member

i think it's worse that we are falling back to something rather than not unfolding. we should remove the return default fallback line in these builtins (and actually all builtins as applicable)

@banteg banteg force-pushed the feat/venom-folded-bool-kwargs branch from 31b2ebb to deda489 Compare May 20, 2026 12:10
@banteg

banteg commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Updated. The helpers still return the default when the kwarg is absent, but a provided bool kwarg that cannot be interpreted after folding now raises CompilerPanic instead of falling back to the default.

Comment thread vyper/codegen_venom/builtins/misc.py Outdated
Comment thread vyper/codegen_venom/builtins/abi.py Outdated
@codecov

codecov Bot commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.81%. Comparing base (e57e6ac) to head (6e25d45).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4984      +/-   ##
==========================================
+ Coverage   92.71%   92.81%   +0.09%     
==========================================
  Files         187      187              
  Lines       27626    27629       +3     
  Branches     4776     4776              
==========================================
+ Hits        25614    25644      +30     
+ Misses       1363     1336      -27     
  Partials      649      649              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@banteg banteg force-pushed the feat/venom-folded-bool-kwargs branch from deda489 to 8530645 Compare May 20, 2026 15:29
@banteg

banteg commented May 20, 2026

Copy link
Copy Markdown
Contributor Author

Updated to use kw_node.reduced() in both bool-kwarg helpers. I also added direct unit coverage for both the folded path and the unreduced/error path, so the Codecov patch complaint should clear on the new run.

@charles-cooper charles-cooper requested a review from harkal May 21, 2026 08:24
Comment thread vyper/codegen_venom/builtins/abi.py Outdated
if isinstance(kw_node, vy_ast.Int):
return bool(kw_node.value)
return default
raise CompilerPanic(f"unfoldable boolean kwarg: {kwarg_name}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
raise CompilerPanic(f"unfoldable boolean kwarg: {kwarg_name}")
raise CompilerPanic(f"unfoldable boolean kwarg: {kwarg_name}", kw_node)

Comment thread vyper/codegen_venom/builtins/misc.py Outdated
if isinstance(kw_node, vy_ast.Int):
return bool(kw_node.value)
return default
raise CompilerPanic(f"unfoldable boolean kwarg: {kwarg_name}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
raise CompilerPanic(f"unfoldable boolean kwarg: {kwarg_name}")
raise CompilerPanic(f"unfoldable boolean kwarg: {kwarg_name}", kw_node)

@banteg

banteg commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

Applied the suggestion in both bool-kwarg helpers: the CompilerPanic now carries the offending kwarg node. Validated with the focused bool-kwarg, abi_decode, print tests, and ruff check.

@charles-cooper charles-cooper changed the title fix[venom]: honor folded bool kwargs in builtins fix[codegen]: remove silent fallback for bool kwargs in venom pipeline May 21, 2026
@charles-cooper charles-cooper enabled auto-merge (squash) May 21, 2026 09:26
@charles-cooper charles-cooper merged commit 6bf9669 into vyperlang:master May 21, 2026
177 checks passed
@banteg

banteg commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

Correction: #4984 was merged while this follow-up was being pushed, so the branch update is not part of the merged PR.

I opened the audited follow-up as #5001: #5001

Summary of the follow-up:

  • removed every Venom _get_kwarg_value copy across abi, create, system, and misc
  • added shared reduced-kwarg helpers so builtin callers receive kw.value.reduced()
  • made required literal kwargs fail closed with CompilerPanic instead of silently falling back to defaults
  • kept runtime fallback only for create_from_blueprint(code_offset=...), where runtime values are allowed
  • found and fixed an adjacent ABIEncode.fetch_call_return raw ensure_tuple read that ICEd on ensure_tuple=CONST before codegen

Validated with focused Venom helper, abi_encode, abi_decode, print, constants, create, raw_call, and send tests; ruff check is clean on all touched files.

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.

3 participants