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

@@ -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,

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<Diagnostic>,
+    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<Diagnostic> {
+    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 }.
+            "#]],
+        )
+    }
+}

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,

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

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}.
+```