[move unit tests] Run extended checker as part of unit tests (aptos-l…

…abs#10309)

* [move unit tests] Run extended checker as part of unit tests

Closes aptos-labs#9251

This runs the extended checker as part of Aptos unit tests (either our own Rust integrated tests or from the CLI). It uses the same technique as we already used for native extensions specific to Aptos: a hook is defined where additional, move-model based validations can be run. This is hook is then connected to the extended checker when running Aptos tests.

The implementation also optimizes the construction of the move model: if that one is already needed by abi generation (which is the default), it is not constructed a 2nd time for the extended checker -- both  for the existing build step and the new test step. This should avoid one full additional compilation (source -> bytecode -> model run).

* Extended checks until now excluded test code, leading to wrong usage of entry functions and attributes marked as test-only. Because fixing this is a breaking change, this commit adds the behavior to check test code via a new CLI option `--check-test-code`. This flag should eventually become default behavior.

Also fixes some reviewer comments.

aptos-move/framework/Cargo.toml

@@ -53,6 +53,7 @@ log = { workspace = true }
 lru = { workspace = true }
 merlin = { workspace = true }
 move-binary-format = { workspace = true }
+move-cli = { workspace = true }
 move-command-line-common = { workspace = true }
 move-compiler = { workspace = true }
 move-core-types = { workspace = true }

aptos-move/framework/src/aptos.rs

@@ -121,6 +121,7 @@ impl ReleaseTarget {
                 bytecode_version: None,
                 compiler_version: None,
                 skip_attribute_checks: false,
+                check_test_code: false,
                 known_attributes: extended_checks::get_all_attribute_names().clone(),
             },
             packages: packages.iter().map(|(path, _)| path.to_owned()).collect(),

aptos-move/framework/src/built_package.rs

@@ -72,6 +72,8 @@ pub struct BuildOptions {
     pub compiler_version: Option<CompilerVersion>,
     #[clap(long)]
     pub skip_attribute_checks: bool,
+    #[clap(long)]
+    pub check_test_code: bool,
     #[clap(skip)]
     pub known_attributes: BTreeSet<String>,
 }
@@ -96,6 +98,7 @@ impl Default for BuildOptions {
             bytecode_version: None,
             compiler_version: None,
             skip_attribute_checks: false,
+            check_test_code: false,
             known_attributes: extended_checks::get_all_attribute_names().clone(),
         }
     }
