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.

Sign up for GitHub

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

[compiler-v2] Externalize the linter architecture #15138

Merged
merged 1 commit into from
Nov 5, 2024
Merged

[compiler-v2] Externalize the linter architecture #15138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 16 additions & 2 deletions Cargo.lock
View file
Open in desktop

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

2 changes: 2 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ members = [
"third_party/move/tools/move-decompiler",
"third_party/move/tools/move-disassembler",
"third_party/move/tools/move-explain",
"third_party/move/tools/move-linter",
"third_party/move/tools/move-package",
"third_party/move/tools/move-resource-viewer",
"third_party/move/tools/move-unit-test",
Expand Down Expand Up @@ -853,6 +854,7 @@ move-docgen = { path = "third_party/move/move-prover/move-docgen" }
move-disassembler = { path = "third_party/move/tools/move-disassembler" }
move-ir-types = { path = "third_party/move/move-ir/types" }
move-ir-compiler = { path = "third_party/move/move-ir-compiler" }
move-linter = { path = "third_party/move/tools/move-linter" }
move-bytecode-source-map = { path = "third_party/move/move-ir-compiler/move-bytecode-source-map" }
move-model = { path = "third_party/move/move-model" }
move-package = { path = "third_party/move/tools/move-package" }
Expand Down
200 changes: 108 additions & 92 deletions aptos-move/framework/src/built_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use itertools::Itertools;
use move_binary_format::{file_format_common::VERSION_7, CompiledModule};
use move_command_line_common::files::MOVE_COMPILED_EXTENSION;
use move_compiler::compiled_unit::{CompiledUnit, NamedCompiledModule};
use move_compiler_v2::{options::Options, Experiment};
use move_compiler_v2::{external_checks::ExternalChecks, options::Options, Experiment};
use move_core_types::{language_storage::ModuleId, metadata::Metadata};
use move_model::{
metadata::{CompilerVersion, LanguageVersion},
Expand All @@ -38,6 +38,7 @@ use std::{
collections::{BTreeMap, BTreeSet},
io::{stderr, Write},
path::{Path, PathBuf},
sync::Arc,
};

pub const METADATA_FILE_NAME: &str = "package-metadata.bcs";
Expand Down Expand Up @@ -222,108 +223,123 @@ impl BuiltPackage {
/// This function currently reports all Move compilation errors and warnings to stdout,
/// 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 = Some(options.inferred_bytecode_version());
let compiler_version = options.compiler_version;
let language_version = options.language_version;
Self::check_versions(&compiler_version, &language_version)?;
let skip_attribute_checks = options.skip_attribute_checks;
let build_config = BuildConfig {
dev_mode: options.dev,
additional_named_addresses: options.named_addresses.clone(),
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,
override_std: options.override_std.clone(),
force_recompilation: false,
fetch_deps_only: false,
skip_fetch_latest_git_deps: options.skip_fetch_latest_git_deps,
compiler_config: CompilerConfig {
bytecode_version,
compiler_version,
language_version,
skip_attribute_checks,
known_attributes: options.known_attributes.clone(),
experiments: options.experiments.clone(),
},
};
BuiltPackage::build_with_external_checks(package_path, options, vec![])
}

/// Same as `build` but allows to provide external checks to be made on Move code.
/// The `external_checks` are only run when compiler v2 is used.
pub fn build_with_external_checks(
package_path: PathBuf,
options: BuildOptions,
external_checks: Vec<Arc<dyn ExternalChecks>>,
) -> anyhow::Result<Self> {
{
let bytecode_version = Some(options.inferred_bytecode_version());
let compiler_version = options.compiler_version;
let language_version = options.language_version;
Self::check_versions(&compiler_version, &language_version)?;
let skip_attribute_checks = options.skip_attribute_checks;
let build_config = BuildConfig {
dev_mode: options.dev,
additional_named_addresses: options.named_addresses.clone(),
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,
override_std: options.override_std.clone(),
force_recompilation: false,
fetch_deps_only: false,
skip_fetch_latest_git_deps: options.skip_fetch_latest_git_deps,
compiler_config: CompilerConfig {
bytecode_version,
compiler_version,
language_version,
skip_attribute_checks,
known_attributes: options.known_attributes.clone(),
experiments: options.experiments.clone(),
},
};

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

// Run extended checks as well derive runtime metadata
let model = &model_opt.expect("move model");
// Run extended checks as well derive runtime metadata
let model = &model_opt.expect("move model");

if let Some(model_options) = model.get_extension::<Options>() {
if model_options.experiment_on(Experiment::STOP_BEFORE_EXTENDED_CHECKS) {
std::process::exit(0)
if let Some(model_options) = model.get_extension::<Options>() {
if model_options.experiment_on(Experiment::STOP_BEFORE_EXTENDED_CHECKS) {
std::process::exit(0)
}
}
}

let runtime_metadata = 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() {
bail!("extended checks failed")
let runtime_metadata = 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() {
bail!("extended checks failed")
}
}
}

if let Some(model_options) = model.get_extension::<Options>() {
if model_options.experiment_on(Experiment::STOP_AFTER_EXTENDED_CHECKS) {
std::process::exit(0)
if let Some(model_options) = model.get_extension::<Options>() {
if model_options.experiment_on(Experiment::STOP_AFTER_EXTENDED_CHECKS) {
std::process::exit(0)
}
}
}

let compiled_pkg_path = package
.compiled_package_info
.build_flags
.install_dir
.as_ref()
.unwrap_or(&package_path)
.join(CompiledPackageLayout::Root.path())
.join(package.compiled_package_info.package_name.as_str());
inject_runtime_metadata(
compiled_pkg_path,
&mut package,
runtime_metadata,
bytecode_version,
)?;
let compiled_pkg_path = package
.compiled_package_info
.build_flags
.install_dir
.as_ref()
.unwrap_or(&package_path)
.join(CompiledPackageLayout::Root.path())
.join(package.compiled_package_info.package_name.as_str());
inject_runtime_metadata(
compiled_pkg_path,
&mut package,
runtime_metadata,
bytecode_version,
)?;

// If enabled generate docs.
if options.with_docs {
let docgen = if let Some(opts) = options.docgen_options.clone() {
opts
} else {
DocgenOptions::default()
};
let dep_paths = package
.deps_compiled_units
.iter()
.map(|(_, u)| {
u.source_path
.parent()
.unwrap()
.parent()
.unwrap()
.join(get_docgen_output_dir())
.display()
.to_string()
})
.unique()
.collect::<Vec<_>>();
docgen.run(package_path.display().to_string(), dep_paths, model)?
}
// If enabled generate docs.
if options.with_docs {
let docgen = if let Some(opts) = options.docgen_options.clone() {
opts
} else {
DocgenOptions::default()
};
let dep_paths = package
.deps_compiled_units
.iter()
.map(|(_, u)| {
u.source_path
.parent()
.unwrap()
.parent()
.unwrap()
.join(get_docgen_output_dir())
.display()
.to_string()
})
.unique()
.collect::<Vec<_>>();
docgen.run(package_path.display().to_string(), dep_paths, model)?
}

Ok(Self {
options,
package_path,
package,
})
Ok(Self {
options,
package_path,
package,
})
}
}

// Check versions and warn user if using unstable ones.
Expand Down
1 change: 1 addition & 0 deletions crates/aptos/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ move-core-types = { workspace = true }
move-coverage = { workspace = true }
move-disassembler = { workspace = true }
move-ir-types = { workspace = true }
move-linter = { workspace = true }
move-model = { workspace = true }
move-package = { workspace = true }
move-prover-boogie-backend = { workspace = true }
Expand Down
7 changes: 5 additions & 2 deletions crates/aptos/src/move_tool/lint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use aptos_framework::{BuildOptions, BuiltPackage};
use async_trait::async_trait;
use clap::Parser;
use move_compiler_v2::Experiment;
use move_linter::MoveLintChecks;
use move_model::metadata::{CompilerVersion, LanguageVersion};
use move_package::source_package::std_lib::StdVersion;
use std::{collections::BTreeMap, path::PathBuf};
Expand Down Expand Up @@ -112,7 +113,7 @@ impl CliCommand<&'static str> for LintPackage {

async fn execute(self) -> CliTypedResult<&'static str> {
let move_options = MovePackageDir {
compiler_version: Some(CompilerVersion::V2_0),
compiler_version: Some(CompilerVersion::latest_stable()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, but we maybe should have a separate PR to fix these.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is always good to fix code on the fly, specifically such trivialities.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because the problem may be more widespread. Unless you have time to go fix all the use cases, you've just swept one case under the rug. If you do this, do a thorough search or file an issue to follow up and do it.

You also will have a commit message that says this line was changed to "Externalize the linter architecture". Is that useful for future devs? No.

SWE is communication, not just twiddling dials to make it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this PR, keeping this change.

Will follow up with another PR that does a more wide-spread fix across the code base.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I filed an issues #15178 to track this.

..self.to_move_options()
};
let more_experiments = vec![
Expand All @@ -131,7 +132,9 @@ impl CliCommand<&'static str> for LintPackage {
true,
)?
};
BuiltPackage::build(package_path, build_options)?;
BuiltPackage::build_with_external_checks(package_path, build_options, vec![
MoveLintChecks::make(),
])?;
Ok("succeeded")
}
}
2 changes: 0 additions & 2 deletions third_party/move/move-compiler-v2/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ num = { workspace = true }
once_cell = { workspace = true }
#paste = "1.0.5"
petgraph = { workspace = true }
strum = { workspace = true }
strum_macros = { workspace = true }

[dev-dependencies]
anyhow = { workspace = true }
Expand Down
Loading
Loading