Skip to content

Commit

Permalink
[move-compiler] Remove WarningFiltersScope from CompilationEnv (#20065)
Browse files Browse the repository at this point in the history
## Description 

- To make the compiler more parallel friendly, the warning filter scope
needs to be local to the environment it is in
- Conceptually, CompilationEnv was always the wrong place for this, and
trying to add parallelism just exposed this
- Added a RwLock around Diagnostics to help with linters. It was a bit
hard to tie the knot without doing this as a part of this PR

## Test plan 

- Ran tests

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
tnowacki authored Oct 29, 2024
1 parent b2700da commit 29cb03e
Show file tree
Hide file tree
Showing 56 changed files with 1,297 additions and 1,203 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ impl TypingAnalysisContext<'_> {

impl<'a> TypingVisitorContext for TypingAnalysisContext<'a> {
// Nothing to do -- we're not producing errors.
fn add_warning_filter_scope(&mut self, _filter: diag::WarningFilters) {}
fn push_warning_filter_scope(&mut self, _filter: diag::WarningFilters) {}

// Nothing to do -- we're not producing errors.
fn pop_warning_filter_scope(&mut self) {}
Expand Down
8 changes: 4 additions & 4 deletions external-crates/move/crates/move-analyzer/src/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1875,7 +1875,7 @@ pub fn get_compiled_pkg(
eprintln!("compiled to parsed AST");
let (compiler, parsed_program) = compiler.into_ast();
parsed_ast = Some(parsed_program.clone());
mapped_files.extend_with_duplicates(compiler.compilation_env_ref().mapped_files().clone());
mapped_files.extend_with_duplicates(compiler.compilation_env().mapped_files().clone());

// extract typed AST
let compilation_result = compiler.at_parser(parsed_program).run::<PASS_TYPING>();
Expand All @@ -1890,17 +1890,17 @@ pub fn get_compiled_pkg(
}
};
eprintln!("compiled to typed AST");
let (mut compiler, typed_program) = compiler.into_ast();
let (compiler, typed_program) = compiler.into_ast();
typed_ast = Some(typed_program.clone());
compiler_info = Some(CompilerInfo::from(
compiler.compilation_env().ide_information.clone(),
compiler.compilation_env().ide_information().clone(),
));
edition = Some(compiler.compilation_env().edition(Some(root_pkg_name)));

// compile to CFGIR for accurate diags
eprintln!("compiling to CFGIR");
let compilation_result = compiler.at_typing(typed_program).run::<PASS_CFGIR>();
let mut compiler = match compilation_result {
let compiler = match compilation_result {
Ok(v) => v,
Err((_pass, diags)) => {
let failure = false;
Expand Down
2 changes: 1 addition & 1 deletion external-crates/move/crates/move-cli/src/base/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ pub fn run_move_unit_tests<W: Write + Send>(
let (files, comments_and_compiler_res) = compiler.run::<PASS_CFGIR>().unwrap();
let (_, compiler) =
diagnostics::unwrap_or_report_pass_diagnostics(&files, comments_and_compiler_res);
let (mut compiler, cfgir) = compiler.into_ast();
let (compiler, cfgir) = compiler.into_ast();
let compilation_env = compiler.compilation_env();
let built_test_plan = construct_test_plan(compilation_env, Some(root_package), &cfgir);
let mapped_files = compilation_env.mapped_files().clone();
Expand Down
17 changes: 6 additions & 11 deletions external-crates/move/crates/move-compiler/src/cfgir/borrows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::{
translate::{display_var, DisplayVar},
},
parser::ast::BinOp_,
shared::{unique_map::UniqueMap, CompilationEnv},
shared::unique_map::UniqueMap,
};
use move_proc_macros::growing_stack;

Expand Down Expand Up @@ -90,7 +90,6 @@ impl TransferFunctions for BorrowSafety {
impl AbstractInterpreter for BorrowSafety {}

pub fn verify(
compilation_env: &mut CompilationEnv,
context: &super::CFGContext,
cfg: &super::cfg::MutForwardCFG,
) -> BTreeMap<Label, BorrowState> {
Expand All @@ -100,21 +99,17 @@ pub fn verify(
let mut safety = BorrowSafety::new(locals);

// check for existing errors
let has_errors = compilation_env.has_errors();
let has_errors = context.env.has_errors();
let mut initial_state = BorrowState::initial(locals, safety.mutably_used.clone(), has_errors);
initial_state.bind_arguments(&signature.parameters);
initial_state.canonicalize_locals(&safety.local_numbers);
let (final_state, ds) = safety.analyze_function(cfg, initial_state);
compilation_env.add_diags(ds);
unused_mut_borrows(compilation_env, context, safety.mutably_used);
context.add_diags(ds);
unused_mut_borrows(context, safety.mutably_used);
final_state
}

fn unused_mut_borrows(
compilation_env: &mut CompilationEnv,
context: &super::CFGContext,
mutably_used: RefExpInfoMap,
) {
fn unused_mut_borrows(context: &super::CFGContext, mutably_used: RefExpInfoMap) {
const MSG: &str = "Mutable reference is never used mutably, \
consider switching to an immutable reference '&' instead";

Expand Down Expand Up @@ -143,7 +138,7 @@ fn unused_mut_borrows(
} else {
diag!(UnusedItem::MutReference, (*loc, MSG))
};
compilation_env.add_diag(diag)
context.add_diag(diag)
}
}
}
Expand Down
28 changes: 10 additions & 18 deletions external-crates/move/crates/move-compiler/src/cfgir/liveness/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::{
diagnostics::Diagnostics,
expansion::ast::Mutability,
hlir::ast::{self as H, *},
shared::{unique_map::UniqueMap, CompilationEnv},
shared::unique_map::UniqueMap,
};
use move_ir_types::location::*;
use move_proc_macros::growing_stack;
Expand Down Expand Up @@ -168,11 +168,7 @@ fn exp(state: &mut LivenessState, parent_e: &Exp) {
/// - Reports an error if an assignment/let was not used
/// Switches it to an `Ignore` if it has the drop ability (helps with error messages for borrows)
pub fn last_usage(
compilation_env: &mut CompilationEnv,
context: &super::CFGContext,
cfg: &mut MutForwardCFG,
) {
pub fn last_usage(context: &super::CFGContext, cfg: &mut MutForwardCFG) {
let super::CFGContext {
infinite_loop_starts,
..
Expand All @@ -183,46 +179,45 @@ pub fn last_usage(
.get(lbl)
.unwrap_or_else(|| panic!("ICE no liveness states for {}", lbl));
let command_states = per_command_states.get(lbl).unwrap();
last_usage::block(compilation_env, final_invariant, command_states, block)
last_usage::block(context, final_invariant, command_states, block)
}
}

mod last_usage {
use move_proc_macros::growing_stack;

use crate::{
cfgir::liveness::state::LivenessState,
cfgir::{liveness::state::LivenessState, CFGContext},
diag,
hlir::{
ast::*,
translate::{display_var, DisplayVar},
},
shared::*,
};
use std::collections::{BTreeSet, VecDeque};

struct Context<'a, 'b> {
env: &'a mut CompilationEnv,
outer: &'a CFGContext<'a>,
next_live: &'b BTreeSet<Var>,
dropped_live: BTreeSet<Var>,
}

impl<'a, 'b> Context<'a, 'b> {
fn new(
env: &'a mut CompilationEnv,
outer: &'a CFGContext<'a>,
next_live: &'b BTreeSet<Var>,
dropped_live: BTreeSet<Var>,
) -> Self {
Context {
env,
outer,
next_live,
dropped_live,
}
}
}

pub fn block(
compilation_env: &mut CompilationEnv,
context: &CFGContext,
final_invariant: &LivenessState,
command_states: &VecDeque<LivenessState>,
block: &mut BasicBlock,
Expand All @@ -245,10 +240,7 @@ mod last_usage {
.difference(next_data)
.cloned()
.collect::<BTreeSet<_>>();
command(
&mut Context::new(compilation_env, next_data, dropped_live),
cmd,
)
command(&mut Context::new(context, next_data, dropped_live), cmd)
}
}

Expand Down Expand Up @@ -300,7 +292,7 @@ mod last_usage {
'_{vstr}')",
);
context
.env
.outer
.add_diag(diag!(UnusedItem::Assignment, (l.loc, msg)));
}
*unused_assignment = true;
Expand Down
62 changes: 20 additions & 42 deletions external-crates/move/crates/move-compiler/src/cfgir/locals/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub mod state;

use super::absint::*;
use crate::{
cfgir::CFGContext,
diag,
diagnostics::{Diagnostic, Diagnostics},
editions::Edition,
Expand All @@ -16,15 +17,10 @@ use crate::{
},
naming::ast::{self as N, TParam},
parser::ast::{Ability_, DatatypeName},
shared::{
program_info::{DatatypeKind, TypingProgramInfo},
unique_map::UniqueMap,
*,
},
shared::{program_info::DatatypeKind, unique_map::UniqueMap},
};
use move_ir_types::location::*;
use move_proc_macros::growing_stack;
use move_symbol_pool::Symbol;
use state::*;
use std::collections::BTreeMap;

Expand All @@ -33,19 +29,15 @@ use std::collections::BTreeMap;
//**************************************************************************************************

struct LocalsSafety<'a> {
env: &'a CompilationEnv,
info: &'a TypingProgramInfo,
package: Option<Symbol>,
context: &'a CFGContext<'a>,
local_types: &'a UniqueMap<Var, (Mutability, SingleType)>,
signature: &'a FunctionSignature,
unused_mut: BTreeMap<Var, Loc>,
}

impl<'a> LocalsSafety<'a> {
fn new(
env: &'a CompilationEnv,
info: &'a TypingProgramInfo,
package: Option<Symbol>,
context: &'a CFGContext<'a>,
local_types: &'a UniqueMap<Var, (Mutability, SingleType)>,
signature: &'a FunctionSignature,
) -> Self {
Expand All @@ -60,9 +52,7 @@ impl<'a> LocalsSafety<'a> {
})
.collect();
Self {
env,
info,
package,
context,
local_types,
signature,
unused_mut,
Expand All @@ -71,9 +61,7 @@ impl<'a> LocalsSafety<'a> {
}

struct Context<'a, 'b> {
env: &'a CompilationEnv,
info: &'a TypingProgramInfo,
package: Option<Symbol>,
outer: &'a CFGContext<'a>,
local_types: &'a UniqueMap<Var, (Mutability, SingleType)>,
unused_mut: &'a mut BTreeMap<Var, Loc>,
local_states: &'b mut LocalStates,
Expand All @@ -83,15 +71,12 @@ struct Context<'a, 'b> {

impl<'a, 'b> Context<'a, 'b> {
fn new(locals_safety: &'a mut LocalsSafety, local_states: &'b mut LocalStates) -> Self {
let env = locals_safety.env;
let info = locals_safety.info;
let outer = locals_safety.context;
let local_types = locals_safety.local_types;
let signature = locals_safety.signature;
let unused_mut = &mut locals_safety.unused_mut;
Self {
env,
info,
package: locals_safety.package,
outer,
local_types,
unused_mut,
local_states,
Expand Down Expand Up @@ -154,18 +139,18 @@ impl<'a, 'b> Context<'a, 'b> {
// .unwrap();

fn datatype_decl_loc(&self, m: &ModuleIdent, n: &DatatypeName) -> Loc {
let kind = self.info.datatype_kind(m, n);
let kind = self.outer.info.datatype_kind(m, n);
match kind {
DatatypeKind::Struct => self.info.struct_declared_loc(m, n),
DatatypeKind::Enum => self.info.enum_declared_loc(m, n),
DatatypeKind::Struct => self.outer.info.struct_declared_loc(m, n),
DatatypeKind::Enum => self.outer.info.enum_declared_loc(m, n),
}
}

fn datatype_declared_abilities(&self, m: &ModuleIdent, n: &DatatypeName) -> &'a AbilitySet {
let kind = self.info.datatype_kind(m, n);
let kind = self.outer.info.datatype_kind(m, n);
match kind {
DatatypeKind::Struct => self.info.struct_declared_abilities(m, n),
DatatypeKind::Enum => self.info.enum_declared_abilities(m, n),
DatatypeKind::Struct => self.outer.info.struct_declared_abilities(m, n),
DatatypeKind::Enum => self.outer.info.enum_declared_abilities(m, n),
}
}
}
Expand All @@ -189,30 +174,23 @@ impl<'a> TransferFunctions for LocalsSafety<'a> {
impl<'a> AbstractInterpreter for LocalsSafety<'a> {}

pub fn verify(
compilation_env: &mut CompilationEnv,
context: &super::CFGContext,
cfg: &super::cfg::MutForwardCFG,
) -> BTreeMap<Label, LocalStates> {
let super::CFGContext {
signature, locals, ..
} = context;
let initial_state = LocalStates::initial(&signature.parameters, locals);
let mut locals_safety = LocalsSafety::new(
compilation_env,
context.info,
context.package,
locals,
signature,
);
let mut locals_safety = LocalsSafety::new(context, locals, signature);
let (final_state, ds) = locals_safety.analyze_function(cfg, initial_state);
unused_let_muts(compilation_env, locals, locals_safety.unused_mut);
compilation_env.add_diags(ds);
unused_let_muts(context, locals, locals_safety.unused_mut);
context.add_diags(ds);
final_state
}

/// Generates warnings for unused mut declarations
fn unused_let_muts<T>(
env: &mut CompilationEnv,
context: &CFGContext,
locals: &UniqueMap<Var, T>,
unused_mut_locals: BTreeMap<Var, Loc>,
) {
Expand All @@ -226,7 +204,7 @@ fn unused_let_muts<T>(
let decl_loc = *locals.get_loc(&v).unwrap();
let decl_msg = format!("The variable '{vstr}' is never used mutably");
let mut_msg = "Consider removing the 'mut' declaration here";
env.add_diag(diag!(
context.add_diag(diag!(
UnusedItem::MutModifier,
(decl_loc, decl_msg),
(mut_loc, mut_msg)
Expand Down Expand Up @@ -524,7 +502,7 @@ fn check_mutability(
let usage_msg = format!("Invalid {usage} of immutable variable '{vstr}'");
let decl_msg =
format!("To use the variable mutably, it must be declared 'mut', e.g. 'mut {vstr}'");
if context.env.edition(context.package) == Edition::E2024_MIGRATION {
if context.outer.env.edition(context.outer.package) == Edition::E2024_MIGRATION {
context.add_diag(diag!(Migration::NeedsLetMut, (decl_loc, decl_msg.clone())))
} else {
let mut diag = diag!(
Expand Down
Loading

0 comments on commit 29cb03e

Please sign in to comment.