From 6442b7727ce243d66ecdb87f3dfb915b54764eb8 Mon Sep 17 00:00:00 2001 From: Tom Davies Date: Thu, 13 Feb 2025 09:55:29 -0800 Subject: [PATCH] Add diagnostic when calling maps:from_list on a list comprehension Summary: Adds a diagnostic to warn on code of the form: ``` maps:from_list([{K,V} || {K,V} <- L]) ``` and suggest the equivalent map comprehension: ``` #{K => V || {K, V} <- L]} ``` Reviewed By: alanz Differential Revision: D69403570 fbshipit-source-id: 3e6da086efd2ae9dd9df472cabcc72631b75e8e7 --- crates/ide/src/diagnostics.rs | 2 + ...sary_map_from_list_around_comprehension.rs | 204 ++++++++++++++++++ crates/ide_db/src/diagnostic_code.rs | 6 + website/docs/erlang-error-index/w/W0035.md | 4 +- website/docs/erlang-error-index/w/W0036.md | 32 +++ 5 files changed, 246 insertions(+), 2 deletions(-) create mode 100644 crates/ide/src/diagnostics/unnecessary_map_from_list_around_comprehension.rs create mode 100644 website/docs/erlang-error-index/w/W0036.md diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index ea63d0346..650c10d4d 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -110,6 +110,7 @@ mod slow_functions; mod trivial_match; mod undefined_function; mod unnecessary_fold_to_build_map; +mod unnecessary_map_from_list_around_comprehension; mod unnecessary_map_to_list_in_comprehension; mod unused_function_args; mod unused_include; @@ -863,6 +864,7 @@ pub fn diagnostics_descriptors<'a>() -> Vec<&'a DiagnosticDescriptor<'a>> { &inefficient_flatlength::DESCRIPTOR, &inefficient_enumerate::DESCRIPTOR, &map_insertion_to_syntax::DESCRIPTOR, + &unnecessary_map_from_list_around_comprehension::DESCRIPTOR, &unnecessary_map_to_list_in_comprehension::DESCRIPTOR, &unnecessary_fold_to_build_map::DESCRIPTOR, &map_find_to_syntax::DESCRIPTOR, diff --git a/crates/ide/src/diagnostics/unnecessary_map_from_list_around_comprehension.rs b/crates/ide/src/diagnostics/unnecessary_map_from_list_around_comprehension.rs new file mode 100644 index 000000000..0b6ca522b --- /dev/null +++ b/crates/ide/src/diagnostics/unnecessary_map_from_list_around_comprehension.rs @@ -0,0 +1,204 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under both the MIT license found in the + * LICENSE-MIT file in the root directory of this source tree and the Apache + * License, Version 2.0 found in the LICENSE-APACHE file in the root directory + * of this source tree. + */ + +//! Lint: unnecessary_map_from_list_around_comprehension +//! +//! warn on code of the form `maps:from_list([{K,V} || {K,V} <- L])` and suggest `#{K => V || {K, V} <- L]}` + +use elp_ide_db::elp_base_db::FileId; +use elp_ide_db::source_change::SourceChangeBuilder; +use elp_ide_db::DiagnosticCode; +use elp_ide_ssr::match_pattern_in_file_functions; +use elp_ide_ssr::Match; +use hir::fold::MacroStrategy; +use hir::fold::ParenStrategy; +use hir::fold::Strategy; +use hir::Semantic; + +use crate::diagnostics::Category; +use crate::diagnostics::Diagnostic; +use crate::diagnostics::DiagnosticConditions; +use crate::diagnostics::DiagnosticDescriptor; +use crate::diagnostics::Severity; +use crate::fix; + +pub(crate) static DESCRIPTOR: DiagnosticDescriptor = DiagnosticDescriptor { + conditions: DiagnosticConditions { + experimental: false, + include_generated: false, + include_tests: true, + default_disabled: false, + }, + checker: &|acc, sema, file_id, _ext| { + unnecessary_maps_from_list_around_comprehension_ssr(acc, sema, file_id); + }, +}; + +static GENERATOR_VAR: &str = "_@Generator"; +static BINDING_VAR: &str = "_@Binding"; +static KEY_VAR: &str = "_@Key"; +static VALUE_VAR: &str = "_@Value"; + +fn unnecessary_maps_from_list_around_comprehension_ssr( + diags: &mut Vec, + sema: &Semantic, + file_id: FileId, +) { + let matches = match_pattern_in_file_functions( + sema, + Strategy { + macros: MacroStrategy::Expand, + parens: ParenStrategy::InvisibleParens, + }, + file_id, + format!( + "ssr: maps:from_list([{{{KEY_VAR},{VALUE_VAR}}} || {BINDING_VAR} <- {GENERATOR_VAR}])." + ) + .as_str(), + ); + matches.matches.iter().for_each(|m| { + if let Some(diagnostic) = make_diagnostic(sema, m) { + diags.push(diagnostic); + } + }); +} + +fn make_diagnostic(sema: &Semantic, matched: &Match) -> Option { + let file_id = matched.range.file_id; + let inefficient_comprehension_range = matched.range.range; + let generator = matched.placeholder_text(sema, GENERATOR_VAR)?; + let key = matched.placeholder_text(sema, KEY_VAR)?; + let value = matched.placeholder_text(sema, VALUE_VAR)?; + let binding = matched.placeholder_text(sema, BINDING_VAR)?; + let message = "Unnecessary intermediate list allocated.".to_string(); + let mut builder = SourceChangeBuilder::new(file_id); + let direct_comprehension = format!("#{{ {key} => {value} || {binding} <- {generator} }}"); + builder.replace(inefficient_comprehension_range, direct_comprehension); + let fixes = vec![fix( + "unnecessary_map_from_list_around_comprehension", + "Rewrite to construct the map directly from the comprehension", + builder.finish(), + inefficient_comprehension_range, + )]; + Some( + Diagnostic::new( + DiagnosticCode::UnnecessaryMapFromListAroundComprehension, + message, + inefficient_comprehension_range, + ) + .with_severity(Severity::WeakWarning) + .with_ignore_fix(sema, file_id) + .with_fixes(Some(fixes)) + .add_categories([Category::SimplificationRule]), + ) +} + +#[cfg(test)] +mod tests { + + use expect_test::expect; + use expect_test::Expect; + + use crate::diagnostics::Diagnostic; + use crate::diagnostics::DiagnosticCode; + use crate::tests; + + fn filter(d: &Diagnostic) -> bool { + d.code == DiagnosticCode::UnnecessaryMapFromListAroundComprehension + } + + #[track_caller] + fn check_diagnostics(fixture: &str) { + tests::check_filtered_diagnostics(fixture, &filter) + } + + #[track_caller] + fn check_fix(fixture_before: &str, fixture_after: Expect) { + tests::check_fix(fixture_before, fixture_after) + } + + #[test] + fn ignores_when_element_is_not_syntactically_a_pair() { + check_diagnostics( + r#" + //- /src/unnecessary_maps_from_list_around_comprehension.erl + -module(unnecessary_maps_from_list_around_comprehension). + + % elp:ignore W0017 (undefined_function) + fn(List) -> maps:from_list([Pair || Pair <- List]). + "#, + ) + } + + #[test] + fn detects_unnecessary_maps_from_list_around_comprehension_when_deconstructing_list_elem() { + check_diagnostics( + r#" + //- /src/unnecessary_maps_from_list_around_comprehension.erl + -module(unnecessary_maps_from_list_around_comprehension). + + % elp:ignore W0017 (undefined_function) + fn(List) -> maps:from_list([{K + 1, V + 2} || {K,V} <- List]). + %% ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 weak: Unnecessary intermediate list allocated. + "#, + ) + } + + #[test] + fn detects_unnecessary_maps_from_list_around_comprehension_when_binding_whole_list_elem() { + check_diagnostics( + r#" + //- /src/unnecessary_maps_from_list_around_comprehension.erl + -module(unnecessary_maps_from_list_around_comprehension). + + % elp:ignore W0017 (undefined_function) + fn(List) -> maps:from_list([{element(1, Pair) + 1, element(2, Pair) + 2} || Pair <- List]). + %% ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 weak: Unnecessary intermediate list allocated. + "#, + ) + } + + #[test] + fn fixes_unnecessary_maps_from_list_around_comprehension_when_deconstructing_list_elem() { + check_fix( + r#" + //- /src/unnecessary_maps_from_list_around_comprehension.erl + -module(unnecessary_maps_from_list_around_comprehension). + + % elp:ignore W0017 (undefined_function) + fn(List) -> maps:from_l~ist([{K + 1, V + 2} || {K,V} <- List]). + "#, + expect![[r#" + -module(unnecessary_maps_from_list_around_comprehension). + + % elp:ignore W0017 (undefined_function) + fn(List) -> #{ K + 1 => V + 2 || {K,V} <- List }. + "#]], + ) + } + + #[test] + fn fixes_unnecessary_maps_from_list_around_comprehension_when_binding_whole_list_elem() { + check_fix( + r#" + //- /src/unnecessary_maps_from_list_around_comprehension.erl + -module(unnecessary_maps_from_list_around_comprehension). + + % elp:ignore W0017 (undefined_function) + fn(List) -> maps:from_l~ist([{element(1, Pair) + 1, element(2, Pair) + 2} || Pair <- List]). + "#, + expect![[r#" + -module(unnecessary_maps_from_list_around_comprehension). + + % elp:ignore W0017 (undefined_function) + fn(List) -> #{ element(1, Pair) + 1 => element(2, Pair) + 2 || Pair <- List }. + "#]], + ) + } +} diff --git a/crates/ide_db/src/diagnostic_code.rs b/crates/ide_db/src/diagnostic_code.rs index b652cb128..71710a434 100644 --- a/crates/ide_db/src/diagnostic_code.rs +++ b/crates/ide_db/src/diagnostic_code.rs @@ -66,6 +66,7 @@ pub enum DiagnosticCode { MapsFindFunctionRatherThanSyntax, ListsZipWithSeqRatherThanEnumerate, UnnecessaryFoldToBuildMapFromList, + UnnecessaryMapFromListAroundComprehension, // Wrapper for erlang service diagnostic codes ErlangService(String), @@ -199,6 +200,7 @@ impl DiagnosticCode { DiagnosticCode::ListsZipWithSeqRatherThanEnumerate => "W0033".to_string(), DiagnosticCode::UnnecessaryMapToListInComprehension => "W0034".to_string(), DiagnosticCode::UnnecessaryFoldToBuildMapFromList => "W0035".to_string(), + DiagnosticCode::UnnecessaryMapFromListAroundComprehension => "W0036".to_string(), DiagnosticCode::ErlangService(c) => c.to_string(), DiagnosticCode::Eqwalizer(c) => format!("eqwalizer: {c}"), DiagnosticCode::AdHoc(c) => format!("ad-hoc: {c}"), @@ -268,6 +270,9 @@ impl DiagnosticCode { DiagnosticCode::UnnecessaryFoldToBuildMapFromList => { "unnecessary_fold_to_build_map_from_list".to_string() } + DiagnosticCode::UnnecessaryMapFromListAroundComprehension => { + "unnecessary_map_from_list_around_comprehension".to_string() + } DiagnosticCode::RecordTupleMatch => "record_tuple_match".to_string(), DiagnosticCode::ErlangService(c) => c.to_string(), DiagnosticCode::Eqwalizer(c) => c.to_string(), @@ -390,6 +395,7 @@ impl DiagnosticCode { DiagnosticCode::MapsFindFunctionRatherThanSyntax => false, DiagnosticCode::ListsZipWithSeqRatherThanEnumerate => false, DiagnosticCode::UnnecessaryFoldToBuildMapFromList => false, + DiagnosticCode::UnnecessaryMapFromListAroundComprehension => false, DiagnosticCode::CannotEvaluateCTCallbacks => false, DiagnosticCode::MeckMissingNoLinkInInitPerSuite => false, DiagnosticCode::AtomsExhaustion => false, diff --git a/website/docs/erlang-error-index/w/W0035.md b/website/docs/erlang-error-index/w/W0035.md index c43ed2bf7..e6b79a221 100644 --- a/website/docs/erlang-error-index/w/W0035.md +++ b/website/docs/erlang-error-index/w/W0035.md @@ -9,13 +9,13 @@ sidebar_position: 35 ```erlang main(List) -> lists:foldl(fun(K, Acc) -> Acc#{K => []} end, #{}, List). - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 information: Unnecessary explicit fold to construct map from list. +%% ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 information: Unnecessary explicit fold to construct map from list. ``` ```erlang main(List) -> lists:foldl(fun({K,V}, Acc) -> Acc#{K => V} end, #{}, List). - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 information: Unnecessary explicit fold to construct map from list. +%% ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 information: Unnecessary explicit fold to construct map from list. ``` ## Explanation diff --git a/website/docs/erlang-error-index/w/W0036.md b/website/docs/erlang-error-index/w/W0036.md new file mode 100644 index 000000000..cfb2c3b83 --- /dev/null +++ b/website/docs/erlang-error-index/w/W0036.md @@ -0,0 +1,32 @@ +--- +sidebar_position: 36 +--- + +# W0036 - unnecessary Map From List Around Comprehension + +## Warning + +```erlang +fn(List) -> maps:from_list([{K + 1, V + 2} || {K,V} <- List]). +%% ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 💡 weak: Unnecessary intermediate list allocated. +``` + +## Explanation + +The warning message indicates an unidiomatic and sub-optimal way to build a map +from a list. + +Whilst the comprehension and subsequent call to `maps:from_list/1` will ultimately +construct a map from the key-values pair in the given list, it is less +efficient than it could be, since an intermediate list is created by the list +comprehension. + +Using the map comprehension syntax is clearer and more performant. + +To fix this warning, replace the list comprehension and `maps:from_list/1` call +with a map comprehension: + +```erlang +main(List) -> + #{K + 1 => V + 2 || {K,V} <- List}. +```