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.

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

[CLI] Add message-format-json experiment value to aptos move compile and aptos move lint #15540

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions Cargo.lock

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

6 changes: 6 additions & 0 deletions crates/aptos/src/move_tool/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ pub struct LintPackage {
/// See <https://github.com/aptos-labs/aptos-core/issues/10335>
#[clap(long, env = "APTOS_CHECK_TEST_CODE")]
pub check_test_code: bool,

/// Experiments
#[clap(long, hide(true))]
pub experiments: Vec<String>,
}

impl LintPackage {
Expand All @@ -89,6 +93,7 @@ impl LintPackage {
language_version,
skip_attribute_checks,
check_test_code,
experiments,
} = self.clone();
MovePackageDir {
dev,
Expand All @@ -100,6 +105,7 @@ impl LintPackage {
language_version,
skip_attribute_checks,
check_test_code,
experiments,
..MovePackageDir::new()
}
}
Expand Down
3 changes: 3 additions & 0 deletions third_party/move/move-compiler-v2/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ abstract-domain-derive = { path = "../move-model/bytecode/abstract_domain_derive
anyhow = { workspace = true }
bcs = { workspace = true }
clap = { workspace = true, features = ["derive", "env"] }
codespan = { workspace = true }
codespan-reporting = { workspace = true, features = ["serde", "serialization"] }
ethnum = { workspace = true }
flexi_logger = { workspace = true }
Expand All @@ -35,6 +36,8 @@ move-symbol-pool = { workspace = true }
num = { workspace = true }
once_cell = { workspace = true }
petgraph = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true }

[dev-dependencies]
anyhow = { workspace = true }
Expand Down
28 changes: 28 additions & 0 deletions third_party/move/move-compiler-v2/src/diagnostics/human.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
use crate::diagnostics::Emitter;
use codespan::{FileId, Files};
use codespan_reporting::{
diagnostic::Diagnostic,
term::{emit, termcolor::WriteColor, Config},
};

pub struct HumanEmitter<'w, W: WriteColor> {
writer: &'w mut W,
}

impl<'w, W> HumanEmitter<'w, W>
where
W: WriteColor,
{
pub fn new(writer: &'w mut W) -> Self {
HumanEmitter { writer }
}
}

impl<'w, W> Emitter for HumanEmitter<'w, W>
where
W: WriteColor,
{
fn emit(&mut self, source_files: &Files<String>, diag: &Diagnostic<FileId>) {
emit(&mut self.writer, &Config::default(), source_files, diag).expect("emit must not fail")
}
}
21 changes: 21 additions & 0 deletions third_party/move/move-compiler-v2/src/diagnostics/json.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
use crate::diagnostics::Emitter;
use codespan::{FileId, Files};
use codespan_reporting::diagnostic::Diagnostic;
use std::io::Write;

pub struct JsonEmitter<'w, W: Write> {
writer: &'w mut W,
}

impl<'w, W: Write> JsonEmitter<'w, W> {
pub fn new(writer: &'w mut W) -> Self {
JsonEmitter { writer }
}
}

impl<'w, W: Write> Emitter for JsonEmitter<'w, W> {
fn emit(&mut self, _source_files: &Files<String>, diag: &Diagnostic<FileId>) {
serde_json::to_writer(&mut self.writer, diag).expect("emit must not fail");
writeln!(&mut self.writer).unwrap();
}
}
52 changes: 52 additions & 0 deletions third_party/move/move-compiler-v2/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use crate::{
diagnostics::{human::HumanEmitter, json::JsonEmitter},
options, Experiment,
};
use anyhow::bail;
use codespan::{FileId, Files};
use codespan_reporting::{
diagnostic::{Diagnostic, Severity},
term::termcolor::StandardStream,
};
use move_model::model::GlobalEnv;

pub mod human;
pub mod json;

impl options::Options {
pub fn to_emitter<'w>(&self, stderr: &'w mut StandardStream) -> Box<dyn Emitter + 'w> {
if self.experiment_on(Experiment::MESSAGE_FORMAT_JSON) {
Box::new(JsonEmitter::new(stderr))
} else {
Box::new(HumanEmitter::new(stderr))
}
}
}

