Skip to content

Commit

Permalink
[move-compiler] Added basic stats about suppressed linters (#13800)
Browse files Browse the repository at this point in the history
## Description 

This PR adds basic stats about suppressed linters in user code.

## Test Plan 

All existing tests must pass plus manually tested that the correct stats
get printed.

---
If your changes are not user-facing and not a breaking change, you can
skip the following section. Otherwise, please indicate what changed, and
then add to the Release Notes section as highlighted during the release
process.

### Type of Change (Check all that apply)

- [ ] protocol change
- [x] user-visible impact
- [ ] breaking change for a client SDKs
- [ ] breaking change for FNs (FN binary must upgrade)
- [ ] breaking change for validators or node operators (must upgrade
binaries)
- [ ] breaking change for on-chain data layout
- [ ] necessitate either a data wipe or data migration

### Release notes

When linter warnings in user code are suppressed, basic statistics about
suppressed warnings (number of suppresed warnings and number of
suppressed warning categories) will be printed.
  • Loading branch information
awelc committed Sep 21, 2023
1 parent 0bbd3b0 commit 7d8f830
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 21 deletions.
74 changes: 61 additions & 13 deletions move-compiler/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,17 @@ pub struct Diagnostic {
#[derive(PartialEq, Eq, Hash, Clone, Debug, Default)]
pub struct Diagnostics {
diagnostics: Vec<Diagnostic>,
// diagnostics filtered in source code
filtered_source_diagnostics: Vec<Diagnostic>,
severity_count: BTreeMap<Severity, usize>,
}

#[derive(PartialEq, Eq, Clone, Debug)]
/// Used to filter out diagnostics, specifically used for warning suppression
pub struct WarningFilters(BTreeMap<ExternalPrefix, UnprefixedWarningFilters>);
pub struct WarningFilters {
filters: BTreeMap<ExternalPrefix, UnprefixedWarningFilters>,
for_dependency: bool, // if false, the filters are used for source code
}

#[derive(PartialEq, Eq, Clone, Debug)]
/// Filters split by category and code
Expand Down Expand Up @@ -214,6 +219,7 @@ impl Diagnostics {
pub fn new() -> Self {
Self {
diagnostics: vec![],
filtered_source_diagnostics: vec![],
severity_count: BTreeMap::new(),
}
}
Expand Down Expand Up @@ -245,9 +251,14 @@ impl Diagnostics {
}
}

pub fn add_source_filtered(&mut self, diag: Diagnostic) {
self.filtered_source_diagnostics.push(diag)
}

pub fn extend(&mut self, other: Self) {
let Self {
diagnostics,
filtered_source_diagnostics: _,
severity_count,
} = other;
for (sev, count) in severity_count {
Expand Down Expand Up @@ -294,6 +305,21 @@ impl Diagnostics {
.iter()
.any(|d| d.info.external_prefix() == Some(prefix))
}

/// Returns the number of diags filtered in source (user) code (an not in the dependencies) that
/// have a given prefix (first value returned) and how many different categories of diags were
/// filtered.
pub fn filtered_source_diags_with_prefix(&self, prefix: &str) -> (usize, usize) {
let mut filtered_diags_num = 0;
let mut filtered_categories = HashSet::new();
self.filtered_source_diagnostics.iter().for_each(|d| {
if d.info.external_prefix() == Some(prefix) {
filtered_diags_num += 1;
filtered_categories.insert(d.info.category());
}
});
(filtered_diags_num, filtered_categories.len())
}
}

impl Diagnostic {
Expand Down Expand Up @@ -379,8 +405,18 @@ macro_rules! diag {
}

impl WarningFilters {
pub fn new() -> Self {
Self(BTreeMap::new())
pub fn new_for_source() -> Self {
Self {
filters: BTreeMap::new(),
for_dependency: false,
}
}

pub fn new_for_dependency() -> Self {
Self {
filters: BTreeMap::new(),
for_dependency: true,
}
}

pub fn is_filtered(&self, diag: &Diagnostic) -> bool {
Expand All @@ -389,24 +425,28 @@ impl WarningFilters {

fn is_filtered_by_info(&self, info: &DiagnosticInfo) -> bool {
let prefix = info.external_prefix();
self.0
self.filters
.get(&prefix)
.is_some_and(|filters| filters.is_filtered_by_info(info))
}

pub fn union(&mut self, other: &Self) {
for (prefix, filters) in &other.0 {
self.0
for (prefix, filters) in &other.filters {
self.filters
.entry(*prefix)
.or_insert_with(UnprefixedWarningFilters::new)
.union(filters);
}
// if there is a dependency code filter on the stack, it means we are filtering dependent
// code and this information must be preserved when stacking up additional filters (which
// involves union of the current filter with the new one)
self.for_dependency = self.for_dependency || other.for_dependency;
}

pub fn add(&mut self, filter: WarningFilter) {
let (prefix, category, code, name) = match filter {
WarningFilter::All(prefix) => {
self.0.insert(prefix, UnprefixedWarningFilters::All);
self.filters.insert(prefix, UnprefixedWarningFilters::All);
return;
}
WarningFilter::Category {
Expand All @@ -421,17 +461,24 @@ impl WarningFilters {
name,
} => (prefix, category, Some(code), name),
};
self.0
self.filters
.entry(prefix)
.or_insert(UnprefixedWarningFilters::Empty)
.add(category, code, name)
}

pub fn unused_warnings_filter_for_test() -> Self {
Self(BTreeMap::from([(
None,
UnprefixedWarningFilters::unused_warnings_filter_for_test(),
)]))
Self {
filters: BTreeMap::from([(
None,
UnprefixedWarningFilters::unused_warnings_filter_for_test(),
)]),
for_dependency: false,
}
}

pub fn for_dependency(&self) -> bool {
self.for_dependency
}
}

Expand Down Expand Up @@ -561,6 +608,7 @@ impl From<Vec<Diagnostic>> for Diagnostics {
}
Self {
diagnostics,
filtered_source_diagnostics: vec![],
severity_count,
}
}
Expand All @@ -574,7 +622,7 @@ impl From<Option<Diagnostic>> for Diagnostics {

impl AstDebug for WarningFilters {
fn ast_debug(&self, w: &mut crate::shared::ast_debug::AstWriter) {
for (prefix, filters) in &self.0 {
for (prefix, filters) in &self.filters {
let prefix_str = prefix.unwrap_or(WARNING_FILTER_ATTR);
match filters {
UnprefixedWarningFilters::All => w.write(&format!(
Expand Down
4 changes: 2 additions & 2 deletions move-compiler/src/expansion/translate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl<'env, 'map> Context<'env, 'map> {
compilation_env: &'env mut CompilationEnv,
module_members: UniqueMap<ModuleIdent, ModuleMembers>,
) -> Self {
let mut all_filter_alls = WarningFilters::new();
let mut all_filter_alls = WarningFilters::new_for_dependency();
for allow in compilation_env.filter_attributes() {
for f in compilation_env.filter_from_str(FILTER_ALL, *allow) {
all_filter_alls.add(f);
Expand Down Expand Up @@ -839,7 +839,7 @@ fn warning_filter(
) -> WarningFilters {
use crate::diagnostics::codes::Category;
use known_attributes::DiagnosticAttribute;
let mut warning_filters = WarningFilters::new();
let mut warning_filters = WarningFilters::new_for_source();
let filter_attribute_names = context.env.filter_attributes().clone();
for allow in filter_attribute_names {
let Some(attr) = attributes.get_(&allow) else {
Expand Down
11 changes: 6 additions & 5 deletions move-compiler/src/shared/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,17 +364,15 @@ impl CompilationEnv {
}

pub fn add_diag(&mut self, mut diag: Diagnostic) {
let is_filtered = self
.warning_filter
.last()
let filter = self.warning_filter.last();
let is_filtered = filter
.map(|filter| filter.is_filtered(&diag))
.unwrap_or(false);
if !is_filtered {
// add help to suppress warning, if applicable
// TODO do we want a centralized place for tips like this?
if diag.info().severity() == Severity::Warning {
if let Some(filter_info) = self.known_filter_names.get(&diag.info().id()) {
// if let Some(filter_attr_name) =
let help = format!(
"This warning can be suppressed with '#[{}({})]' \
applied to the 'module' or module member ('const', 'fun', or 'struct')",
Expand All @@ -385,6 +383,9 @@ impl CompilationEnv {
}
}
self.diags.add(diag)
} else if !filter.unwrap().for_dependency() {
// unwrap above is safe as the filter has been used (thus it must exist)
self.diags.add_source_filtered(diag)
}
}

Expand Down Expand Up @@ -692,7 +693,7 @@ impl Default for PackageConfig {
fn default() -> Self {
Self {
is_dependency: false,
warning_filter: WarningFilters::new(),
warning_filter: WarningFilters::new_for_source(),
flavor: Flavor::default(),
edition: Edition::default(),
}
Expand Down
2 changes: 1 addition & 1 deletion tools/move-package/src/resolution/resolution_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ impl Package {
.edition
.or(config.default_edition)
.unwrap_or_default(),
warning_filter: WarningFilters::new(),
warning_filter: WarningFilters::new_for_source(),
}
}
}
Expand Down

0 comments on commit 7d8f830

Please sign in to comment.