Lint suggestion variables on non-camelCase (#6590)

* initial

Signed-off-by: Jess Frazelle <github@jessfraz.com>

* updates

Signed-off-by: Jess Frazelle <github@jessfraz.com>

* updates

Signed-off-by: Jess Frazelle <github@jessfraz.com>

---------

Signed-off-by: Jess Frazelle <github@jessfraz.com>

rust/kcl-lib/src/lint/checks/camel_case.rs

@@ -2,8 +2,9 @@ use anyhow::Result;
 use convert_case::Casing;
 
 use crate::{
+    errors::Suggestion,
     lint::rule::{def_finding, Discovered, Finding},
-    parsing::ast::types::{ObjectProperty, VariableDeclarator},
+    parsing::ast::types::{Node as AstNode, ObjectProperty, Program, VariableDeclarator},
     walk::Node,
     SourceRange,
 };
@@ -23,16 +24,28 @@ https://en.wikipedia.org/wiki/Camel_case
 "
 );
 
-fn lint_lower_camel_case_var(decl: &VariableDeclarator) -> Result<Vec<Discovered>> {
+fn lint_lower_camel_case_var(decl: &VariableDeclarator, prog: &AstNode<Program>) -> Result<Vec<Discovered>> {
     let mut findings = vec![];
     let ident = &decl.id;
     let name = &ident.name;
 
     if !name.is_case(convert_case::Case::Camel) {
+        // Get what it should be.
+        let new_name = name.to_case(convert_case::Case::Camel);
+
+        let mut prog = prog.clone();
+        prog.rename_symbol(&new_name, ident.start);
+        let recast = prog.recast(&Default::default(), 0);
+
+        let suggestion = Suggestion {
+            title: format!("rename '{}' to '{}'", name, new_name),
+            insert: recast,
+            source_range: prog.as_source_range(),
+        };
         findings.push(Z0001.at(
             format!("found '{}'", name),
             SourceRange::new(ident.start, ident.end, ident.module_id),
-            None,
+            Some(suggestion.clone()),
         ));
         return Ok(findings);
     }
@@ -40,12 +53,13 @@ fn lint_lower_camel_case_var(decl: &VariableDeclarator) -> Result<Vec<Discovered
     Ok(findings)
 }
 
-fn lint_lower_camel_case_property(decl: &ObjectProperty) -> Result<Vec<Discovered>> {
+fn lint_lower_camel_case_property(decl: &ObjectProperty, _prog: &AstNode<Program>) -> Result<Vec<Discovered>> {
     let mut findings = vec![];
     let ident = &decl.key;
     let name = &ident.name;
 
     if !name.is_case(convert_case::Case::Camel) {
+        // We can't rename the properties yet.
         findings.push(Z0001.at(
             format!("found '{}'", name),
             SourceRange::new(ident.start, ident.end, ident.module_id),
@@ -57,15 +71,15 @@ fn lint_lower_camel_case_property(decl: &ObjectProperty) -> Result<Vec<Discovere
     Ok(findings)
 }
 
-pub fn lint_variables(decl: Node) -> Result<Vec<Discovered>> {
+pub fn lint_variables(decl: Node, prog: &AstNode<Program>) -> Result<Vec<Discovered>> {
     let Node::VariableDeclaration(decl) = decl else {
         return Ok(vec![]);
     };
 
-    lint_lower_camel_case_var(&decl.declaration)
+    lint_lower_camel_case_var(&decl.declaration, prog)
 }
 
-pub fn lint_object_properties(decl: Node) -> Result<Vec<Discovered>> {
+pub fn lint_object_properties(decl: Node, prog: &AstNode<Program>) -> Result<Vec<Discovered>> {
     let Node::ObjectExpression(decl) = decl else {
         return Ok(vec![]);
     };
@@ -73,7 +87,7 @@ pub fn lint_object_properties(decl: Node) -> Result<Vec<Discovered>> {
     Ok(decl
         .properties
         .iter()
-        .flat_map(|v| lint_lower_camel_case_property(v).unwrap_or_default())
+        .flat_map(|v| lint_lower_camel_case_property(v, prog).unwrap_or_default())
         .collect())
 }
 
@@ -84,17 +98,44 @@ mod tests {
 
     #[tokio::test]
     async fn z0001_const() {
-        assert_finding!(lint_variables, Z0001, "Thickness = 0.5", "found 'Thickness'", None);
-        assert_finding!(lint_variables, Z0001, "THICKNESS = 0.5", "found 'THICKNESS'", None);
-        assert_finding!(lint_variables, Z0001, "THICC_NES = 0.5", "found 'THICC_NES'", None);
-        assert_finding!(lint_variables, Z0001, "thicc_nes = 0.5", "found 'thicc_nes'", None);
-    }
-
-    test_finding!(
-        z0001_full_bad,
+        assert_finding!(
             lint_variables,
             Z0001,
-        "\
+            "Thickness = 0.5",
+            "found 'Thickness'",
+            Some("thickness = 0.5\n".to_string())
+        );
+        assert_finding!(
+            lint_variables,
+            Z0001,
+            "THICKNESS = 0.5",
+            "found 'THICKNESS'",
+            Some("thickness = 0.5\n".to_string())
+        );
+        assert_finding!(
+            lint_variables,
+            Z0001,
+            "THICC_NES = 0.5",
+            "found 'THICC_NES'",
+            Some("thiccNes = 0.5\n".to_string())
+        );
+        assert_finding!(
+            lint_variables,
+            Z0001,
+            "thicc_nes = 0.5",
+            "found 'thicc_nes'",
+            Some("thiccNes = 0.5\n".to_string())
+        );
+        assert_finding!(
+            lint_variables,
+            Z0001,
+            "myAPIVar = 0.5",
+            "found 'myAPIVar'",
+            Some("myApiVar = 0.5\n".to_string())
+        );
+    }
+
+    const FULL_BAD: &str = "\
 // Define constants
 pipeLength = 40
 pipeSmallDia = 10
@@ -117,9 +158,15 @@ Part001 = startSketchOn(XY)
   |> angledLine(angle = 60, endAbsoluteX = pipeLargeDia)
   |> close()
   |> revolve(axis = Y)
-", 
+";
+
+    test_finding!(
+        z0001_full_bad,
+        lint_variables,
+        Z0001,
+        FULL_BAD,
         "found 'Part001'",
-    None
+        Some(FULL_BAD.replace("Part001", "part001").to_string())
     );
 
     test_no_finding!(

rust/kcl-lib/src/lint/checks/default_plane.rs

@@ -3,6 +3,7 @@ use anyhow::Result;
 use crate::{
     errors::Suggestion,
     lint::rule::{def_finding, Discovered, Finding},
+    parsing::ast::types::{Node as AstNode, Program},
     walk::Node,
 };
 
@@ -21,7 +22,7 @@ These are the default planes: XY, -XY, XZ, -XZ, YZ, -YZ.
 "
 );
 
-pub fn lint_should_be_default_plane(node: Node) -> Result<Vec<Discovered>> {
+pub fn lint_should_be_default_plane(node: Node, _prog: &AstNode<Program>) -> Result<Vec<Discovered>> {
     let Some((call_source_range, plane_name, offset)) = start_sketch_on_check_specific_plane(node)? else {
         return Ok(vec![]);
     };

rust/kcl-lib/src/lint/checks/offset_plane.rs

@@ -5,7 +5,7 @@ use crate::{
     errors::Suggestion,
     execution::{types::UnitLen, PlaneInfo, Point3d},
     lint::rule::{def_finding, Discovered, Finding},
-    parsing::ast::types::{BinaryPart, Expr, LiteralValue, ObjectExpression, UnaryOperator},
+    parsing::ast::types::{BinaryPart, Expr, LiteralValue, Node as AstNode, ObjectExpression, Program, UnaryOperator},
     walk::Node,
     SourceRange,
 };
@@ -27,7 +27,7 @@ use offsetPlane where possible.
 "
 );
 
-pub fn lint_should_be_offset_plane(node: Node) -> Result<Vec<Discovered>> {
+pub fn lint_should_be_offset_plane(node: Node, _prog: &AstNode<Program>) -> Result<Vec<Discovered>> {
     let Some((call_source_range, plane_name, offset)) = start_sketch_on_check_specific_plane(node)? else {
         return Ok(vec![]);
     };

rust/kcl-lib/src/lint/rule.rs

@@ -3,7 +3,13 @@ use schemars::JsonSchema;
 use serde::Serialize;
 use tower_lsp::lsp_types::{Diagnostic, DiagnosticSeverity};
 
-use crate::{errors::Suggestion, lsp::IntoDiagnostic, walk::Node, SourceRange};
+use crate::{
+    errors::Suggestion,
+    lsp::IntoDiagnostic,
+    parsing::ast::types::{Node as AstNode, Program},
+    walk::Node,
+    SourceRange,
+};
 
 /// Check the provided AST for any found rule violations.
 ///
@@ -11,15 +17,15 @@ use crate::{errors::Suggestion, lsp::IntoDiagnostic, walk::Node, SourceRange};
 /// but it can also be manually implemented as required.
 pub trait Rule<'a> {
     /// Check the AST at this specific node for any Finding(s).
-    fn check(&self, node: Node<'a>) -> Result<Vec<Discovered>>;
+    fn check(&self, node: Node<'a>, prog: &AstNode<Program>) -> Result<Vec<Discovered>>;
 }
 
 impl<'a, FnT> Rule<'a> for FnT
 where
-    FnT: Fn(Node<'a>) -> Result<Vec<Discovered>>,
+    FnT: Fn(Node<'a>, &AstNode<Program>) -> Result<Vec<Discovered>>,
 {
-    fn check(&self, n: Node<'a>) -> Result<Vec<Discovered>> {
-        self(n)
+    fn check(&self, n: Node<'a>, prog: &AstNode<Program>) -> Result<Vec<Discovered>> {
+        self(n, prog)
     }
 }
 

rust/kcl-lib/src/parsing/ast/types/mod.rs

@@ -312,7 +312,7 @@ impl Node<Program> {
         let v = Arc::new(Mutex::new(vec![]));
         crate::walk::walk(self, |node: crate::walk::Node<'a>| {
             let mut findings = v.lock().map_err(|_| anyhow::anyhow!("mutex"))?;
-            findings.append(&mut rule.check(node)?);
+            findings.append(&mut rule.check(node, self)?);
             Ok::<bool, anyhow::Error>(true)
         })?;
         let x = v.lock().unwrap();

src/editor/plugins/lsp/kcl/index.ts

@@ -81,6 +81,14 @@ export class KclPlugin implements PluginValue {
       } else if (tr.annotation(lspFormatCodeEvent.type)) {
         isRelevant = true
       }
+      // This is ON because the artifact graph and ast will be stale if we
+      // don't update the world.
+      // Also, then the file won't be saved.
+      else if (tr.annotation(lspRenameEvent.type)) {
+        isRelevant = true
+      } else if (tr.annotation(lspCodeActionEvent.type)) {
+        isRelevant = true
+      }
 
       // Don't make this an else.
       if (tr.annotation(codeManagerUpdateEvent.type)) {
@@ -95,16 +103,6 @@ export class KclPlugin implements PluginValue {
         // We want to ignore other events outside the editor.
         isRelevant = false
         break
-      } else if (tr.annotation(lspRenameEvent.type)) {
-        // Rename does not need to trigger the world.
-        // It's the same ast just different variable names.
-        isRelevant = false
-        break
-      } else if (tr.annotation(lspCodeActionEvent.type)) {
-        // Code actions should be stable enough where they create the same
-        // code and we do not need to need trigger an update.
-        isRelevant = false
-        break
       }
     }