pub trait Emitter {
fn emit(&mut self, source_files: &Files<String>, diag: &Diagnostic<FileId>);

/// Writes accumulated diagnostics of given or higher severity.
fn report_diag(&mut self, global_env: &GlobalEnv, severity: Severity) {
global_env.report_diag_with_filter(
|files, diag| self.emit(files, diag),
|d| d.severity >= severity,
);
}

/// Helper function to report diagnostics, check for errors, and fail with a message on
/// errors. This function is idempotent and will not report the same diagnostics again.
fn check_diag(
&mut self,
global_env: &GlobalEnv,
report_severity: Severity,
msg: &str,
) -> anyhow::Result<()> {
self.report_diag(global_env, report_severity);
if global_env.has_errors() {
bail!("exiting with {}", msg);
} else {
Ok(())
}
}
}
6 changes: 6 additions & 0 deletions third_party/move/move-compiler-v2/src/experiments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,11 @@ pub static EXPERIMENTS: Lazy<BTreeMap<String, Experiment>> = Lazy::new(|| {
.to_string(),
default: Given(false),
},
Experiment {
name: Experiment::MESSAGE_FORMAT_JSON.to_string(),
description: "Enable json format for compiler messages".to_string(),
default: Given(false),
},
];
experiments
.into_iter()
Expand Down Expand Up @@ -302,6 +307,7 @@ impl Experiment {
pub const LAMBDA_LIFTING: &'static str = "lambda-lifting";
pub const LAMBDA_VALUES: &'static str = "lambda-values";
pub const LINT_CHECKS: &'static str = "lint-checks";
pub const MESSAGE_FORMAT_JSON: &'static str = "message-format-json";
pub const OPTIMIZE: &'static str = "optimize";
pub const OPTIMIZE_EXTRA: &'static str = "optimize-extra";
pub const OPTIMIZE_WAITING_FOR_COMPARE_TESTS: &'static str =
Expand Down
39 changes: 28 additions & 11 deletions third_party/move/move-compiler-v2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// SPDX-License-Identifier: Apache-2.0

mod bytecode_generator;
pub mod diagnostics;
pub mod env_pipeline;
mod experiments;
pub mod external_checks;
Expand All @@ -14,6 +15,7 @@ pub mod pipeline;
pub mod plan_builder;

use crate::{
diagnostics::{human::HumanEmitter, Emitter},
env_pipeline::{
acquires_checker, ast_simplifier, cyclic_instantiation_checker, flow_insensitive_checkers,
function_checker, inliner, lambda_lifter, lambda_lifter::LambdaLiftingOptions,
Expand Down Expand Up @@ -77,8 +79,9 @@ use std::{collections::BTreeSet, io::Write, path::Path};
pub fn run_move_compiler_to_stderr(
options: Options,
) -> anyhow::Result<(GlobalEnv, Vec<AnnotatedCompiledUnit>)> {
let mut error_writer = StandardStream::stderr(ColorChoice::Auto);
run_move_compiler(&mut error_writer, options)
let mut stderr = StandardStream::stderr(ColorChoice::Auto);
let mut emitter = HumanEmitter::new(&mut stderr);
run_move_compiler_with_emitter(&mut emitter, options)
}

/// Run move compiler and print errors to given writer. Returns the set of compiled units.
Expand All @@ -88,21 +91,34 @@ pub fn run_move_compiler<W>(
) -> anyhow::Result<(GlobalEnv, Vec<AnnotatedCompiledUnit>)>
where
W: WriteColor + Write,
{
let mut emitter = HumanEmitter::new(error_writer);
run_move_compiler_with_emitter(&mut emitter, options)
}

/// Run move compiler and print errors to given writer. Returns the set of compiled units.
pub fn run_move_compiler_with_emitter<E>(
emitter: &mut E,
options: Options,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the code duplication? Could we not create the emitter based on the message format in options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, it's a draft, I'll remove it when someone confirms that the Emitter trait approach works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

) -> anyhow::Result<(GlobalEnv, Vec<AnnotatedCompiledUnit>)>
where
E: Emitter + ?Sized,
{
logging::setup_logging();
info!("Move Compiler v2");

// Run context check.
let mut env = run_checker_and_rewriters(options.clone())?;
check_errors(&env, error_writer, "checking errors")?;
check_errors(&env, emitter, "checking errors")?;

if options.experiment_on(Experiment::STOP_BEFORE_STACKLESS_BYTECODE) {
std::process::exit(0)
}

// Run code generator
let mut targets = run_bytecode_gen(&env);
check_errors(&env, error_writer, "code generation errors")?;
check_errors(&env, emitter, "code generation errors")?;

debug!("After bytecode_gen, GlobalEnv={}", env.dump_env());

// Run transformation pipeline
Expand All @@ -129,14 +145,14 @@ where
} else {
pipeline.run_with_hook(&env, &mut targets, |_| {}, |_, _, _| !env.has_errors())
}
check_errors(&env, error_writer, "stackless-bytecode analysis errors")?;
check_errors(&env, emitter, "stackless-bytecode analysis errors")?;

if options.experiment_on(Experiment::STOP_BEFORE_FILE_FORMAT) {
std::process::exit(0)
}

let modules_and_scripts = run_file_format_gen(&mut env, &targets);
check_errors(&env, error_writer, "assembling errors")?;
check_errors(&env, emitter, "assembling errors")?;

debug!(
"File format bytecode:\n{}",
Expand All @@ -145,7 +161,7 @@ where

let annotated_units = annotate_units(modules_and_scripts);
run_bytecode_verifier(&annotated_units, &mut env);
check_errors(&env, error_writer, "bytecode verification errors")?;
check_errors(&env, emitter, "bytecode verification errors")?;

// Finally mark this model to be generated by v2
env.set_compiler_v2(true);
Expand Down Expand Up @@ -605,13 +621,14 @@ fn get_vm_error_loc(env: &GlobalEnv, source_map: &SourceMap, e: &VMError) -> Opt
}

/// Report any diags in the env to the writer and fail if there are errors.
pub fn check_errors<W>(env: &GlobalEnv, error_writer: &mut W, msg: &str) -> anyhow::Result<()>
pub fn check_errors<E>(env: &GlobalEnv, emitter: &mut E, msg: &str) -> anyhow::Result<()>
where
W: WriteColor + Write,
E: Emitter + ?Sized,
{
let options = env.get_extension::<Options>().unwrap_or_default();
env.report_diag(error_writer, options.report_severity());
env.check_diag(error_writer, options.report_severity(), msg)

emitter.report_diag(env, options.report_severity());
emitter.check_diag(env, options.report_severity(), msg)
}

/// Annotate the given compiled units.
Expand Down
20 changes: 12 additions & 8 deletions third_party/move/move-model/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1213,7 +1213,12 @@ impl GlobalEnv {

/// Writes accumulated diagnostics of given or higher severity.
pub fn report_diag<W: WriteColor>(&self, writer: &mut W, severity: Severity) {
self.report_diag_with_filter(writer, |d| d.severity >= severity)
self.report_diag_with_filter(
|files, diag| {
emit(writer, &Config::default(), files, diag).expect("emit must not fail")
},
|d| d.severity >= severity,
);
}

/// Helper function to report diagnostics, check for errors, and fail with a message on
Expand Down Expand Up @@ -1312,11 +1317,11 @@ impl GlobalEnv {
}

/// Writes accumulated diagnostics that pass through `filter`
pub fn report_diag_with_filter<W: WriteColor, F: FnMut(&Diagnostic<FileId>) -> bool>(
&self,
writer: &mut W,
mut filter: F,
) {
pub fn report_diag_with_filter<E, F>(&self, mut emitter: E, mut filter: F)
where
E: FnMut(&Files<String>, &Diagnostic<FileId>),
F: FnMut(&Diagnostic<FileId>) -> bool,
{
let mut shown = BTreeSet::new();
self.diags.borrow_mut().sort_by(|a, b| {
let reported_ordering = a.1.cmp(&b.1);
Expand All @@ -1338,8 +1343,7 @@ impl GlobalEnv {
// Avoid showing the same message twice. This can happen e.g. because of
// duplication of expressions via schema inclusion.
if shown.insert(format!("{:?}", diag)) {
emit(writer, &Config::default(), &self.source_files, diag)
.expect("emit must not fail");
emitter(&self.source_files, diag);
}
*reported = true;
}
Expand Down
7 changes: 5 additions & 2 deletions third_party/move/tools/move-cli/src/base/test_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
//! This module manages validation of the unit tests, in addition to standard compiler
//! checking.

use codespan_reporting::term::{termcolor, termcolor::StandardStream};
use codespan_reporting::term::{emit, termcolor, termcolor::StandardStream, Config};
use move_model::model::GlobalEnv;
use once_cell::sync::Lazy;
use std::sync::Mutex;
Expand Down Expand Up @@ -35,8 +35,11 @@ pub(crate) fn validate(env: &GlobalEnv) {

pub fn has_errors_then_report(model: &GlobalEnv) -> bool {
let mut has_errors = false;
let mut writer = StandardStream::stderr(termcolor::ColorChoice::Auto);
model.report_diag_with_filter(
&mut StandardStream::stderr(termcolor::ColorChoice::Auto),
|files, diag| {
emit(&mut writer, &Config::default(), files, diag).expect("emit must not fail")
},
|d| {
let include = d.labels.iter().all(|l| {
let fname = model.get_file(l.file_id).to_string_lossy();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1155,8 +1155,9 @@ pub fn build_and_report_v2_driver(options: move_compiler_v2::Options) -> Compile
pub fn build_and_report_no_exit_v2_driver(
options: move_compiler_v2::Options,
) -> CompilerDriverResult {
let mut writer = StandardStream::stderr(ColorChoice::Auto);
let (env, units) = move_compiler_v2::run_move_compiler(&mut writer, options)?;
let mut stderr = StandardStream::stderr(ColorChoice::Auto);
let mut emitter = options.to_emitter(&mut stderr);
let (env, units) = move_compiler_v2::run_move_compiler_with_emitter(emitter.as_mut(), options)?;
Ok((
move_compiler_v2::make_files_source_text(&env),
units,
Expand Down
Loading