Skip to content

Commit

Permalink
Fix scwq instruction and "phantom" error in purity checks (#6432)
Browse files Browse the repository at this point in the history
## Description

This PR fixes #6320 and #6431 by:
- properly treating the `scwq` instruction in purity check as a storage
writing instruction.
- not emitting purity issues for `__entry` functions.

Additionally, the PR:
- removes purity checks from the type-checking phase. Those checks were:
  - redundant. The same checks were also done at the IR side.
- incomplete. The checks were covering only the function and method
calls, not other kinds of storage access (e.g., intrinsics).
- inaccurate. The checks were relying on `#[storage]` attributes, which
do not necessary represent actual storage access patterns.
- extends `MetadataManager` to be able to store more then one `Span` per
`MetadataIndex`. E.g., for a function, we can now store the span of the
whole function declaration, but in addition, also the span pointing only
to the function name.
- removes two different and overlapping storage access mismatch errors
and introduces one expressive diagnostics that points to the exact
storage access violations inside of a function (see demo below).

Note that having the purity tests at the IR level means that no purity
errors will be reported on non-used functions. This was already the case
before, since the complete set of tests was done at the IR level, and
only two tests (inaccurately) at the type-checking phase, where errors
would be reported even for the non-used functions. The purity guaranty
is given for all the compiled code.

Closes #6320.
Closes #6431.

## Demo

Before, we had two different error messages, rendered potentially
several times per access violation.

![Before 01 - Storage attribute access
mismatch](https://github.com/user-attachments/assets/1207b640-2742-4cee-afa9-da9cfc743e2c)

![Before 02 - Function performs storage
read](https://github.com/user-attachments/assets/284f9a65-fc6c-4318-ab83-5beab730b4d3)

Now, we have only one error message that points to the access violations
and explains them.

![After 01 - Pure function cannot access
storage](https://github.com/user-attachments/assets/87f69493-efd2-4230-bd52-95f0ea1c6fd8)

![After 02 - Pure function cannot access
storage](https://github.com/user-attachments/assets/2fd3ff3b-5738-4e46-99f9-417c4be8520d)

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
  • Loading branch information
ironcev authored Aug 19, 2024
1 parent 7dc3e2d commit 7731c6b
Show file tree
Hide file tree
Showing 74 changed files with 1,061 additions and 368 deletions.
13 changes: 13 additions & 0 deletions docs/book/src/blockchain-development/purity.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
A function is _pure_ if it does not access any [persistent storage](./storage.md). Conversely, the function is _impure_ if it does access any storage. Naturally, as storage is only available in smart contracts, impure functions cannot be used in predicates, scripts, or libraries. A pure function cannot call an impure function.

In Sway, functions are pure by default but can be opted into impurity via the `storage` function attribute. The `storage` attribute may take `read` and/or `write` arguments indicating which type of access the function requires.

The `storage` attribute without any arguments, `#[storage()]`, indicates a pure function, and has the same effect as not having the attribute at all.
<!-- pure:example:end -->

```sway
Expand All @@ -17,6 +19,15 @@ fn get_amount() -> u64 {
fn increment_amount(increment: u64) -> u64 {
...
}
fn a_pure_function() {
...
}
#[storage()]
fn also_a_pure_function() {
...
}
```

> **Note**: the `#[storage(write)]` attribute also permits a function to read from storage. This is due to the fact that partially writing a storage slot requires first reading the slot.
Expand All @@ -31,6 +42,8 @@ The `storage` attribute may also be applied to [methods and associated functions
<!-- This section should explain the benefits of using pure functions in Sway -->
<!-- pure_benefits:example:start -->
A pure function gives you some guarantees: you will not incur excessive storage gas costs, the compiler can apply additional optimizations, and they are generally easy to reason about and audit.

> **Note**: Purity does not provide an absolute guarantee that a storage access will not happen as a result of calling a pure function. E.g., it is possible for a pure function to call another contract, which can then call a write function in the original contract. The guarantee that the purity gives in this example is, that the original pure function itself does not change the storage, as well as that any function later called, that accesses storage, is clearly marked as impure.
<!-- pure_benefits:example:end -->
[A similar concept exists in Solidity](https://docs.soliditylang.org/en/v0.8.10/contracts.html#pure-functions). Note that Solidity refers to contract storage as _contract state_, and in the Sway/Fuel ecosystem, these two terms are largely interchangeable.
1 change: 1 addition & 0 deletions sway-ast/src/assignable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub enum Assignable {
/// E.g.:
/// - `my_variable`
/// - `array[0].field.x.1`
///
/// Note that within the path, we cannot have dereferencing
/// (except, of course, in expressions inside of array index operator).
/// This is guaranteed by the grammar.
Expand Down
1 change: 1 addition & 0 deletions sway-core/src/ir_generation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ impl CompiledFunctionCache {
md_mgr,
module,
&callee_fn_decl,
&decl.name,
logged_types_map,
messages_types_map,
is_entry,
Expand Down
13 changes: 12 additions & 1 deletion sway-core/src/ir_generation/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use super::{

use sway_error::{error::CompileError, handler::Handler};
use sway_ir::{metadata::combine as md_combine, *};
use sway_types::Spanned;
use sway_types::{Ident, Spanned};

use std::{collections::HashMap, sync::Arc};

Expand Down Expand Up @@ -390,6 +390,7 @@ pub(super) fn compile_function(
md_mgr: &mut MetadataManager,
module: Module,
ast_fn_decl: &ty::TyFunctionDecl,
original_name: &Ident,
logged_types_map: &HashMap<TypeId, LogId>,
messages_types_map: &HashMap<TypeId, MessageId>,
is_entry: bool,
Expand All @@ -408,6 +409,7 @@ pub(super) fn compile_function(
md_mgr,
module,
ast_fn_decl,
original_name,
is_entry,
is_original_entry,
None,
Expand Down Expand Up @@ -443,6 +445,7 @@ pub(super) fn compile_entry_function(
md_mgr,
module,
&ast_fn_decl,
&ast_fn_decl.name,
logged_types_map,
messages_types_map,
is_entry,
Expand Down Expand Up @@ -489,6 +492,11 @@ fn compile_fn(
md_mgr: &mut MetadataManager,
module: Module,
ast_fn_decl: &ty::TyFunctionDecl,
// Original function name, before it is postfixed with
// a number, to get a unique name.
// The span in the name must point to the name in the
// function declaration.
original_name: &Ident,
is_entry: bool,
is_original_entry: bool,
selector: Option<[u8; 4]>,
Expand Down Expand Up @@ -567,7 +575,9 @@ fn compile_fn(

let span_md_idx = md_mgr.span_to_md(context, span);
let storage_md_idx = md_mgr.purity_to_md(context, *purity);
let name_span_md_idx = md_mgr.fn_name_span_to_md(context, original_name);
let mut metadata = md_combine(context, &span_md_idx, &storage_md_idx);
metadata = md_combine(context, &metadata, &name_span_md_idx);

let decl_index = test_decl_ref.map(|decl_ref| *decl_ref.id());
if let Some(decl_index) = decl_index {
Expand Down Expand Up @@ -688,6 +698,7 @@ fn compile_abi_method(
md_mgr,
module,
&ast_fn_decl,
&ast_fn_decl.name,
// ABI methods are only entries when the "new encoding" is off
!context.experimental.new_encoding,
// ABI methods are always original entries
Expand Down
15 changes: 13 additions & 2 deletions sway-core/src/ir_generation/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,14 @@ impl<'eng> FnCompiler<'eng> {
)
} else {
let function_decl = self.engines.de().get_function(fn_ref);
self.compile_fn_call(context, md_mgr, arguments, &function_decl, span_md_idx)
self.compile_fn_call(
context,
md_mgr,
arguments,
&function_decl,
span_md_idx,
name,
)
}
}
ty::TyExpressionVariant::LazyOperator { op, lhs, rhs } => {
Expand Down Expand Up @@ -2868,6 +2875,7 @@ impl<'eng> FnCompiler<'eng> {
ast_args: &[(Ident, ty::TyExpression)],
callee: &ty::TyFunctionDecl,
span_md_idx: Option<MetadataIndex>,
call_path: &CallPath,
) -> Result<TerminatorValue, CompileError> {
let new_callee = self.cache.ty_function_decl_to_unique_function(
self.engines,
Expand All @@ -2893,11 +2901,14 @@ impl<'eng> FnCompiler<'eng> {
args.push(arg);
}

let call_path_span_md_idx = md_mgr.fn_call_path_span_to_md(context, call_path);
let md_idx = combine(context, &span_md_idx, &call_path_span_md_idx);

let val = self
.current_block
.append(context)
.call(new_callee, &args)
.add_metadatum(context, span_md_idx);
.add_metadatum(context, md_idx);

Ok(TerminatorValue::new(val, context))
}
Expand Down
176 changes: 134 additions & 42 deletions sway-core/src/ir_generation/purity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@ use crate::{
promote_purity,
Purity::{self, *},
},
metadata::{MetadataManager, StorageOperation},
metadata::MetadataManager,
};

use sway_error::warning::{CompileWarning, Warning};
use sway_error::{error::CompileError, handler::Handler};
use sway_error::{
error::StorageAccess,
warning::{CompileWarning, Warning},
};
use sway_ir::{Context, FuelVmInstruction, Function, InstOp};
use sway_types::span::Span;

Expand All @@ -34,44 +37,80 @@ pub(crate) fn check_function_purity(
// Iterate for each instruction in the function and gather whether we have read and/or
// write storage operations:
// - via the storage IR instructions,
// - via ASM blocks with storage VM instructions or
// - via ASM blocks with storage VM instructions, or
// - via calls into functions with the above.
let attributed_purity = md_mgr.md_to_purity(context, function.get_metadata(context));

let mut storage_access_violations = vec![];
let (reads, writes) = function.instruction_iter(context).fold(
(false, false),
|(reads, writes), (_block, ins_value)| {
ins_value
.get_instruction(context)
.map(|instruction| {
match &instruction.op {
InstOp::FuelVm(FuelVmInstruction::StateLoadQuadWord { .. })
| InstOp::FuelVm(FuelVmInstruction::StateLoadWord(_)) => (true, writes),
InstOp::FuelVm(inst) if is_store_access_fuel_vm_instruction(inst) => {
let storage_access = store_access_fuel_vm_instruction_to_storage_access(inst);
if violates_purity(&storage_access, &attributed_purity) {
// When compiling Sway code, the only way to get FuelVM store access instructions in the IR
// is via store access intrinsics. So we know that the span stored in the metadata will be
// the intrinsic's call span which is suitable for error reporting.
let intrinsic_call_span = md_mgr.md_to_span(context, ins_value.get_metadata(context)).unwrap_or(Span::dummy());
storage_access_violations.push((intrinsic_call_span, storage_access));
}

InstOp::FuelVm(FuelVmInstruction::StateClear { .. })
| InstOp::FuelVm(FuelVmInstruction::StateStoreQuadWord { .. })
| InstOp::FuelVm(FuelVmInstruction::StateStoreWord { .. }) => (reads, true),
match inst {
FuelVmInstruction::StateLoadQuadWord { .. }
| FuelVmInstruction::StateLoadWord(_) => (true, writes),
FuelVmInstruction::StateClear { .. }
| FuelVmInstruction::StateStoreQuadWord { .. }
| FuelVmInstruction::StateStoreWord { .. } => (reads, true),
_ => unreachable!("The FuelVM instruction is checked to be a store access instruction."),
}
}

// Iterate for and check each instruction in the ASM block.
InstOp::AsmBlock(asm_block, _args) => asm_block.body.iter().fold(
(reads, writes),
|(reads, writes), asm_op| match asm_op.op_name.as_str() {
"scwq" | "srw" | "srwq" => (true, writes),
"sww" | "swwq" => (reads, true),
_ => (reads, writes),
},
|(reads, writes), asm_op| {
let inst = asm_op.op_name.as_str();
if is_store_access_asm_instruction(inst) {
let storage_access = store_access_asm_instruction_to_storage_access(inst);
if violates_purity(&storage_access, &attributed_purity) {
let asm_inst_span = md_mgr.md_to_span(context, asm_op.metadata).unwrap_or(Span::dummy());
storage_access_violations.push((asm_inst_span, storage_access));
}

match inst {
"srw" | "srwq" => (true, writes),
"scwq" | "sww" | "swwq" => (reads, true),
_ => unreachable!("The ASM instruction is checked to be a store access instruction."),
}
} else {
(reads, writes)
}
}
),

// Recurse to find the called function purity. Use memoisation to
// avoid redoing work.
InstOp::Call(callee, _args) => {
let (called_fn_reads, called_fn_writes) =
let (callee_reads, callee_writes) =
env.memos.get(callee).copied().unwrap_or_else(|| {
let r_w = check_function_purity(
handler, env, context, md_mgr, callee,
);
env.memos.insert(*callee, r_w);
r_w
});
(reads || called_fn_reads, writes || called_fn_writes)
if callee_reads || callee_writes {
let callee_span = md_mgr.md_to_fn_call_path_span(context, ins_value.get_metadata(context)).unwrap_or(Span::dummy());
let storage_access = StorageAccess::ImpureFunctionCall(callee_span.clone(), callee_reads, callee_writes);
if violates_purity(&storage_access, &attributed_purity) {
storage_access_violations.push((callee_span, storage_access));
}
}
(reads || callee_reads, writes || callee_writes)
}

_otherwise => (reads, writes),
Expand All @@ -81,19 +120,21 @@ pub(crate) fn check_function_purity(
},
);

let attributed_purity = md_mgr.md_to_storage_op(context, function.get_metadata(context));
let span = md_mgr
.md_to_span(context, function.get_metadata(context))
.unwrap_or_else(Span::dummy);

// Simple closures for each of the error types.
let error = |span, storage_op, existing, needed| {
handler.emit_err(CompileError::ImpureInPureContext {
storage_op,
attrs: promote_purity(existing, needed).to_attribute_syntax(),
span,
});
let error = |span: Span, needed| {
// We don't emit errors on the generated `__entry` function
// but do on the original entry functions and all other functions.
if !function.is_entry(context) || function.is_original_entry(context) {
handler.emit_err(CompileError::StorageAccessMismatched {
span,
is_pure: matches!(attributed_purity, Pure),
suggested_attributes: promote_purity(attributed_purity, needed)
.to_attribute_syntax(),
storage_access_violations,
});
}
};

let warn = |span, purity: Purity| {
// Do not warn on generated code
if span != Span::dummy() {
Expand All @@ -106,28 +147,79 @@ pub(crate) fn check_function_purity(
}
};

let span = md_mgr
.md_to_fn_name_span(context, function.get_metadata(context))
.unwrap_or_else(Span::dummy);

match (attributed_purity, reads, writes) {
// Has no attributes but needs some.
(None, true, false) => error(span, "read", Pure, Reads),
(None, false, true) => error(span, "write", Pure, Writes),
(None, true, true) => error(span, "read & write", Pure, ReadsWrites),
(Pure, true, false) => error(span, Reads),
(Pure, false, true) => error(span, Writes),
(Pure, true, true) => error(span, ReadsWrites),

// Or the attribute must match the behaviour.
(Some(StorageOperation::Reads), _, true) => error(span, "write", Reads, Writes),
// Or the attribute must match the behavior.
(Reads, _, true) => error(span, Writes),

// Or we have unneeded attributes.
(Some(StorageOperation::ReadsWrites), false, true) => warn(span, Reads),
(Some(StorageOperation::ReadsWrites), true, false) => warn(span, Writes),
(Some(StorageOperation::ReadsWrites), false, false) => warn(span, ReadsWrites),
(Some(StorageOperation::Reads), false, false) => warn(span, Reads),
(Some(StorageOperation::Writes), _, false) => warn(span, Writes),

// Attributes and effects are in total agreement
(None, false, false)
| (Some(StorageOperation::Reads), true, false)
| (Some(StorageOperation::Writes), _, true) // storage(write) allows reading as well
| (Some(StorageOperation::ReadsWrites), true, true) => (),
(ReadsWrites, false, true) => warn(span, Reads),
(ReadsWrites, true, false) => warn(span, Writes),
(ReadsWrites, false, false) => warn(span, ReadsWrites),
(Reads, false, false) => warn(span, Reads),
(Writes, _, false) => warn(span, Writes),

// Attributes and effects are in total agreement.
(Pure, false, false)
| (Reads, true, false)
| (Writes, _, true) // storage(write) allows reading as well
| (ReadsWrites, true, true) => (),
};

(reads, writes)
}

fn is_store_access_fuel_vm_instruction(inst: &FuelVmInstruction) -> bool {
matches!(
inst,
FuelVmInstruction::StateLoadWord(_)
| FuelVmInstruction::StateLoadQuadWord { .. }
| FuelVmInstruction::StateClear { .. }
| FuelVmInstruction::StateStoreWord { .. }
| FuelVmInstruction::StateStoreQuadWord { .. }
)
}

fn store_access_fuel_vm_instruction_to_storage_access(inst: &FuelVmInstruction) -> StorageAccess {
match inst {
FuelVmInstruction::StateLoadWord(_) => StorageAccess::ReadWord,
FuelVmInstruction::StateLoadQuadWord { .. } => StorageAccess::ReadSlots,
FuelVmInstruction::StateClear { .. } => StorageAccess::Clear,
FuelVmInstruction::StateStoreWord { .. } => StorageAccess::WriteWord,
FuelVmInstruction::StateStoreQuadWord { .. } => StorageAccess::WriteSlots,
_ => panic!("The FuelVM instruction is not a store access instruction."),
}
}

fn is_store_access_asm_instruction(inst: &str) -> bool {
matches!(inst, "srw" | "srwq" | "scwq" | "sww" | "swwq")
}

fn store_access_asm_instruction_to_storage_access(inst: &str) -> StorageAccess {
match inst {
"srw" => StorageAccess::ReadWord,
"srwq" => StorageAccess::ReadSlots,
"scwq" => StorageAccess::Clear,
"sww" => StorageAccess::WriteWord,
"swwq" => StorageAccess::WriteSlots,
_ => panic!("The ASM instruction \"{inst}\" is not a store access instruction."),
}
}

/// Returns true if the `storage_access` violates the given expected `purity`.
fn violates_purity(storage_access: &StorageAccess, purity: &Purity) -> bool {
match purity {
Pure => true,
Reads => storage_access.is_write(),
Writes => false,
ReadsWrites => false,
}
}
Loading

0 comments on commit 7731c6b

Please sign in to comment.