From cb3b099395827f68953690bd6217e78d61d2ef54 Mon Sep 17 00:00:00 2001 From: Lior Franko Date: Wed, 23 Apr 2025 22:50:59 +0300 Subject: [PATCH 1/4] 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/4] 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/4] 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/4] 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, ); }