Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: warn when payable fallback function found but no receive function #170

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions crates/sema/src/typeck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,38 @@ use crate::{
};
use rayon::prelude::*;
use solar_data_structures::{map::FxHashSet, parallel};
use solar_interface::error_code;

pub(crate) fn check(gcx: Gcx<'_>) {
parallel!(
gcx.sess,
gcx.hir.par_contract_ids().for_each(|id| {
check_duplicate_definitions(gcx, &gcx.symbol_resolver.contract_scopes[id]);
check_payable_fallback_without_receive(gcx, id);
}),
gcx.hir.par_source_ids().for_each(|id| {
check_duplicate_definitions(gcx, &gcx.symbol_resolver.source_scopes[id]);
}),
);
}

fn check_payable_fallback_without_receive(gcx: Gcx<'_>, contract_id: hir::ContractId) {
let contract = gcx.hir.contract(contract_id);

let payable_fallback_found =
contract.fallback.is_some_and(|f| gcx.hir.function(f).state_mutability.is_payable());

let receive_found = contract.receive.is_some();

if payable_fallback_found && !receive_found {
gcx.dcx()
.warn("contract has a payable fallback function, but no receive ether function")
.code(error_code!(3628))
.span_help(contract.name.span, "consider changing fallback to receive")
.emit();
}
}

/// Checks for definitions that have the same name and parameter types in the given scope.
fn check_duplicate_definitions(gcx: Gcx<'_>, scope: &Declarations) {
let is_duplicate = |a: Declaration, b: Declaration| -> bool {
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/parser/payable_fallback_without_receive.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
contract P1 {
fallback() external payable {}
}

contract P2 is P1 {
receive() external payable {}
}

contract P3 {
fallback() external payable {}

receive() external payable {}
Copy link
Member

Choose a reason for hiding this comment

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

Please add annotations for the warnings //~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit strange 🤔 .. I tried

  • Same line variant (//~ WARN: ...)
  • Next line variant(//~^ WARN: ...)

but they both fail on cargo uitest payable_fallback 😕

Copy link
Member

Choose a reason for hiding this comment

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

pushed a fix 8433135
the diagnostic was missing the main span (.span()) but then it also required a workaround for a edge case in ui_test

}

contract P4 is P1 {}
16 changes: 16 additions & 0 deletions tests/ui/parser/payable_fallback_without_receive.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
warning[3628]: contract has a payable fallback function, but no receive ether function
--> ROOT/tests/ui/parser/payable_fallback_without_receive.sol:LL:CC
|
LL | contract P1 {
| -- help: consider changing fallback to receive
|

warning[3628]: contract has a payable fallback function, but no receive ether function
--> ROOT/tests/ui/parser/payable_fallback_without_receive.sol:LL:CC
|
LL | contract P4 is P1 {}
| -- help: consider changing fallback to receive
|

warning: 2 warnings emitted