Fix scwq instruction and "phantom" error in purity checks

docs/book/src/blockchain-development/purity.md

@@ -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
@@ -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.
@@ -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.

sway-ast/src/assignable.rs

@@ -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.

sway-core/src/ir_generation.rs

@@ -93,6 +93,7 @@ impl CompiledFunctionCache {
                     md_mgr,
                     module,
                     &callee_fn_decl,
+                    &decl.name,
                     logged_types_map,
                     messages_types_map,
                     is_entry,

sway-core/src/ir_generation/compile.rs

@@ -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};
 
@@ -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,
@@ -408,6 +409,7 @@ pub(super) fn compile_function(
             md_mgr,
             module,
             ast_fn_decl,
+            original_name,
             is_entry,
             is_original_entry,
             None,
@@ -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,
@@ -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]>,
@@ -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 {
@@ -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

sway-core/src/ir_generation/function.rs

@@ -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 } => {
@@ -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,
@@ -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))
     }

sway-core/src/ir_generation/purity.rs

@@ -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;
 
@@ -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),
@@ -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() {
@@ -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,
+    }
+}