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 all 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
5 changes: 4 additions & 1 deletion crates/interface/src/diagnostics/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,10 @@ impl DiagCtxtInner {
match (self.deduplicated_err_count, self.deduplicated_warn_count) {
(0, 0) => Ok(()),
(0, w) => {
self.emitter.emit_diagnostic(&Diag::new(Level::Warning, warnings(w)));
// TODO: Don't emit in tests since it's not handled by `ui_test`.
if self.flags.track_diagnostics {
self.emitter.emit_diagnostic(&Diag::new(Level::Warning, warnings(w)));
}
Ok(())
}
(e, 0) => self.emit_diagnostic(Diag::new(Level::Error, errors(e))),
Expand Down
5 changes: 5 additions & 0 deletions crates/sema/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,11 @@ pub struct Function<'hir> {
}

impl Function<'_> {
/// Returns the span of the `kind` keyword.
pub fn keyword_span(&self) -> Span {
self.span.with_hi(self.span.lo() + self.kind.to_str().len() as u32)
}

/// Returns `true` if this is a free function, meaning it is not part of a contract.
pub fn is_free(&self) -> bool {
self.contract.is_none()
Expand Down
18 changes: 18 additions & 0 deletions crates/sema/src/typeck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,37 @@ 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);

if let Some(fallback) = contract.fallback {
let fallback = gcx.hir.function(fallback);
if fallback.state_mutability.is_payable() && contract.receive.is_none() {
gcx.dcx()
.warn("contract has a payable fallback function, but no receive ether function")
.span(contract.name.span)
.code(error_code!(3628))
.span_help(fallback.keyword_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
17 changes: 17 additions & 0 deletions tests/ui/parser/payable_fallback_without_receive.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
contract P1 {
//~^ WARN: contract has a payable fallback function, but no receive ether function
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 {}
//~^ WARN: contract has a payable fallback function, but no receive ether function
22 changes: 22 additions & 0 deletions tests/ui/parser/payable_fallback_without_receive.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
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 {
| --
LL |
LL | fallback() external payable {}
| -------- 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 | fallback() external payable {}
| -------- help: consider changing `fallback` to `receive`
LL | }
...
LL |
LL | contract P4 is P1 {}
| --
|