Skip to content

Commit

Permalink
[compiler] Fix issue-8875 thoroughly: error for unknown attributes, a…
Browse files Browse the repository at this point in the history
…dd --skip-attribute-check flag
  • Loading branch information
brmataptos committed Aug 5, 2023
1 parent cda4ae6 commit c123a0f
Show file tree
Hide file tree
Showing 57 changed files with 714 additions and 152 deletions.
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ use move_transactional_test_runner::{
use move_vm_runtime::session::SerializedReturnValues;
use once_cell::sync::Lazy;
use std::{
collections::{BTreeMap, HashMap},
collections::{BTreeMap, BTreeSet, HashMap},
convert::TryFrom,
fmt,
path::Path,
Expand Down Expand Up @@ -294,6 +294,7 @@ static PRECOMPILED_APTOS_FRAMEWORK: Lazy<FullyCompiledProgram> = Lazy::new(|| {
deps,
None,
move_compiler::Flags::empty().set_sources_shadow_deps(false),
aptos_framework::extended_checks::get_all_attribute_names(),
)
.unwrap();
match program_res {
Expand Down Expand Up @@ -551,6 +552,10 @@ impl<'a> MoveTestAdapter<'a> for AptosTestAdapter<'a> {
self.default_syntax
}

fn known_attributes(&self) -> &BTreeSet<String> {
aptos_framework::extended_checks::get_all_attribute_names()
}

fn init(
default_syntax: SyntaxChoice,
_comparison_mode: bool,
Expand Down
7 changes: 5 additions & 2 deletions aptos-move/framework/src/aptos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
#![forbid(unsafe_code)]

use crate::{
docgen::DocgenOptions, path_in_crate, release_builder::RELEASE_BUNDLE_EXTENSION,
release_bundle::ReleaseBundle, BuildOptions, ReleaseOptions,
docgen::DocgenOptions, extended_checks, path_in_crate,
release_builder::RELEASE_BUNDLE_EXTENSION, release_bundle::ReleaseBundle, BuildOptions,
ReleaseOptions,
};
use clap::ValueEnum;
use move_command_line_common::address::NumericalAddress;
Expand Down Expand Up @@ -118,6 +119,8 @@ impl ReleaseTarget {
}),
skip_fetch_latest_git_deps: true,
bytecode_version: None,
skip_attribute_checks: false,
known_attributes: extended_checks::get_all_attribute_names().clone(),
},
packages: packages.iter().map(|(path, _)| path.to_owned()).collect(),
rust_bindings: packages
Expand Down
16 changes: 16 additions & 0 deletions aptos-move/framework/src/built_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ pub struct BuildOptions {
pub skip_fetch_latest_git_deps: bool,
#[clap(long)]
pub bytecode_version: Option<u32>,
#[clap(long)]
pub skip_attribute_checks: bool,
#[clap(skip)]
pub known_attributes: BTreeSet<String>,
}

// Because named_addresses has no parser, we can't use clap's default impl. This must be aligned
Expand All @@ -88,6 +92,8 @@ impl Default for BuildOptions {
// while in a test (and cause some havoc)
skip_fetch_latest_git_deps: false,
bytecode_version: None,
skip_attribute_checks: false,
known_attributes: extended_checks::get_all_attribute_names().clone(),
}
}
}
Expand All @@ -106,6 +112,8 @@ pub fn build_model(
additional_named_addresses: BTreeMap<String, AccountAddress>,
target_filter: Option<String>,
bytecode_version: Option<u32>,
skip_attribute_checks: bool,
known_attributes: BTreeSet<String>,
) -> anyhow::Result<GlobalEnv> {
let build_config = BuildConfig {
dev_mode,
Expand All @@ -119,6 +127,8 @@ pub fn build_model(
fetch_deps_only: false,
skip_fetch_latest_git_deps: true,
bytecode_version,
skip_attribute_checks,
known_attributes,
};
build_config.move_model_for_package(package_path, ModelConfig {
target_filter,
Expand All @@ -133,6 +143,7 @@ impl BuiltPackage {
/// and is not `Ok` if there was an error among those.
pub fn build(package_path: PathBuf, options: BuildOptions) -> anyhow::Result<Self> {
let bytecode_version = options.bytecode_version;
let skip_attribute_checks = options.skip_attribute_checks;
let build_config = BuildConfig {
dev_mode: options.dev,
additional_named_addresses: options.named_addresses.clone(),
Expand All @@ -145,7 +156,10 @@ impl BuiltPackage {
fetch_deps_only: false,
skip_fetch_latest_git_deps: options.skip_fetch_latest_git_deps,
bytecode_version,
skip_attribute_checks,
known_attributes: options.known_attributes.clone(),
};

eprintln!("Compiling, may take a little while to download git dependencies...");
let mut package = build_config.compile_package_no_exit(&package_path, &mut stderr())?;

Expand All @@ -157,6 +171,8 @@ impl BuiltPackage {
options.named_addresses.clone(),
None,
bytecode_version,
skip_attribute_checks,
options.known_attributes.clone(),
)?;
let runtime_metadata = extended_checks::run_extended_checks(model);
if model.diag_count(Severity::Warning) > 0 {
Expand Down
36 changes: 33 additions & 3 deletions aptos-move/framework/src/extended_checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use crate::{KnownAttribute, RuntimeModuleMetadataV1};
use move_binary_format::file_format::{Ability, AbilitySet, Visibility};
use move_compiler::shared::known_attributes;
use move_core_types::{
account_address::AccountAddress,
errmap::{ErrorDescription, ErrorMapping},
Expand All @@ -18,18 +19,47 @@ use move_model::{
symbol::Symbol,
ty::{PrimitiveType, ReferenceKind, Type},
};
use std::{collections::BTreeMap, rc::Rc, str::FromStr};
use once_cell::sync::Lazy;
use std::{
collections::{BTreeMap, BTreeSet},
rc::Rc,
str::FromStr,
};
use thiserror::Error;

const INIT_MODULE_FUN: &str = "init_module";
const LEGAC_ENTRY_FUN_ATTRIBUTE: &str = "legacy_entry_fun";
const LEGACY_ENTRY_FUN_ATTRIBUTE: &str = "legacy_entry_fun";
const ERROR_PREFIX: &str = "E";
const RESOURCE_GROUP: &str = "resource_group";
const RESOURCE_GROUP_MEMBER: &str = "resource_group_member";
const RESOURCE_GROUP_NAME: &str = "group";
const RESOURCE_GROUP_SCOPE: &str = "scope";
const VIEW_FUN_ATTRIBUTE: &str = "view";

// top-level attribute names, only.
pub fn get_all_attribute_names() -> &'static BTreeSet<String> {
const ALL_ATTRIBUTE_NAMES: [&str; 4] = [
LEGACY_ENTRY_FUN_ATTRIBUTE,
RESOURCE_GROUP,
RESOURCE_GROUP_MEMBER,
VIEW_FUN_ATTRIBUTE,
];

fn add_attribute_names(table: &mut BTreeSet<String>) {
for str in ALL_ATTRIBUTE_NAMES {
table.insert(str.to_string());
}
}
static KNOWN_ATTRIBUTES_SET: Lazy<BTreeSet<String>> = Lazy::new(|| {
use known_attributes::AttributeKind;
let mut known_attributes = BTreeSet::new();
known_attributes::KnownAttribute::add_attribute_names(&mut known_attributes);
add_attribute_names(&mut known_attributes);
known_attributes
});
&KNOWN_ATTRIBUTES_SET
}

/// Run the extended context checker on target modules in the environment and returns a map
/// from module to extended runtime metadata. Any errors during context checking are reported to
/// `env`. This is invoked after general build succeeds.
Expand Down Expand Up @@ -118,7 +148,7 @@ impl<'a> ExtendedChecker<'a> {
if !fun.is_entry() {
continue;
}
if self.has_attribute(fun, LEGAC_ENTRY_FUN_ATTRIBUTE) {
if self.has_attribute(fun, LEGACY_ENTRY_FUN_ATTRIBUTE) {
// Skip checking for legacy entries
continue;
}
Expand Down
10 changes: 9 additions & 1 deletion aptos-move/framework/src/prover.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ use codespan_reporting::{
};
use log::LevelFilter;
use move_core_types::account_address::AccountAddress;
use std::{collections::BTreeMap, path::Path, time::Instant};
use std::{
collections::{BTreeMap, BTreeSet},
path::Path,
time::Instant,
};
use tempfile::TempDir;

#[derive(Debug, Clone, clap::Parser, serde::Serialize, serde::Deserialize)]
Expand Down Expand Up @@ -114,6 +118,8 @@ impl ProverOptions {
package_path: &Path,
named_addresses: BTreeMap<String, AccountAddress>,
bytecode_version: Option<u32>,
skip_attribute_checks: bool,
known_attributes: &BTreeSet<String>,
) -> anyhow::Result<()> {
let now = Instant::now();
let for_test = self.for_test;
Expand All @@ -123,6 +129,8 @@ impl ProverOptions {
named_addresses,
self.filter.clone(),
bytecode_version,
skip_attribute_checks,
known_attributes.clone(),
)?;
let mut options = self.convert_options();
// Need to ensure a distinct output.bpl file for concurrent execution. In non-test
Expand Down
12 changes: 10 additions & 2 deletions aptos-move/framework/tests/move_prover_tests.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright © Aptos Foundation
// SPDX-License-Identifier: Apache-2.0

use aptos_framework::prover::ProverOptions;
use aptos_framework::{extended_checks, prover::ProverOptions};
use std::{collections::BTreeMap, path::PathBuf};

const ENV_TEST_INCONSISTENCY: &str = "MVP_TEST_INCONSISTENCY";
Expand Down Expand Up @@ -51,8 +51,16 @@ pub fn run_prover_for_pkg(path_to_pkg: impl Into<String>) {
options.vc_timeout = read_env_var(ENV_TEST_VC_TIMEOUT)
.parse::<usize>()
.unwrap_or(options.vc_timeout);
let skip_attribute_checks = false;
options
.prove(false, pkg_path.as_path(), BTreeMap::default(), None)
.prove(
false,
pkg_path.as_path(),
BTreeMap::default(),
None,
skip_attribute_checks,
extended_checks::get_all_attribute_names(),
)
.unwrap()
}
}
Expand Down
3 changes: 2 additions & 1 deletion aptos-move/framework/tests/move_unit_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Parts of the project are originally copyright © Meta Platforms, Inc.
// SPDX-License-Identifier: Apache-2.0

use aptos_framework::path_in_crate;
use aptos_framework::{extended_checks, path_in_crate};
use aptos_gas_schedule::{MiscGasParameters, NativeGasParameters, LATEST_GAS_FEATURE_VERSION};
use aptos_types::on_chain_config::{Features, TimedFeatures};
use aptos_vm::natives;
Expand All @@ -18,6 +18,7 @@ fn run_tests_for_pkg(path_to_pkg: impl Into<String>) {
move_package::BuildConfig {
test_mode: true,
install_dir: Some(tempdir().unwrap().path().to_path_buf()),
known_attributes: extended_checks::get_all_attribute_names().clone(),
..Default::default()
},
// TODO(Gas): double check if this is correct
Expand Down
1 change: 1 addition & 0 deletions aptos-move/move-examples/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ repository = { workspace = true }
rust-version = { workspace = true }

[dependencies]
aptos-framework = { workspace = true }
aptos-gas-schedule = { workspace = true }
aptos-types = { workspace = true }
aptos-vm ={ workspace = true, features = ["testing"] }
Expand Down
2 changes: 2 additions & 0 deletions aptos-move/move-examples/tests/move_prover_tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright © Aptos Foundation
// SPDX-License-Identifier: Apache-2.0

use aptos_framework::extended_checks;
use aptos_types::account_address::AccountAddress;
use move_cli::base::prove::run_move_prover;
use std::{collections::BTreeMap, path::PathBuf};
Expand All @@ -24,6 +25,7 @@ pub fn run_prover_for_pkg(
additional_named_addresses: named_addr,
test_mode: true,
install_dir: Some(tempdir().unwrap().path().to_path_buf()),
known_attributes: extended_checks::get_all_attribute_names().clone(),
..Default::default()
};
run_move_prover(
Expand Down
2 changes: 2 additions & 0 deletions aptos-move/move-examples/tests/move_unit_tests.rs
Original file line number Diff line number Diff line change
@@ -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::{
account_address::{create_resource_address, AccountAddress},
Expand Down Expand Up @@ -33,6 +34,7 @@ pub fn run_tests_for_pkg(
test_mode: true,
install_dir: Some(tempdir().unwrap().path().to_path_buf()),
additional_named_addresses: named_addr,
known_attributes: extended_checks::get_all_attribute_names().clone(),
..Default::default()
},
UnitTestingConfig::default_with_bound(Some(100_000)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,11 @@ pub fn compile_script(source_file_str: String, bytecode_version: Option<u32>) ->
.files()
.unwrap(),
aptos_framework::named_addresses().clone(),
Flags::empty()
.set_sources_shadow_deps(false)
.set_skip_attribute_checks(false),
aptos_framework::extended_checks::get_all_attribute_names(),
)
.set_flags(Flags::empty().set_sources_shadow_deps(false))
.build_and_report()
.unwrap();
assert!(compiled_program.len() == 1);
Expand Down
5 changes: 5 additions & 0 deletions crates/aptos/src/common/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,10 @@ pub struct MovePackageDir {
/// Specify the version of the bytecode the compiler is going to emit.
#[clap(long)]
pub bytecode_version: Option<u32>,

/// Do not complain about unknown attributes in Move code.
#[clap(long)]
pub skip_attribute_checks: bool,
}

impl MovePackageDir {
Expand All @@ -998,6 +1002,7 @@ impl MovePackageDir {
named_addresses: Default::default(),
skip_fetch_latest_git_deps: true,
bytecode_version: None,
skip_attribute_checks: false,
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/aptos/src/governance/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,7 @@ impl CliCommand<()> for GenerateUpgradeProposal {
move_options.skip_fetch_latest_git_deps,
move_options.named_addresses(),
move_options.bytecode_version,
move_options.skip_attribute_checks,
);
let package = BuiltPackage::build(package_path, options)?;
let release = ReleasePackage::new(package)?;
Expand Down
3 changes: 3 additions & 0 deletions crates/aptos/src/move_tool/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

use crate::common::types::{CliCommand, CliError, CliResult, CliTypedResult, MovePackageDir};
use aptos_framework::extended_checks;
use async_trait::async_trait;
use clap::{Parser, Subcommand};
use move_compiler::compiled_unit::{CompiledUnit, NamedCompiledModule};
Expand Down Expand Up @@ -149,6 +150,8 @@ fn compile_coverage(
additional_named_addresses: move_options.named_addresses(),
test_mode: false,
install_dir: move_options.output_dir.clone(),
known_attributes: extended_checks::get_all_attribute_names().clone(),
skip_attribute_checks: false,
..Default::default()
};
let path = move_options.get_package_path()?;
Expand Down
Loading

0 comments on commit c123a0f

Please sign in to comment.