UPDATE: Adds name validation and exposes it in wasm

We do a trick I am not proud of. Because all of our errors are Strings,
we don't have a way to separate a name error from an index error,
for instance. What I do in prepend the error with a string that indicates
where it comes from.

Author: nhatcher

base/src/expressions/utils/mod.rs

@@ -211,15 +211,19 @@ pub fn parse_reference_a1(r: &str) -> Option<ParsedReference> {
 pub fn is_valid_identifier(name: &str) -> bool {
     // https://support.microsoft.com/en-us/office/names-in-formulas-fc2935f9-115d-4bef-a370-3aa8bb4c91f1
     // https://github.com/MartinTrummer/excel-names/
-    // NOTE: We are being much more restrictive than Excel.
-    // In particular we do not support non ascii characters.
     let upper = name.to_ascii_uppercase();
-    let bytes = upper.as_bytes();
-    let len = bytes.len();
+    // length of chars
+    let len = upper.chars().count();
+
+    let mut chars = upper.chars();
+
     if len > 255 || len == 0 {
         return false;
     }
-    let first = bytes[0] as char;
+    let first = match chars.next() {
+        Some(ch) => ch,
+        None => return false,
+    };
     // The first character of a name must be a letter, an underscore character (_), or a backslash (\).
     if !(first.is_ascii_alphabetic() || first == '_' || first == '\\') {
         return false;
@@ -237,20 +241,10 @@ pub fn is_valid_identifier(name: &str) -> bool {
     if parse_reference_r1c1(name).is_some() {
         return false;
     }
-    let mut i = 1;
-    while i < len {
-        let ch = bytes[i] as char;
-        match ch {
-            'a'..='z' => {}
-            'A'..='Z' => {}
-            '0'..='9' => {}
-            '_' => {}
-            '.' => {}
-            _ => {
-                return false;
-            }
+    for ch in chars {
+        if !(ch.is_alphanumeric() || ch == '_' || ch == '.') {
+            return false;
         }
-        i += 1;
     }
 
     true

base/src/expressions/utils/test.rs

@@ -196,6 +196,7 @@ fn test_names() {
     assert!(is_valid_identifier("_."));
     assert!(is_valid_identifier("_1"));
     assert!(is_valid_identifier("\\."));
+    assert!(is_valid_identifier("truñe"));
 
     // invalid
     assert!(!is_valid_identifier("true"));
@@ -209,7 +210,6 @@ fn test_names() {
     assert!(!is_valid_identifier("1true"));
 
     assert!(!is_valid_identifier("test€"));
-    assert!(!is_valid_identifier("truñe"));
     assert!(!is_valid_identifier("tr&ue"));
 
     assert!(!is_valid_identifier("LOG10"));

base/src/model.rs

@@ -2068,29 +2068,57 @@ impl Model {
         scope: Option<u32>,
         formula: &str,
     ) -> Result<(), String> {
+        let sheet_id = self.is_valid_defined_name(name, scope, formula)?;
+        self.workbook.defined_names.push(DefinedName {
+            name: name.to_string(),
+            formula: formula.to_string(),
+            sheet_id,
+        });
+        self.reset_parsed_structures();
+
+        Ok(())
+    }
+
+    /// Validates if a defined name can be created
+    pub fn is_valid_defined_name(
+        &self,
+        name: &str,
+        scope: Option<u32>,
+        formula: &str,
+    ) -> Result<Option<u32>, String> {
         if !is_valid_identifier(name) {
-            return Err("Invalid defined name".to_string());
-        };
+            return Err("Name: Invalid defined name".to_string());
+        }
         let name_upper = name.to_uppercase();
         let defined_names = &self.workbook.defined_names;
         let sheet_id = match scope {
-            Some(index) => Some(self.workbook.worksheet(index)?.sheet_id),
+            Some(index) => match self.workbook.worksheet(index) {
+                Ok(ws) => Some(ws.sheet_id),
+                Err(_) => return Err("Scope: Invalid sheet index".to_string()),
+            },
             None => None,
         };
         // if the defined name already exist return error
         for df in defined_names {
             if df.name.to_uppercase() == name_upper && df.sheet_id == sheet_id {
-                return Err("Defined name already exists".to_string());
+                return Err("Name: Defined name already exists".to_string());
             }
         }
-        self.workbook.defined_names.push(DefinedName {
-            name: name.to_string(),
-            formula: formula.to_string(),
-            sheet_id,
-        });
-        self.reset_parsed_structures();
 
-        Ok(())
+        // Make sure the formula is valid
+        match common::ParsedReference::parse_reference_formula(
+            None,
+            formula,
+            &self.locale,
+            |name| self.get_sheet_index_by_name(name),
+        ) {
+            Ok(_) => {}
+            Err(_) => {
+                return Err("Formula: Invalid defined name formula".to_string());
+            }
+        };
+
+        Ok(sheet_id)
     }
 
     /// Delete defined name of name and scope
@@ -2126,26 +2154,36 @@ impl Model {
         new_formula: &str,
     ) -> Result<(), String> {
         if !is_valid_identifier(new_name) {
-            return Err("Invalid defined name".to_string());
+            return Err("Name: Invalid defined name".to_string());
         };
         let name_upper = name.to_uppercase();
         let new_name_upper = new_name.to_uppercase();
 
         if name_upper != new_name_upper || scope != new_scope {
             for key in self.parsed_defined_names.keys() {
                 if key.1.to_uppercase() == new_name_upper && key.0 == new_scope {
-                    return Err("Defined name already exists".to_string());
+                    return Err("Name: Defined name already exists".to_string());
                 }
             }
         }
         let defined_names = &self.workbook.defined_names;
         let sheet_id = match scope {
-            Some(index) => Some(self.workbook.worksheet(index)?.sheet_id),
+            Some(index) => Some(
+                self.workbook
+                    .worksheet(index)
+                    .map_err(|_| "Scope: Invalid sheet index")?
+                    .sheet_id,
+            ),
             None => None,
         };
 
         let new_sheet_id = match new_scope {
-            Some(index) => Some(self.workbook.worksheet(index)?.sheet_id),
+            Some(index) => Some(
+                self.workbook
+                    .worksheet(index)
+                    .map_err(|_| "Scope: Invalid sheet index")?
+                    .sheet_id,
+            ),
             None => None,
         };
 

base/src/test/user_model/test_defined_names.rs

@@ -254,19 +254,19 @@ fn invalid_names() {
     // spaces
     assert_eq!(
         model.new_defined_name("A real", None, "Sheet1!$A$1"),
-        Err("Invalid defined name".to_string())
+        Err("Name: Invalid defined name".to_string())
     );
 
     // Starts with number
     assert_eq!(
         model.new_defined_name("2real", None, "Sheet1!$A$1"),
-        Err("Invalid defined name".to_string())
+        Err("Name: Invalid defined name".to_string())
     );
 
     // Updating also fails
     assert_eq!(
         model.update_defined_name("MyName", None, "My Name", None, "Sheet1!$A$1"),
-        Err("Invalid defined name".to_string())
+        Err("Name: Invalid defined name".to_string())
     );
 }
 
@@ -284,13 +284,13 @@ fn already_existing() {
     // Can't create a new name with the same name
     assert_eq!(
         model.new_defined_name("MyName", None, "Sheet1!$A$2"),
-        Err("Defined name already exists".to_string())
+        Err("Name: Defined name already exists".to_string())
     );
 
     // Can't update one into an existing
     assert_eq!(
         model.update_defined_name("Another", None, "MyName", None, "Sheet1!$A$1"),
-        Err("Defined name already exists".to_string())
+        Err("Name: Defined name already exists".to_string())
     );
 }
 
@@ -304,25 +304,25 @@ fn invalid_sheet() {
 
     assert_eq!(
         model.new_defined_name("Mything", Some(2), "Sheet1!$A$1"),
-        Err("Invalid sheet index".to_string())
+        Err("Scope: Invalid sheet index".to_string())
     );
 
     assert_eq!(
         model.update_defined_name("MyName", None, "MyName", Some(2), "Sheet1!$A$1"),
-        Err("Invalid sheet index".to_string())
+        Err("Scope: Invalid sheet index".to_string())
     );
 
     assert_eq!(
         model.update_defined_name("MyName", Some(9), "YourName", None, "Sheet1!$A$1"),
-        Err("Invalid sheet index".to_string())
+        Err("General: Failed to get old name".to_string())
     );
 }
 
 #[test]
 fn invalid_formula() {
     let mut model = UserModel::new_empty("model", "en", "UTC").unwrap();
     model.set_user_input(0, 1, 1, "Hello").unwrap();
-    model.new_defined_name("MyName", None, "A1").unwrap();
+    assert!(model.new_defined_name("MyName", None, "A1").is_err());
 
     model.set_user_input(0, 1, 2, "=MyName").unwrap();
 

base/src/user_model/common.rs

@@ -2001,7 +2001,10 @@ impl UserModel {
         new_scope: Option<u32>,
         new_formula: &str,
     ) -> Result<(), String> {
-        let old_formula = self.model.get_defined_name_formula(name, scope)?;
+        let old_formula = self
+            .model
+            .get_defined_name_formula(name, scope)
+            .map_err(|_| "General: Failed to get old name")?;
         let diff_list = vec![Diff::UpdateDefinedName {
             name: name.to_string(),
             scope,
@@ -2017,6 +2020,16 @@ impl UserModel {
         Ok(())
     }
 
+    /// validates a new defined name
+    pub fn is_valid_defined_name(
+        &self,
+        name: &str,
+        scope: Option<u32>,
+        formula: &str,
+    ) -> Result<Option<u32>, String> {
+        self.model.is_valid_defined_name(name, scope, formula)
+    }
+
     // **** Private methods ****** //
 
     pub(crate) fn push_diff_list(&mut self, diff_list: DiffList) {

bindings/wasm/src/lib.rs

@@ -775,4 +775,17 @@ impl Model {
             .get_first_non_empty_in_row_after_column(sheet, row, column)
             .map_err(to_js_error)
     }
+
+    #[wasm_bindgen(js_name = "isValidDefinedName")]
+    pub fn is_valid_defined_name(
+        &self,
+        name: &str,
+        scope: Option<u32>,
+        formula: &str,
+    ) -> Result<(), JsError> {
+        match self.model.is_valid_defined_name(name, scope, formula) {
+            Ok(_) => Ok(()),
+            Err(e) => Err(to_js_error(e.to_string())),
+        }
+    }
 }

webapp/IronCalc/src/components/RightDrawer/NamedRanges/EditNamedRange.tsx

@@ -83,7 +83,7 @@ function EditNamedRange({
     } else {
       // Check for duplicates only if format is valid
       const scopeIndex = worksheets.findIndex((s) => s.name === scope);
-      const newScope = scopeIndex >= 0 ? scopeIndex : null;
+      const newScope = scopeIndex >= 0 ? scopeIndex : undefined;
       const existing = definedNameList.find(
         (dn) =>
           dn.name === trimmed &&
@@ -99,6 +99,7 @@ function EditNamedRange({
     }
 
     setNameError(error);
+    setFormulaError("");
   }, [name, scope, definedNameList, editingDefinedName, worksheets]);
 
   const hasAnyError = nameError !== "" || formulaError !== "";