Merge #9084 #9087

9084: fix: avoid panics in match case diagnostic r=matklad a=matklad

bors r+
🤖

closes #8809

9087: fix: fix shell injection in task spawning r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

crates/hir_ty/src/diagnostics.rs

@@ -349,11 +349,11 @@ impl fmt::Display for CaseType {
 
 #[derive(Debug)]
 pub enum IdentType {
-    Argument,
     Constant,
     Enum,
     Field,
     Function,
+    Parameter,
     StaticVariable,
     Structure,
     Variable,
@@ -363,11 +363,11 @@ pub enum IdentType {
 impl fmt::Display for IdentType {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let repr = match self {
-            IdentType::Argument => "Argument",
             IdentType::Constant => "Constant",
             IdentType::Enum => "Enum",
             IdentType::Field => "Field",
             IdentType::Function => "Function",
+            IdentType::Parameter => "Parameter",
             IdentType::StaticVariable => "Static variable",
             IdentType::Structure => "Structure",
             IdentType::Variable => "Variable",

crates/hir_ty/src/diagnostics/decl_check.rs

@@ -150,29 +150,11 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
             expected_case: CaseType::LowerSnakeCase,
         });
 
-        // Check the param names.
-        let fn_param_replacements = body
-            .params
-            .iter()
-            .filter_map(|&id| match &body[id] {
-                Pat::Bind { name, .. } => Some(name),
-                _ => None,
-            })
-            .filter_map(|param_name| {
-                Some(Replacement {
-                    current_name: param_name.clone(),
-                    suggested_text: to_lower_snake_case(&param_name.to_string())?,
-                    expected_case: CaseType::LowerSnakeCase,
-                })
-            })
-            .collect();
-
         // Check the patterns inside the function body.
+        // This includes function parameters.
         let pats_replacements = body
             .pats
             .iter()
-            // We aren't interested in function parameters, we've processed them above.
-            .filter(|(pat_idx, _)| !body.params.contains(&pat_idx))
             .filter_map(|(id, pat)| match pat {
                 Pat::Bind { name, .. } => Some((id, name)),
                 _ => None,
@@ -190,11 +172,10 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
             .collect();
 
         // If there is at least one element to spawn a warning on, go to the source map and generate a warning.
-        self.create_incorrect_case_diagnostic_for_func(
+        if let Some(fn_name_replacement) = fn_name_replacement {
-            func,
+            self.create_incorrect_case_diagnostic_for_func(func, fn_name_replacement);
-            fn_name_replacement,
+        }
-            fn_param_replacements,
+
-        );
         self.create_incorrect_case_diagnostic_for_variables(func, pats_replacements);
     }
 
@@ -203,100 +184,34 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
     fn create_incorrect_case_diagnostic_for_func(
         &mut self,
         func: FunctionId,
-        fn_name_replacement: Option<Replacement>,
+        fn_name_replacement: Replacement,
-        fn_param_replacements: Vec<Replacement>,
     ) {
-        // XXX: only look at sources if we do have incorrect names
-        if fn_name_replacement.is_none() && fn_param_replacements.is_empty() {
-            return;
-        }
-
         let fn_loc = func.lookup(self.db.upcast());
         let fn_src = fn_loc.source(self.db.upcast());
 
         // Diagnostic for function name.
-        if let Some(replacement) = fn_name_replacement {
+        let ast_ptr = match fn_src.value.name() {
-            let ast_ptr = match fn_src.value.name() {
+            Some(name) => name,
-                Some(name) => name,
-                None => {
-                    never!(
-                        "Replacement ({:?}) was generated for a function without a name: {:?}",
-                        replacement,
-                        fn_src
-                    );
-                    return;
-                }
-            };
-
-            let diagnostic = IncorrectCase {
-                file: fn_src.file_id,
-                ident_type: IdentType::Function,
-                ident: AstPtr::new(&ast_ptr),
-                expected_case: replacement.expected_case,
-                ident_text: replacement.current_name.to_string(),
-                suggested_text: replacement.suggested_text,
-            };
-
-            self.sink.push(diagnostic);
-        }
-
-        // Diagnostics for function params.
-        let fn_params_list = match fn_src.value.param_list() {
-            Some(params) => params,
             None => {
-                always!(
+                never!(
-                    fn_param_replacements.is_empty(),
+                    "Replacement ({:?}) was generated for a function without a name: {:?}",
-                    "Replacements ({:?}) were generated for a function parameters which had no parameters list: {:?}",
+                    fn_name_replacement,
-                    fn_param_replacements,
                     fn_src
                 );
                 return;
             }
         };
-        let mut fn_params_iter = fn_params_list.params();
-        for param_to_rename in fn_param_replacements {
-            // We assume that parameters in replacement are in the same order as in the
-            // actual params list, but just some of them (ones that named correctly) are skipped.
-            let ast_ptr: ast::Name = loop {
-                match fn_params_iter.next() {
-                    Some(element) => {
-                        if let Some(ast::Pat::IdentPat(pat)) = element.pat() {
-                            if pat.to_string() == param_to_rename.current_name.to_string() {
-                                if let Some(name) = pat.name() {
-                                    break name;
-                                }
-                                // This is critical. If we consider this parameter the expected one,
-                                // it **must** have a name.
-                                never!(
-                                    "Pattern {:?} equals to expected replacement {:?}, but has no name",
-                                    element,
-                                    param_to_rename
-                                );
-                                return;
-                            }
-                        }
-                    }
-                    None => {
-                        never!(
-                            "Replacement ({:?}) was generated for a function parameter which was not found: {:?}",
-                            param_to_rename, fn_src
-                        );
-                        return;
-                    }
-                }
-            };
 
-            let diagnostic = IncorrectCase {
+        let diagnostic = IncorrectCase {
-                file: fn_src.file_id,
+            file: fn_src.file_id,
-                ident_type: IdentType::Argument,
+            ident_type: IdentType::Function,
-                ident: AstPtr::new(&ast_ptr),
+            ident: AstPtr::new(&ast_ptr),
-                expected_case: param_to_rename.expected_case,
+            expected_case: fn_name_replacement.expected_case,
-                ident_text: param_to_rename.current_name.to_string(),
+            ident_text: fn_name_replacement.current_name.to_string(),
-                suggested_text: param_to_rename.suggested_text,
+            suggested_text: fn_name_replacement.suggested_text,
-            };
+        };
 
-            self.sink.push(diagnostic);
+        self.sink.push(diagnostic);
-        }
     }
 
     /// Given the information about incorrect variable names, looks up into the source code
@@ -327,20 +242,25 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
                             None => continue,
                         };
 
+                        let is_param = ast::Param::can_cast(parent.kind());
+
                         // We have to check that it's either `let var = ...` or `var @ Variant(_)` statement,
                         // because e.g. match arms are patterns as well.
                         // In other words, we check that it's a named variable binding.
                         let is_binding = ast::LetStmt::can_cast(parent.kind())
                             || (ast::MatchArm::can_cast(parent.kind())
                                 && ident_pat.at_token().is_some());
-                        if !is_binding {
+                        if !(is_param || is_binding) {
                             // This pattern is not an actual variable declaration, e.g. `Some(val) => {..}` match arm.
                             continue;
                         }
 
+                        let ident_type =
+                            if is_param { IdentType::Parameter } else { IdentType::Variable };
+
                         let diagnostic = IncorrectCase {
                             file: source_ptr.file_id,
-                            ident_type: IdentType::Variable,
+                            ident_type,
                             ident: AstPtr::new(&name_ast),
                             expected_case: replacement.expected_case,
                             ident_text: replacement.current_name.to_string(),
@@ -408,7 +328,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> {
         struct_name_replacement: Option<Replacement>,
         struct_fields_replacements: Vec<Replacement>,
     ) {
-        // XXX: only look at sources if we do have incorrect names
+        // XXX: Only look at sources if we do have incorrect names.
         if struct_name_replacement.is_none() && struct_fields_replacements.is_empty() {
             return;
         }
@@ -723,10 +643,10 @@ fn NonSnakeCaseName() {}
         check_diagnostics(
             r#"
 fn foo(SomeParam: u8) {}
-    // ^^^^^^^^^ Argument `SomeParam` should have snake_case name, e.g. `some_param`
+    // ^^^^^^^^^ Parameter `SomeParam` should have snake_case name, e.g. `some_param`
 
 fn foo2(ok_param: &str, CAPS_PARAM: u8) {}
-                     // ^^^^^^^^^^ Argument `CAPS_PARAM` should have snake_case name, e.g. `caps_param`
+                     // ^^^^^^^^^^ Parameter `CAPS_PARAM` should have snake_case name, e.g. `caps_param`
 "#,
         );
     }
@@ -1037,4 +957,9 @@ fn qualify() {
             "#,
         )
     }
+
+    #[test] // Issue #8809.
+    fn parenthesized_parameter() {
+        check_diagnostics(r#"fn f((O): _) {}"#)
+    }
 }

editors/code/src/tasks.ts

@@ -80,7 +80,7 @@ export async function buildCargoTask(
     throwOnError: boolean = false
 ): Promise<vscode.Task> {
 
-    let exec: vscode.ShellExecution | undefined = undefined;
+    let exec: vscode.ProcessExecution | vscode.ShellExecution | undefined = undefined;
 
     if (customRunner) {
         const runnerCommand = `${customRunner}.buildShellExecution`;
@@ -105,13 +105,13 @@ export async function buildCargoTask(
 
     if (!exec) {
         // Check whether we must use a user-defined substitute for cargo.
-        const cargoCommand = definition.overrideCargo ? definition.overrideCargo : toolchain.cargoPath();
+        // Split on spaces to allow overrides like "wrapper cargo".
+        const overrideCargo = definition.overrideCargo ?? definition.overrideCargo;
+        const cargoCommand = overrideCargo?.split(" ") ?? [toolchain.cargoPath()];
 
-        // Prepare the whole command as one line. It is required if user has provided override command which contains spaces,
+        const fullCommand = [...cargoCommand, ...args];
-        // for example "wrapper cargo". Without manual preparation the overridden command will be quoted and fail to execute.
-        const fullCommand = [cargoCommand, ...args].join(" ");
 
-        exec = new vscode.ShellExecution(fullCommand, definition);
+        exec = new vscode.ProcessExecution(fullCommand[0], fullCommand.slice(1), definition);
     }
 
     return new vscode.Task(