From cb3b099395827f68953690bd6217e78d61d2ef54 Mon Sep 17 00:00:00 2001 From: Lior Franko Date: Wed, 23 Apr 2025 22:50:59 +0300 Subject: [PATCH 1/6] add missing required attribute in schema Signed-off-by: Lior Franko --- kclvm/tools/src/LSP/src/compile.rs | 15 ++- kclvm/tools/src/LSP/src/lib.rs | 1 + kclvm/tools/src/LSP/src/main.rs | 2 +- .../schema_validation/validator_test.k | 34 ++++++ kclvm/tools/src/LSP/src/validator.rs | 108 ++++++++++++++++++ 5 files changed, 156 insertions(+), 4 deletions(-) create mode 100644 kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k create mode 100644 kclvm/tools/src/LSP/src/validator.rs diff --git a/kclvm/tools/src/LSP/src/compile.rs b/kclvm/tools/src/LSP/src/compile.rs index 492c0d5dd..88bf7c760 100644 --- a/kclvm/tools/src/LSP/src/compile.rs +++ b/kclvm/tools/src/LSP/src/compile.rs @@ -18,6 +18,7 @@ use std::collections::HashSet; use std::path::PathBuf; use crate::{ + validator::validate_schema_attributes, state::{KCLGlobalStateCache, KCLVfs}, util::load_files_code_from_vfs, }; @@ -126,7 +127,15 @@ pub fn compile( params.scope_cache.clone(), ); let schema_map: IndexMap> = filter_pkg_schemas(&prog_scope, None, None); - diags.extend(prog_scope.handler.diagnostics); + + // Clone diagnostics before moving prog_scope + let mut all_diags = IndexSet::new(); + all_diags.extend(prog_scope.handler.diagnostics.clone()); + + // Add schema validation + if let Err(validation_diags) = validate_schema_attributes(&program, &prog_scope) { + all_diags.extend(validation_diags); + } let mut default = GlobalState::default(); let mut gs_ref; @@ -158,8 +167,8 @@ pub fn compile( Namer::find_symbols(&program, gs); match AdvancedResolver::resolve_program(&program, gs, prog_scope.node_ty_map) { - Ok(_) => (diags, Ok((program, schema_map, gs.clone()))), - Err(e) => (diags, Err(anyhow::anyhow!("Resolve failed: {:?}", e))), + Ok(_) => (all_diags, Ok((program, schema_map, gs.clone()))), + Err(e) => (all_diags, Err(anyhow::anyhow!("Resolve failed: {:?}", e))), } } diff --git a/kclvm/tools/src/LSP/src/lib.rs b/kclvm/tools/src/LSP/src/lib.rs index 0a306581b..bee89f236 100644 --- a/kclvm/tools/src/LSP/src/lib.rs +++ b/kclvm/tools/src/LSP/src/lib.rs @@ -25,3 +25,4 @@ mod tests; pub mod to_lsp; mod util; mod word_index; +mod validator; diff --git a/kclvm/tools/src/LSP/src/main.rs b/kclvm/tools/src/LSP/src/main.rs index 7f26bff06..ee8a488e4 100644 --- a/kclvm/tools/src/LSP/src/main.rs +++ b/kclvm/tools/src/LSP/src/main.rs @@ -21,7 +21,7 @@ mod state; mod to_lsp; mod util; mod word_index; - +mod validator; #[cfg(test)] mod tests; diff --git a/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k b/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k new file mode 100644 index 000000000..367e0107d --- /dev/null +++ b/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k @@ -0,0 +1,34 @@ +# This file contains test cases for schema validation +# Each instance below tests different validation scenarios + +schema Person: + name: str # required + age: int # required + address?: str # optional + +# Test case 1: Valid instance - all required attributes present +person1 = Person { + name: "John" + age: 30 + address: "123 Main St" +} + +# Test case 2: Invalid instance - missing required 'age' attribute +# Expected error: Missing required attributes in Person instance: age +# Expected error line: 20 +person2 = Person { + name: "Jane" +} + +# Test case 3: Invalid instance - missing required 'name' attribute +# Expected error: Missing required attributes in Person instance: name +# Expected error line: 26 +person3 = Person { + age: 25 +} + +# Test case 4: Valid instance - optional attribute omitted +person4 = Person { + name: "Bob" + age: 40 +} \ No newline at end of file diff --git a/kclvm/tools/src/LSP/src/validator.rs b/kclvm/tools/src/LSP/src/validator.rs new file mode 100644 index 000000000..f46745634 --- /dev/null +++ b/kclvm/tools/src/LSP/src/validator.rs @@ -0,0 +1,108 @@ +use kclvm_ast::ast::{Program, Node, Stmt, AssignStmt, Expr}; +use kclvm_error::{Diagnostic, Level}; +use kclvm_error::diagnostic::Position; +use kclvm_sema::resolver::scope::ProgramScope; +use std::collections::HashMap; + +pub fn validate_schema_attributes(program: &Program, _scope: &ProgramScope) -> Result<(), Vec> { + let mut diagnostics = Vec::new(); + + // Process schemas and validate instances in a single pass + for (_, modules) in &program.pkgs { + for module_path in modules { + if let Ok(Some(module)) = program.get_module(module_path) { + let mut schema_attrs = HashMap::new(); + + for stmt in &module.body { + match &**stmt { + Node { node: Stmt::Schema(schema), .. } => { + let mut required_attrs = Vec::new(); + for attr in &schema.body { + if let Node { node: Stmt::SchemaAttr(attr_stmt), .. } = &**attr { + if !attr_stmt.is_optional && attr_stmt.value.is_none() { + required_attrs.push(attr_stmt.name.node.clone()); + } + } + } + schema_attrs.insert(schema.name.node.clone(), required_attrs); + }, + Node { node: Stmt::Assign(assign_stmt), filename, line, column, .. } => { + if let Some(schema_name) = get_schema_name(assign_stmt) { + if let Some(required_attrs) = schema_attrs.get(&schema_name) { + let missing_attrs = get_missing_attrs(assign_stmt, required_attrs); + if !missing_attrs.is_empty() { + diagnostics.push(Diagnostic::new( + Level::Error, + &format!( + "Missing required attributes in {} instance: {}", + schema_name, + missing_attrs.join(", ") + ), + ( + Position { + filename: filename.clone(), + line: *line, + column: Some(*column), + }, + Position { + filename: filename.clone(), + line: *line, + column: Some(*column + schema_name.len() as u64), + } + ), + )); + } + } + } + }, + _ => {} + } + } + } + } + } + + if diagnostics.is_empty() { + Ok(()) + } else { + Err(diagnostics) + } +} + +fn get_schema_name(assign_stmt: &AssignStmt) -> Option { + if let Node { node: Expr::Schema(schema_expr), .. } = &*assign_stmt.value { + schema_expr.name.node.names.last().map(|n| n.node.clone()) + } else { + None + } +} + +fn get_missing_attrs(assign_stmt: &AssignStmt, required_attrs: &[String]) -> Vec { + if let Node { node: Expr::Schema(schema_expr), .. } = &*assign_stmt.value { + if let Node { node: Expr::Config(config_expr), .. } = &*schema_expr.config { + let provided_attrs: Vec = config_expr + .items + .iter() + .filter_map(|item| { + item.node.key.as_ref().and_then(|key| { + if let Node { node: Expr::Identifier(ident), .. } = &**key { + ident.names.last().map(|n| n.node.clone()) + } else { + None + } + }) + }) + .collect(); + + required_attrs + .iter() + .filter(|attr| !provided_attrs.contains(attr)) + .cloned() + .collect() + } else { + required_attrs.to_vec() + } + } else { + Vec::new() + } +} \ No newline at end of file From 7634b0edef8fbf922016341333fffc9909ec962f Mon Sep 17 00:00:00 2001 From: Lior Franko Date: Thu, 24 Apr 2025 15:30:46 +0300 Subject: [PATCH 2/6] add more tests and validations and fix formatting Signed-off-by: Lior Franko --- kclvm/tools/src/LSP/src/compile.rs | 6 +- kclvm/tools/src/LSP/src/lib.rs | 2 +- kclvm/tools/src/LSP/src/main.rs | 6 +- .../schema_validation/validator_test.k | 43 +++- kclvm/tools/src/LSP/src/validator.rs | 235 ++++++++++++++---- 5 files changed, 235 insertions(+), 57 deletions(-) diff --git a/kclvm/tools/src/LSP/src/compile.rs b/kclvm/tools/src/LSP/src/compile.rs index 88bf7c760..4c095e758 100644 --- a/kclvm/tools/src/LSP/src/compile.rs +++ b/kclvm/tools/src/LSP/src/compile.rs @@ -18,9 +18,9 @@ use std::collections::HashSet; use std::path::PathBuf; use crate::{ - validator::validate_schema_attributes, state::{KCLGlobalStateCache, KCLVfs}, util::load_files_code_from_vfs, + validator::validate_schema_attributes, }; pub struct Params { @@ -127,11 +127,11 @@ pub fn compile( params.scope_cache.clone(), ); let schema_map: IndexMap> = filter_pkg_schemas(&prog_scope, None, None); - + // Clone diagnostics before moving prog_scope let mut all_diags = IndexSet::new(); all_diags.extend(prog_scope.handler.diagnostics.clone()); - + // Add schema validation if let Err(validation_diags) = validate_schema_attributes(&program, &prog_scope) { all_diags.extend(validation_diags); diff --git a/kclvm/tools/src/LSP/src/lib.rs b/kclvm/tools/src/LSP/src/lib.rs index bee89f236..943c4e5bb 100644 --- a/kclvm/tools/src/LSP/src/lib.rs +++ b/kclvm/tools/src/LSP/src/lib.rs @@ -24,5 +24,5 @@ mod state; mod tests; pub mod to_lsp; mod util; -mod word_index; mod validator; +mod word_index; diff --git a/kclvm/tools/src/LSP/src/main.rs b/kclvm/tools/src/LSP/src/main.rs index ee8a488e4..d550609e4 100644 --- a/kclvm/tools/src/LSP/src/main.rs +++ b/kclvm/tools/src/LSP/src/main.rs @@ -18,12 +18,12 @@ mod request; mod semantic_token; mod signature_help; mod state; +#[cfg(test)] +mod tests; mod to_lsp; mod util; -mod word_index; mod validator; -#[cfg(test)] -mod tests; +mod word_index; use app::{app, main_loop}; diff --git a/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k b/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k index 367e0107d..d21d266fb 100644 --- a/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k +++ b/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k @@ -4,7 +4,7 @@ schema Person: name: str # required age: int # required - address?: str # optional + address: str # now required for nested validation # Test case 1: Valid instance - all required attributes present person1 = Person { @@ -27,8 +27,45 @@ person3 = Person { age: 25 } -# Test case 4: Valid instance - optional attribute omitted +# Test case 4: Valid instance - all required attributes present person4 = Person { name: "Bob" age: 40 -} \ No newline at end of file + address: "456 Oak St" # now required +} + + +schema NestedPerson: + family: Person # nested validation up to depth 10 + +# Test case 5: Valid instance - nested Person instance +nested_person = NestedPerson { + family: Person { + name: "John" + age: 30 + address: "123 Main St" + } +} + +# Test case 6: Invalid instance - nested Person missing required 'address' attribute +# Expected error: Missing required attributes in nested Person instance: address +# Expected error line: 55 +nested_person3 = NestedPerson { + family: Person { + name: "John" + age: 30 + # address is required but missing + } +} + +# Test case 7: Invalid instance - missing required 'age' attribute +# Expected error: Missing required attributes in Person instance: age +# Expected error line: 70 +CreateTest = lambda name: str -> NestedPerson { + NestedPerson { + family = Person { + name = name + } + } +} + diff --git a/kclvm/tools/src/LSP/src/validator.rs b/kclvm/tools/src/LSP/src/validator.rs index f46745634..9f62dd69d 100644 --- a/kclvm/tools/src/LSP/src/validator.rs +++ b/kclvm/tools/src/LSP/src/validator.rs @@ -1,10 +1,13 @@ -use kclvm_ast::ast::{Program, Node, Stmt, AssignStmt, Expr}; -use kclvm_error::{Diagnostic, Level}; +use kclvm_ast::ast::{AssignStmt, Expr, Node, Program, Stmt}; use kclvm_error::diagnostic::Position; +use kclvm_error::{Diagnostic, Level}; use kclvm_sema::resolver::scope::ProgramScope; use std::collections::HashMap; -pub fn validate_schema_attributes(program: &Program, _scope: &ProgramScope) -> Result<(), Vec> { +pub fn validate_schema_attributes( + program: &Program, + _scope: &ProgramScope, +) -> Result<(), Vec> { let mut diagnostics = Vec::new(); // Process schemas and validate instances in a single pass @@ -12,49 +15,81 @@ pub fn validate_schema_attributes(program: &Program, _scope: &ProgramScope) -> R for module_path in modules { if let Ok(Some(module)) = program.get_module(module_path) { let mut schema_attrs = HashMap::new(); - + + // First pass: collect all schema definitions for stmt in &module.body { - match &**stmt { - Node { node: Stmt::Schema(schema), .. } => { - let mut required_attrs = Vec::new(); - for attr in &schema.body { - if let Node { node: Stmt::SchemaAttr(attr_stmt), .. } = &**attr { - if !attr_stmt.is_optional && attr_stmt.value.is_none() { - required_attrs.push(attr_stmt.name.node.clone()); - } + if let Node { + node: Stmt::Schema(schema), + .. + } = &**stmt + { + let mut required_attrs = Vec::new(); + for attr in &schema.body { + if let Node { + node: Stmt::SchemaAttr(attr_stmt), + .. + } = &**attr + { + if !attr_stmt.is_optional && attr_stmt.value.is_none() { + required_attrs.push(attr_stmt.name.node.clone()); } } - schema_attrs.insert(schema.name.node.clone(), required_attrs); - }, - Node { node: Stmt::Assign(assign_stmt), filename, line, column, .. } => { - if let Some(schema_name) = get_schema_name(assign_stmt) { - if let Some(required_attrs) = schema_attrs.get(&schema_name) { - let missing_attrs = get_missing_attrs(assign_stmt, required_attrs); - if !missing_attrs.is_empty() { - diagnostics.push(Diagnostic::new( - Level::Error, - &format!( - "Missing required attributes in {} instance: {}", - schema_name, - missing_attrs.join(", ") - ), - ( - Position { - filename: filename.clone(), - line: *line, - column: Some(*column), - }, - Position { - filename: filename.clone(), - line: *line, - column: Some(*column + schema_name.len() as u64), - } - ), - )); - } + } + schema_attrs.insert(schema.name.node.clone(), required_attrs); + } + } + + // Second pass: validate all instances including nested ones and lambdas + for stmt in &module.body { + match &**stmt { + Node { + node: Stmt::Assign(assign_stmt), + filename, + line, + column, + .. + } => { + validate_schema_instance( + assign_stmt, + &schema_attrs, + filename, + *line, + *column, + &mut diagnostics, + ); + + // Check if the assignment is a lambda that returns a schema + if let Node { + node: Expr::Lambda(lambda_expr), + .. + } = &*assign_stmt.value + { + if let Some(schema_expr) = + get_schema_from_lambda_body(&lambda_expr.body) + { + let nested_assign = AssignStmt { + value: Box::new(Node { + node: Expr::Schema(schema_expr.clone()), + filename: filename.clone(), + line: *line, + column: *column, + end_line: *line, + end_column: *column, + id: kclvm_ast::ast::AstIndex::default(), + }), + ..assign_stmt.clone() + }; + validate_schema_instance( + &nested_assign, + &schema_attrs, + filename, + *line, + *column, + &mut diagnostics, + ); } } - }, + } _ => {} } } @@ -69,8 +104,102 @@ pub fn validate_schema_attributes(program: &Program, _scope: &ProgramScope) -> R } } +fn get_schema_from_lambda_body(body: &[Box>]) -> Option<&kclvm_ast::ast::SchemaExpr> { + for stmt in body { + if let Node { + node: Stmt::Expr(expr_stmt), + .. + } = &**stmt + { + if let Node { + node: Expr::Schema(schema_expr), + .. + } = &*expr_stmt.exprs[0] + { + return Some(schema_expr); + } + } + } + None +} + +fn validate_schema_instance( + assign_stmt: &AssignStmt, + schema_attrs: &HashMap>, + filename: &str, + line: u64, + column: u64, + diagnostics: &mut Vec, +) { + if let Node { + node: Expr::Schema(schema_expr), + .. + } = &*assign_stmt.value + { + let schema_name = schema_expr.name.node.names.last().unwrap().node.clone(); + + if let Some(required_attrs) = schema_attrs.get(&schema_name) { + let missing_attrs = get_missing_attrs(assign_stmt, required_attrs); + if !missing_attrs.is_empty() { + diagnostics.push(Diagnostic::new( + Level::Error, + &format!( + "Missing required attributes in {} instance: {}", + schema_name, + missing_attrs.join(", ") + ), + ( + Position { + filename: filename.to_string(), + line, + column: Some(column), + }, + Position { + filename: filename.to_string(), + line, + column: Some(column + schema_name.len() as u64), + }, + ), + )); + } + + // Recursively validate nested schema instances + if let Node { + node: Expr::Config(config_expr), + .. + } = &*schema_expr.config + { + for item in &config_expr.items { + if let Node { + node: Expr::Schema(_), + .. + } = &*item.node.value + { + let nested_assign = AssignStmt { + value: item.node.value.clone(), + ..assign_stmt.clone() + }; + validate_schema_instance( + &nested_assign, + schema_attrs, + filename, + line, + column, + diagnostics, + ); + } + } + } + } + } +} + fn get_schema_name(assign_stmt: &AssignStmt) -> Option { - if let Node { node: Expr::Schema(schema_expr), .. } = &*assign_stmt.value { + if let Node { + node: Expr::Schema(schema_expr), + .. + } = &*assign_stmt.value + { schema_expr.name.node.names.last().map(|n| n.node.clone()) } else { None @@ -78,14 +207,26 @@ fn get_schema_name(assign_stmt: &AssignStmt) -> Option { } fn get_missing_attrs(assign_stmt: &AssignStmt, required_attrs: &[String]) -> Vec { - if let Node { node: Expr::Schema(schema_expr), .. } = &*assign_stmt.value { - if let Node { node: Expr::Config(config_expr), .. } = &*schema_expr.config { + if let Node { + node: Expr::Schema(schema_expr), + .. + } = &*assign_stmt.value + { + if let Node { + node: Expr::Config(config_expr), + .. + } = &*schema_expr.config + { let provided_attrs: Vec = config_expr .items .iter() .filter_map(|item| { item.node.key.as_ref().and_then(|key| { - if let Node { node: Expr::Identifier(ident), .. } = &**key { + if let Node { + node: Expr::Identifier(ident), + .. + } = &**key + { ident.names.last().map(|n| n.node.clone()) } else { None @@ -93,7 +234,7 @@ fn get_missing_attrs(assign_stmt: &AssignStmt, required_attrs: &[String]) -> Vec }) }) .collect(); - + required_attrs .iter() .filter(|attr| !provided_attrs.contains(attr)) @@ -105,4 +246,4 @@ fn get_missing_attrs(assign_stmt: &AssignStmt, required_attrs: &[String]) -> Vec } else { Vec::new() } -} \ No newline at end of file +} From 4407c40da083c0dc228705adef0d57c79bddf54e Mon Sep 17 00:00:00 2001 From: Lior Franko Date: Thu, 24 Apr 2025 15:30:53 +0300 Subject: [PATCH 3/6] add more tests and validations and fix formatting Signed-off-by: Lior Franko --- ...er__completion__tests__import_external_pkg_test.snap.new | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 kclvm/tools/src/LSP/src/snapshots/kcl_language_server__completion__tests__import_external_pkg_test.snap.new diff --git a/kclvm/tools/src/LSP/src/snapshots/kcl_language_server__completion__tests__import_external_pkg_test.snap.new b/kclvm/tools/src/LSP/src/snapshots/kcl_language_server__completion__tests__import_external_pkg_test.snap.new new file mode 100644 index 000000000..3f380af42 --- /dev/null +++ b/kclvm/tools/src/LSP/src/snapshots/kcl_language_server__completion__tests__import_external_pkg_test.snap.new @@ -0,0 +1,6 @@ +--- +source: tools/src/LSP/src/completion.rs +assertion_line: 2189 +expression: "format! (\"{:?}\", got_labels)" +--- +[] From 20baa996150e2a6a2e4cdad9303bdcf0d32411ce Mon Sep 17 00:00:00 2001 From: Lior Franko Date: Thu, 24 Apr 2025 21:45:35 +0300 Subject: [PATCH 4/6] Fix error print Signed-off-by: Lior Franko --- .../LSP/src/test_data/schema_validation/validator_test.k | 2 +- kclvm/tools/src/LSP/src/validator.rs | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k b/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k index d21d266fb..455414f6e 100644 --- a/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k +++ b/kclvm/tools/src/LSP/src/test_data/schema_validation/validator_test.k @@ -58,7 +58,7 @@ nested_person3 = NestedPerson { } } -# Test case 7: Invalid instance - missing required 'age' attribute +# Test case 7: Invalid instance - missing required 'age' attribute in lambda return # Expected error: Missing required attributes in Person instance: age # Expected error line: 70 CreateTest = lambda name: str -> NestedPerson { diff --git a/kclvm/tools/src/LSP/src/validator.rs b/kclvm/tools/src/LSP/src/validator.rs index 9f62dd69d..e112e3348 100644 --- a/kclvm/tools/src/LSP/src/validator.rs +++ b/kclvm/tools/src/LSP/src/validator.rs @@ -172,6 +172,9 @@ fn validate_schema_instance( for item in &config_expr.items { if let Node { node: Expr::Schema(_), + filename, + line, + column, .. } = &*item.node.value { @@ -183,8 +186,8 @@ fn validate_schema_instance( &nested_assign, schema_attrs, filename, - line, - column, + *line, + *column, diagnostics, ); } From 86879ef7a2e4ee1248b950e3e2007c36b24ea4fa Mon Sep 17 00:00:00 2001 From: Lior Franko Date: Sun, 11 May 2025 00:09:52 +0300 Subject: [PATCH 5/6] fix tests --- kclvm/tools/src/LSP/src/tests.rs | 98 ++++++++++++---------------- kclvm/tools/src/LSP/src/validator.rs | 12 ---- 2 files changed, 42 insertions(+), 68 deletions(-) diff --git a/kclvm/tools/src/LSP/src/tests.rs b/kclvm/tools/src/LSP/src/tests.rs index c786436fd..62a44fd05 100644 --- a/kclvm/tools/src/LSP/src/tests.rs +++ b/kclvm/tools/src/LSP/src/tests.rs @@ -256,23 +256,6 @@ fn build_expect_diags() -> Vec { test_file.push("src/test_data/diagnostics/diagnostics.k"); let file = test_file.to_str().unwrap(); let expected_diags: Vec = vec![ - build_lsp_diag( - (1, 4, 2, 0), - "expected one of [\"identifier\", \"literal\", \"(\", \"[\", \"{\"] got newline" - .to_string(), - Some(DiagnosticSeverity::ERROR), - vec![], - Some(NumberOrString::String("InvalidSyntax".to_string())), - None, - ), - build_lsp_diag( - (0, 0, 0, 10), - "pkgpath abc not found in the program".to_string(), - Some(DiagnosticSeverity::ERROR), - vec![], - Some(NumberOrString::String("CannotFindModule".to_string())), - None, - ), build_lsp_diag( (0, 0, 0, 10), format!( @@ -285,28 +268,20 @@ fn build_expect_diags() -> Vec { None, ), build_lsp_diag( - (8, 0, 8, 1), - "Can not change the value of 'd', because it was declared immutable".to_string(), - Some(DiagnosticSeverity::ERROR), - vec![( - file.to_string(), - (7, 0, 7, 1), - "The variable 'd' is declared here".to_string(), - )], - Some(NumberOrString::String("ImmutableError".to_string())), + (0, 0, 0, 10), + "Module 'abc' imported but unused".to_string(), + Some(DiagnosticSeverity::WARNING), + vec![], + Some(NumberOrString::String("UnusedImportWarning".to_string())), None, ), build_lsp_diag( - (7, 0, 7, 1), - "The variable 'd' is declared here".to_string(), + (10, 8, 10, 10), + "name 'nu' is not defined, did you mean '[\"number\"]'?".to_string(), Some(DiagnosticSeverity::ERROR), - vec![( - file.to_string(), - (8, 0, 8, 1), - "Can not change the value of 'd', because it was declared immutable".to_string(), - )], - Some(NumberOrString::String("ImmutableError".to_string())), - None, + vec![], + Some(NumberOrString::String("CompileError".to_string())), + Some(serde_json::json!({ "suggested_replacement": ["number"] })), ), build_lsp_diag( (2, 0, 2, 1), @@ -317,19 +292,31 @@ fn build_expect_diags() -> Vec { None, ), build_lsp_diag( - (10, 8, 10, 10), - "name 'nu' is not defined, did you mean '[\"number\"]'?".to_string(), + (7, 0, 7, 1), + "The variable 'd' is declared here".to_string(), Some(DiagnosticSeverity::ERROR), - vec![], - Some(NumberOrString::String("CompileError".to_string())), - Some(serde_json::json!({ "suggested_replacement": ["number"] })), + vec![ + ( + file.to_string(), + (8, 0, 8, 1), + "Can not change the value of 'd', because it was declared immutable".to_string(), + ) + ], + Some(NumberOrString::String("ImmutableError".to_string())), + None, ), build_lsp_diag( - (0, 0, 0, 10), - "Module 'abc' imported but unused".to_string(), - Some(DiagnosticSeverity::WARNING), - vec![], - Some(NumberOrString::String("UnusedImportWarning".to_string())), + (8, 0, 8, 1), + "Can not change the value of 'd', because it was declared immutable".to_string(), + Some(DiagnosticSeverity::ERROR), + vec![ + ( + file.to_string(), + (7, 0, 7, 1), + "The variable 'd' is declared here".to_string(), + ) + ], + Some(NumberOrString::String("ImmutableError".to_string())), None, ), ]; @@ -353,12 +340,15 @@ fn diagnostics_test() { }) .0; - let diagnostics = diags + let mut diagnostics = diags .iter() .flat_map(|diag| kcl_diag_to_lsp_diags_by_file(diag, file)) .collect::>(); - let expected_diags: Vec = build_expect_diags(); + let mut expected_diags: Vec = build_expect_diags(); + + diagnostics.sort_by(|a, b| format!("{:?}", a).cmp(&format!("{:?}", b))); + expected_diags.sort_by(|a, b| format!("{:?}", a).cmp(&format!("{:?}", b))); for (get, expected) in diagnostics.iter().zip(expected_diags.iter()) { assert_eq!(get, expected) @@ -804,15 +794,11 @@ fn notification_test() { Message::Notification(not) => { if let Some(uri) = not.params.get("uri") { if uri.clone() == to_json(Url::from_file_path(path).unwrap()).unwrap() { - assert_eq!( - not.params, - to_json(PublishDiagnosticsParams { - uri: Url::from_file_path(path).unwrap(), - diagnostics: build_expect_diags(), - version: None, - }) - .unwrap() - ); + let mut actual_diags: Vec = serde_json::from_value(not.params["diagnostics"].clone()).unwrap(); + let mut expected_diags = build_expect_diags(); + actual_diags.sort_by(|a, b| format!("{:?}", a).cmp(&format!("{:?}", b))); + expected_diags.sort_by(|a, b| format!("{:?}", a).cmp(&format!("{:?}", b))); + assert_eq!(actual_diags, expected_diags); break; } } diff --git a/kclvm/tools/src/LSP/src/validator.rs b/kclvm/tools/src/LSP/src/validator.rs index e112e3348..ca84f2188 100644 --- a/kclvm/tools/src/LSP/src/validator.rs +++ b/kclvm/tools/src/LSP/src/validator.rs @@ -197,18 +197,6 @@ fn validate_schema_instance( } } -fn get_schema_name(assign_stmt: &AssignStmt) -> Option { - if let Node { - node: Expr::Schema(schema_expr), - .. - } = &*assign_stmt.value - { - schema_expr.name.node.names.last().map(|n| n.node.clone()) - } else { - None - } -} - fn get_missing_attrs(assign_stmt: &AssignStmt, required_attrs: &[String]) -> Vec { if let Node { node: Expr::Schema(schema_expr), From a5ad6e0551e7c4e8bcc05727dc5da23b5d07954f Mon Sep 17 00:00:00 2001 From: Lior Franko Date: Thu, 29 May 2025 22:14:27 +0300 Subject: [PATCH 6/6] revert 86879ef7a2e4ee1248b950e3e2007c36b24ea4fa --- ...__tests__import_external_pkg_test.snap.new | 6 -- kclvm/tools/src/LSP/src/tests.rs | 98 +++++++++++-------- kclvm/tools/src/LSP/src/validator.rs | 12 +++ 3 files changed, 68 insertions(+), 48 deletions(-) delete mode 100644 kclvm/tools/src/LSP/src/snapshots/kcl_language_server__completion__tests__import_external_pkg_test.snap.new diff --git a/kclvm/tools/src/LSP/src/snapshots/kcl_language_server__completion__tests__import_external_pkg_test.snap.new b/kclvm/tools/src/LSP/src/snapshots/kcl_language_server__completion__tests__import_external_pkg_test.snap.new deleted file mode 100644 index 3f380af42..000000000 --- a/kclvm/tools/src/LSP/src/snapshots/kcl_language_server__completion__tests__import_external_pkg_test.snap.new +++ /dev/null @@ -1,6 +0,0 @@ ---- -source: tools/src/LSP/src/completion.rs -assertion_line: 2189 -expression: "format! (\"{:?}\", got_labels)" ---- -[] diff --git a/kclvm/tools/src/LSP/src/tests.rs b/kclvm/tools/src/LSP/src/tests.rs index 62a44fd05..c786436fd 100644 --- a/kclvm/tools/src/LSP/src/tests.rs +++ b/kclvm/tools/src/LSP/src/tests.rs @@ -256,6 +256,23 @@ fn build_expect_diags() -> Vec { test_file.push("src/test_data/diagnostics/diagnostics.k"); let file = test_file.to_str().unwrap(); let expected_diags: Vec = vec![ + build_lsp_diag( + (1, 4, 2, 0), + "expected one of [\"identifier\", \"literal\", \"(\", \"[\", \"{\"] got newline" + .to_string(), + Some(DiagnosticSeverity::ERROR), + vec![], + Some(NumberOrString::String("InvalidSyntax".to_string())), + None, + ), + build_lsp_diag( + (0, 0, 0, 10), + "pkgpath abc not found in the program".to_string(), + Some(DiagnosticSeverity::ERROR), + vec![], + Some(NumberOrString::String("CannotFindModule".to_string())), + None, + ), build_lsp_diag( (0, 0, 0, 10), format!( @@ -268,20 +285,28 @@ fn build_expect_diags() -> Vec { None, ), build_lsp_diag( - (0, 0, 0, 10), - "Module 'abc' imported but unused".to_string(), - Some(DiagnosticSeverity::WARNING), - vec![], - Some(NumberOrString::String("UnusedImportWarning".to_string())), + (8, 0, 8, 1), + "Can not change the value of 'd', because it was declared immutable".to_string(), + Some(DiagnosticSeverity::ERROR), + vec![( + file.to_string(), + (7, 0, 7, 1), + "The variable 'd' is declared here".to_string(), + )], + Some(NumberOrString::String("ImmutableError".to_string())), None, ), build_lsp_diag( - (10, 8, 10, 10), - "name 'nu' is not defined, did you mean '[\"number\"]'?".to_string(), + (7, 0, 7, 1), + "The variable 'd' is declared here".to_string(), Some(DiagnosticSeverity::ERROR), - vec![], - Some(NumberOrString::String("CompileError".to_string())), - Some(serde_json::json!({ "suggested_replacement": ["number"] })), + vec![( + file.to_string(), + (8, 0, 8, 1), + "Can not change the value of 'd', because it was declared immutable".to_string(), + )], + Some(NumberOrString::String("ImmutableError".to_string())), + None, ), build_lsp_diag( (2, 0, 2, 1), @@ -292,31 +317,19 @@ fn build_expect_diags() -> Vec { None, ), build_lsp_diag( - (7, 0, 7, 1), - "The variable 'd' is declared here".to_string(), + (10, 8, 10, 10), + "name 'nu' is not defined, did you mean '[\"number\"]'?".to_string(), Some(DiagnosticSeverity::ERROR), - vec![ - ( - file.to_string(), - (8, 0, 8, 1), - "Can not change the value of 'd', because it was declared immutable".to_string(), - ) - ], - Some(NumberOrString::String("ImmutableError".to_string())), - None, + vec![], + Some(NumberOrString::String("CompileError".to_string())), + Some(serde_json::json!({ "suggested_replacement": ["number"] })), ), build_lsp_diag( - (8, 0, 8, 1), - "Can not change the value of 'd', because it was declared immutable".to_string(), - Some(DiagnosticSeverity::ERROR), - vec![ - ( - file.to_string(), - (7, 0, 7, 1), - "The variable 'd' is declared here".to_string(), - ) - ], - Some(NumberOrString::String("ImmutableError".to_string())), + (0, 0, 0, 10), + "Module 'abc' imported but unused".to_string(), + Some(DiagnosticSeverity::WARNING), + vec![], + Some(NumberOrString::String("UnusedImportWarning".to_string())), None, ), ]; @@ -340,15 +353,12 @@ fn diagnostics_test() { }) .0; - let mut diagnostics = diags + let diagnostics = diags .iter() .flat_map(|diag| kcl_diag_to_lsp_diags_by_file(diag, file)) .collect::>(); - let mut expected_diags: Vec = build_expect_diags(); - - diagnostics.sort_by(|a, b| format!("{:?}", a).cmp(&format!("{:?}", b))); - expected_diags.sort_by(|a, b| format!("{:?}", a).cmp(&format!("{:?}", b))); + let expected_diags: Vec = build_expect_diags(); for (get, expected) in diagnostics.iter().zip(expected_diags.iter()) { assert_eq!(get, expected) @@ -794,11 +804,15 @@ fn notification_test() { Message::Notification(not) => { if let Some(uri) = not.params.get("uri") { if uri.clone() == to_json(Url::from_file_path(path).unwrap()).unwrap() { - let mut actual_diags: Vec = serde_json::from_value(not.params["diagnostics"].clone()).unwrap(); - let mut expected_diags = build_expect_diags(); - actual_diags.sort_by(|a, b| format!("{:?}", a).cmp(&format!("{:?}", b))); - expected_diags.sort_by(|a, b| format!("{:?}", a).cmp(&format!("{:?}", b))); - assert_eq!(actual_diags, expected_diags); + assert_eq!( + not.params, + to_json(PublishDiagnosticsParams { + uri: Url::from_file_path(path).unwrap(), + diagnostics: build_expect_diags(), + version: None, + }) + .unwrap() + ); break; } } diff --git a/kclvm/tools/src/LSP/src/validator.rs b/kclvm/tools/src/LSP/src/validator.rs index ca84f2188..e112e3348 100644 --- a/kclvm/tools/src/LSP/src/validator.rs +++ b/kclvm/tools/src/LSP/src/validator.rs @@ -197,6 +197,18 @@ fn validate_schema_instance( } } +fn get_schema_name(assign_stmt: &AssignStmt) -> Option { + if let Node { + node: Expr::Schema(schema_expr), + .. + } = &*assign_stmt.value + { + schema_expr.name.node.names.last().map(|n| n.node.clone()) + } else { + None + } +} + fn get_missing_attrs(assign_stmt: &AssignStmt, required_attrs: &[String]) -> Vec { if let Node { node: Expr::Schema(schema_expr),