@@ -125,6 +128,8 @@ pub fn build_model(
         architecture: None,
         generate_abis: false,
         generate_docs: false,
+        generate_move_model: false,
+        full_model_generation: false,
         install_dir: None,
         test_mode: false,
         force_recompilation: false,
@@ -158,6 +163,8 @@ impl BuiltPackage {
             architecture: None,
             generate_abis: options.with_abis,
             generate_docs: false,
+            generate_move_model: true,
+            full_model_generation: options.check_test_code,
             install_dir: options.install_dir.clone(),
             test_mode: false,
             force_recompilation: false,
@@ -172,20 +179,11 @@ impl BuiltPackage {
         };
 
         eprintln!("Compiling, may take a little while to download git dependencies...");
-        let mut package = build_config.compile_package_no_exit(&package_path, &mut stderr())?;
+        let (mut package, model_opt) =
+            build_config.compile_package_no_exit(&package_path, &mut stderr())?;
 
-        // Build the Move model for extra processing and run extended checks as well derive
-        // runtime metadata
-        let model = &build_model(
-            options.dev,
-            package_path.as_path(),
-            options.named_addresses.clone(),
-            None,
-            bytecode_version,
-            compiler_version,
-            skip_attribute_checks,
-            options.known_attributes.clone(),
-        )?;
+        // Run extended checks as well derive runtime metadata
+        let model = &model_opt.expect("move model");
         let runtime_metadata = extended_checks::run_extended_checks(model);
         if model.diag_count(Severity::Warning) > 0 {
             let mut error_writer = StandardStream::stderr(ColorChoice::Auto);

aptos-move/framework/src/extended_checks.rs

@@ -3,6 +3,7 @@
 
 use crate::{KnownAttribute, RuntimeModuleMetadataV1};
 use move_binary_format::file_format::{Ability, AbilitySet, Visibility};
+use move_cli::base::test_validation;
 use move_compiler::shared::known_attributes;
 use move_core_types::{
     account_address::AccountAddress,
@@ -77,6 +78,14 @@ pub fn run_extended_checks(env: &GlobalEnv) -> BTreeMap<ModuleId, RuntimeModuleM
     checker.output
 }
 
+/// Configures the move-cli unit test validation hook to run the extended checker.
+pub fn configure_extended_checks_for_unit_test() {
+    fn validate(env: &GlobalEnv) {
+        run_extended_checks(env);
+    }
+    test_validation::set_validation_hook(Box::new(validate));
+}
+
 #[derive(Debug)]
 struct ExtendedChecker<'a> {
     env: &'a GlobalEnv,

aptos-move/framework/tests/move_unit_test.rs

@@ -41,6 +41,7 @@ fn run_tests_for_pkg(path_to_pkg: impl Into<String>) {
 pub fn aptos_test_natives() -> NativeFunctionTable {
     // By side effect, configure for unit tests
     natives::configure_for_unit_test();
+    extended_checks::configure_extended_checks_for_unit_test();
     // move_stdlib has the testing feature enabled to include debug native functions
     natives::aptos_natives(
         LATEST_GAS_FEATURE_VERSION,

aptos-move/move-examples/tests/move_unit_tests.rs

@@ -56,6 +56,7 @@ pub fn run_tests_for_pkg(
 
 pub fn aptos_test_natives() -> NativeFunctionTable {
     natives::configure_for_unit_test();
+    extended_checks::configure_extended_checks_for_unit_test();
     natives::aptos_natives(
         LATEST_GAS_FEATURE_VERSION,
         NativeGasParameters::zeros(),

crates/aptos/src/common/types.rs

@@ -1125,6 +1125,12 @@ pub struct MovePackageDir {
     /// Do not complain about unknown attributes in Move code.
     #[clap(long)]
     pub skip_attribute_checks: bool,
+
+    /// Do apply extended checks for Aptos (e.g. `#[view]` attribute) also on test code.
+    /// NOTE: this behavior will become the default in the future.
+    /// See https://github.com/aptos-labs/aptos-core/issues/10335
+    #[clap(long, env = "APTOS_CHECK_TEST_CODE")]
+    pub check_test_code: bool,
 }
 
 impl MovePackageDir {
@@ -1138,6 +1144,7 @@ impl MovePackageDir {
             bytecode_version: None,
             compiler_version: None,
             skip_attribute_checks: false,
+            check_test_code: false,
         }
     }
 

crates/aptos/src/governance/mod.rs

@@ -998,6 +998,7 @@ impl CliCommand<()> for GenerateUpgradeProposal {
             move_options.bytecode_version,
             move_options.compiler_version,
             move_options.skip_attribute_checks,
+            move_options.check_test_code,
         );
         let package = BuiltPackage::build(package_path, options)?;
         let release = ReleasePackage::new(package)?;

crates/aptos/src/move_tool/aptos_debug_natives.rs

@@ -1,6 +1,7 @@
 // Copyright © Aptos Foundation
 // SPDX-License-Identifier: Apache-2.0
 
+use aptos_framework::extended_checks;
 use aptos_gas_schedule::{MiscGasParameters, NativeGasParameters, LATEST_GAS_FEATURE_VERSION};
 use aptos_types::on_chain_config::{Features, TimedFeaturesBuilder};
 use aptos_vm::natives;
@@ -13,6 +14,7 @@ pub fn aptos_debug_natives(
 ) -> NativeFunctionTable {
     // As a side effect, also configure for unit testing
     natives::configure_for_unit_test();
+    extended_checks::configure_extended_checks_for_unit_test();
     // Return all natives -- build with the 'testing' feature, therefore containing
     // debug related functions.
     natives::aptos_natives(

crates/aptos/src/move_tool/mod.rs

@@ -33,8 +33,8 @@ use crate::{
 };
 use aptos_crypto::HashValue;
 use aptos_framework::{
-    build_model, docgen::DocgenOptions, extended_checks, natives::code::UpgradePolicy,
-    prover::ProverOptions, BuildOptions, BuiltPackage,
+    docgen::DocgenOptions, extended_checks, natives::code::UpgradePolicy, prover::ProverOptions,
+    BuildOptions, BuiltPackage,
 };
 use aptos_gas_schedule::{MiscGasParameters, NativeGasParameters};
 use aptos_rest_client::aptos_api_types::{
@@ -46,10 +46,6 @@ use aptos_types::{
 };
 use async_trait::async_trait;
 use clap::{Parser, Subcommand, ValueEnum};
-use codespan_reporting::{
-    diagnostic::Severity,
-    term::termcolor::{ColorChoice, StandardStream},
-};
 use itertools::Itertools;
 use move_cli::{self, base::test::UnitTestResult};
 use move_command_line_common::env::MOVE_HOME;
@@ -317,6 +313,7 @@ impl CliCommand<Vec<String>> for CompilePackage {
                     self.move_options.bytecode_version,
                     self.move_options.compiler_version,
                     self.move_options.skip_attribute_checks,
+                    self.move_options.check_test_code,
                 )
         };
         let pack = BuiltPackage::build(self.move_options.get_package_path()?, build_options)
@@ -378,6 +375,7 @@ impl CompileScript {
                 self.move_options.bytecode_version,
                 self.move_options.compiler_version,
                 self.move_options.skip_attribute_checks,
+                self.move_options.check_test_code,
             )
         };
         let package_dir = self.move_options.get_package_path()?;
@@ -455,6 +453,7 @@ impl CliCommand<&'static str> for TestPackage {
             dev_mode: self.move_options.dev,
             additional_named_addresses: self.move_options.named_addresses(),
             test_mode: true,
+            full_model_generation: self.move_options.check_test_code,
             install_dir: self.move_options.output_dir.clone(),
             skip_fetch_latest_git_deps: self.move_options.skip_fetch_latest_git_deps,
             compiler_config: CompilerConfig {
@@ -465,27 +464,6 @@ impl CliCommand<&'static str> for TestPackage {
             ..Default::default()
         };
 
-        // Build the Move model for extended checks
-        let model = &build_model(
-            self.move_options.dev,
-            self.move_options.get_package_path()?.as_path(),
-            self.move_options.named_addresses(),
-            None,
-            self.move_options.bytecode_version,
-            self.move_options.compiler_version,
-            self.move_options.skip_attribute_checks,
-            known_attributes.clone(),
-        )?;
-        let _ = extended_checks::run_extended_checks(model);
-        if model.diag_count(Severity::Warning) > 0 {
-            let mut error_writer = StandardStream::stderr(ColorChoice::Auto);
-            model.report_diag(&mut error_writer, Severity::Warning);
-            if model.has_errors() {
-                return Err(CliError::MoveCompilationError(
-                    "extended checks failed".to_string(),
-                ));
-            }
-        }
         let path = self.move_options.get_package_path()?;
         let result = move_cli::base::test::run_move_unit_tests(
             path.as_path(),
@@ -611,6 +589,7 @@ impl CliCommand<&'static str> for DocumentPackage {
             bytecode_version: move_options.bytecode_version,
             compiler_version: move_options.compiler_version,
             skip_attribute_checks: move_options.skip_attribute_checks,
+            check_test_code: move_options.check_test_code,
             known_attributes: extended_checks::get_all_attribute_names().clone(),
         };
         BuiltPackage::build(move_options.get_package_path()?, build_options)?;
@@ -679,6 +658,7 @@ impl TryInto<PackagePublicationData> for &PublishPackage {
                 self.move_options.bytecode_version,
                 self.move_options.compiler_version,
                 self.move_options.skip_attribute_checks,
+                self.move_options.check_test_code,
             );
         let package = BuiltPackage::build(package_path, options)
             .map_err(|e| CliError::MoveCompilationError(format!("{:#}", e)))?;
@@ -748,6 +728,7 @@ impl IncludedArtifacts {
         bytecode_version: Option<u32>,
         compiler_version: Option<CompilerVersion>,
         skip_attribute_checks: bool,
+        check_test_code: bool,
     ) -> BuildOptions {
         use IncludedArtifacts::*;
         match self {
@@ -763,6 +744,7 @@ impl IncludedArtifacts {
                 bytecode_version,
                 compiler_version,
                 skip_attribute_checks,
+                check_test_code,
                 known_attributes: extended_checks::get_all_attribute_names().clone(),
                 ..BuildOptions::default()
             },
@@ -777,6 +759,7 @@ impl IncludedArtifacts {
                 bytecode_version,
                 compiler_version,
                 skip_attribute_checks,
+                check_test_code,
                 known_attributes: extended_checks::get_all_attribute_names().clone(),
                 ..BuildOptions::default()
             },
@@ -791,6 +774,7 @@ impl IncludedArtifacts {
                 bytecode_version,
                 compiler_version,
                 skip_attribute_checks,
+                check_test_code,
                 known_attributes: extended_checks::get_all_attribute_names().clone(),
                 ..BuildOptions::default()
             },
@@ -935,6 +919,7 @@ impl CliCommand<TransactionSummary> for CreateResourceAccountAndPublishPackage {
             move_options.bytecode_version,
             move_options.compiler_version,
             move_options.skip_attribute_checks,
+            move_options.check_test_code,
         );
         let package = BuiltPackage::build(package_path, options)?;
         let compiled_units = package.extract_code();
@@ -1074,6 +1059,7 @@ impl CliCommand<&'static str> for VerifyPackage {
                 self.move_options.bytecode_version,
                 self.move_options.compiler_version,
                 self.move_options.skip_attribute_checks,
+                self.move_options.check_test_code,
             )
         };
         let pack = BuiltPackage::build(self.move_options.get_package_path()?, build_options)

crates/aptos/src/move_tool/show.rs

@@ -66,6 +66,7 @@ impl CliCommand<Vec<EntryABI>> for ShowAbi {
                     self.move_options.bytecode_version,
                     self.move_options.compiler_version,
                     self.move_options.skip_attribute_checks,
+                    self.move_options.check_test_code,
                 )
         };
 

crates/aptos/src/test/mod.rs

@@ -1048,6 +1048,7 @@ impl CliTestFramework {
             bytecode_version: None,
             compiler_version: None,
             skip_attribute_checks: false,
+            check_test_code: false,
         }
     }
 

third_party/move/move-compiler/src/shared/mod.rs

@@ -379,6 +379,19 @@ impl Flags {
         }
     }
 
+    pub fn all_functions() -> Self {
+        Self {
+            test: true,
+            verify: true,
+            shadow: false,
+            flavor: "".to_string(),
+            bytecode_version: None,
+            keep_testing_functions: false,
+            skip_attribute_checks: false,
+            debug: debug_compiler_env_var(),
+        }
+    }
+
     pub fn verification() -> Self {
         Self {
             test: false,

third_party/move/move-model/src/ast.rs

@@ -73,6 +73,18 @@ pub enum Attribute {
     Assign(NodeId, Symbol, AttributeValue),
 }
 
+impl Attribute {
+    pub fn name(&self) -> Symbol {
+        match self {
+            Attribute::Assign(_, s, _) | Attribute::Apply(_, s, _) => *s,
+        }
+    }
+
+    pub fn has(attrs: &[Attribute], pred: impl Fn(&Attribute) -> bool) -> bool {
+        attrs.iter().any(pred)
+    }
+}
+
 // =================================================================================================
 /// # Conditions