From 341f8bb200d60caf4c0ea70738198ac8d62218b8 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 31 May 2021 19:06:40 +0300 Subject: fix: avoid panics in match case diagnostic --- crates/hir_ty/src/diagnostics/decl_check.rs | 141 +++++++--------------------- 1 file changed, 33 insertions(+), 108 deletions(-) diff --git a/crates/hir_ty/src/diagnostics/decl_check.rs b/crates/hir_ty/src/diagnostics/decl_check.rs index ef982cbcd..bfa53bdce 100644 --- a/crates/hir_ty/src/diagnostics/decl_check.rs +++ b/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(¶m_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( - func, - fn_name_replacement, - fn_param_replacements, - ); + if let Some(fn_name_replacement) = fn_name_replacement { + self.create_incorrect_case_diagnostic_for_func(func, fn_name_replacement); + } + 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, - fn_param_replacements: Vec, + fn_name_replacement: 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() { - 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, + let ast_ptr = match fn_src.value.name() { + Some(name) => name, None => { - always!( - fn_param_replacements.is_empty(), - "Replacements ({:?}) were generated for a function parameters which had no parameters list: {:?}", - fn_param_replacements, + never!( + "Replacement ({:?}) was generated for a function without a name: {:?}", + fn_name_replacement, 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 { - file: fn_src.file_id, - ident_type: IdentType::Argument, - ident: AstPtr::new(&ast_ptr), - expected_case: param_to_rename.expected_case, - ident_text: param_to_rename.current_name.to_string(), - suggested_text: param_to_rename.suggested_text, - }; + let diagnostic = IncorrectCase { + file: fn_src.file_id, + ident_type: IdentType::Function, + ident: AstPtr::new(&ast_ptr), + expected_case: fn_name_replacement.expected_case, + ident_text: fn_name_replacement.current_name.to_string(), + 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::Argument } 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, struct_fields_replacements: Vec, ) { - // 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; } @@ -1037,4 +957,9 @@ fn qualify() { "#, ) } + + #[test] // Issue #8809. + fn parenthesized_parameter() { + check_diagnostics(r#"fn f((O): _) {}"#) + } } -- cgit v1.2.3 From 159922de93bd0437093f9ca07eda4673ba285b89 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 31 May 2021 19:09:44 +0300 Subject: minor: it's Parameter, not Argument --- crates/hir_ty/src/diagnostics.rs | 4 ++-- crates/hir_ty/src/diagnostics/decl_check.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs index 7598e2193..283894704 100644 --- a/crates/hir_ty/src/diagnostics.rs +++ b/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", diff --git a/crates/hir_ty/src/diagnostics/decl_check.rs b/crates/hir_ty/src/diagnostics/decl_check.rs index bfa53bdce..cfb5d7320 100644 --- a/crates/hir_ty/src/diagnostics/decl_check.rs +++ b/crates/hir_ty/src/diagnostics/decl_check.rs @@ -256,7 +256,7 @@ impl<'a, 'b> DeclValidator<'a, 'b> { } let ident_type = - if is_param { IdentType::Argument } else { IdentType::Variable }; + if is_param { IdentType::Parameter } else { IdentType::Variable }; let diagnostic = IncorrectCase { file: source_ptr.file_id, @@ -643,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` "#, ); } -- cgit v1.2.3 From ee995dbfd441e20bba21306c41aec0049c1d7da4 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Mon, 31 May 2021 19:51:19 +0300 Subject: fix: fix shell injection in task spawning closes #9058 --- editors/code/src/tasks.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/editors/code/src/tasks.ts b/editors/code/src/tasks.ts index 694ee1e41..947b3f2e4 100644 --- a/editors/code/src/tasks.ts +++ b/editors/code/src/tasks.ts @@ -80,7 +80,7 @@ export async function buildCargoTask( throwOnError: boolean = false ): Promise { - 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, - // for example "wrapper cargo". Without manual preparation the overridden command will be quoted and fail to execute. - const fullCommand = [cargoCommand, ...args].join(" "); + const fullCommand = [...cargoCommand, ...args]; - exec = new vscode.ShellExecution(fullCommand, definition); + exec = new vscode.ProcessExecution(fullCommand[0], fullCommand.slice(1), definition); } return new vscode.Task( -- cgit v1.2.3