compiler: Fix identifier normalization function

`__1` is a valid identifier, which we normalized to
`--1`, which is invalid.

This changes the nromalization function to leave a `_` in the first position.

api/node/typescript/index.ts

@@ -461,8 +461,15 @@ function loadSlint(loadData: LoadData): Object {
 
                 // globals
                 instance!.definition().globals.forEach((globalName) => {
-                    if (componentHandle[globalName] !== undefined) {
+                    const jsName = translateName(globalName);
-                        console.warn("Duplicated property name " + globalName);
+                    if (componentHandle[jsName] !== undefined) {
+                        console.warn(
+                            "Duplicated property name " +
+                                globalName +
+                                " (In JS: " +
+                                jsName +
+                                ")",
+                        );
                     } else {
                         const globalObject = Object.create({});
 
@@ -576,7 +583,7 @@ function loadSlint(loadData: LoadData): Object {
                                 }
                             });
 
-                        Object.defineProperty(componentHandle, globalName, {
+                        Object.defineProperty(componentHandle, jsName, {
                             get() {
                                 return globalObject;
                             },

internal/compiler/parser.rs

@@ -13,7 +13,7 @@ This module has different sub modules with the actual parser functions
 */
 
 use crate::diagnostics::{BuildDiagnostics, SourceFile, Spanned};
-use smol_str::{SmolStr, StrExt};
+use smol_str::SmolStr;
 use std::fmt::Display;
 
 mod document;
@@ -1013,7 +1013,29 @@ pub fn identifier_text(node: &SyntaxNode) -> Option<SmolStr> {
 }
 
 pub fn normalize_identifier(ident: &str) -> SmolStr {
-    ident.replace_smolstr("_", "-")
+    let mut builder = smol_str::SmolStrBuilder::default();
+    for (pos, c) in ident.chars().enumerate() {
+        match (pos, c) {
+            (0, '-') | (0, '_') => builder.push('_'),
+            (_, '_') => builder.push('-'),
+            (_, c) => builder.push(c),
+        }
+    }
+    builder.finish()
+}
+
+#[test]
+fn test_normalize_identifier() {
+    assert_eq!(normalize_identifier("true"), SmolStr::new("true"));
+    assert_eq!(normalize_identifier("foo_bar"), SmolStr::new("foo-bar"));
+    assert_eq!(normalize_identifier("-foo_bar"), SmolStr::new("_foo-bar"));
+    assert_eq!(normalize_identifier("-foo-bar"), SmolStr::new("_foo-bar"));
+    assert_eq!(normalize_identifier("foo_bar_"), SmolStr::new("foo-bar-"));
+    assert_eq!(normalize_identifier("foo_bar-"), SmolStr::new("foo-bar-"));
+    assert_eq!(normalize_identifier("_foo_bar_"), SmolStr::new("_foo-bar-"));
+    assert_eq!(normalize_identifier("__1"), SmolStr::new("_-1"));
+    assert_eq!(normalize_identifier("--1"), SmolStr::new("_-1"));
+    assert_eq!(normalize_identifier("--1--"), SmolStr::new("_-1--"));
 }
 
 // Actual parser

internal/interpreter/api.rs

@@ -10,8 +10,7 @@ use i_slint_core::model::{Model, ModelExt, ModelRc};
 #[cfg(feature = "internal")]
 use i_slint_core::window::WindowInner;
 use i_slint_core::{PathData, SharedVector};
-use smol_str::{SmolStr, StrExt};
+use smol_str::SmolStr;
-use std::borrow::Cow;
 use std::collections::HashMap;
 use std::future::Future;
 use std::path::{Path, PathBuf};
@@ -341,7 +340,7 @@ macro_rules! declare_value_enum_conversion {
             fn from(v: i_slint_core::items::$Name) -> Self {
                 Value::EnumerationValue(
                     stringify!($Name).to_owned(),
-                    v.to_string().trim_start_matches("r#").replace('_', "-"),
+                    i_slint_compiler::parser::normalize_identifier(v.to_string().trim_start_matches("r#")).to_string(),
                 )
             }
         }
@@ -357,7 +356,7 @@ macro_rules! declare_value_enum_conversion {
 
                         <i_slint_core::items::$Name>::from_str(value.as_str())
                             .or_else(|_| {
-                                let norm = value.as_str().replace('-', "_");
+                                let norm = i_slint_compiler::parser::normalize_identifier(value.as_str());
                                 <i_slint_core::items::$Name>::from_str(&norm)
                                     .or_else(|_| <i_slint_core::items::$Name>::from_str(&format!("r#{}", norm)))
                             })
@@ -487,21 +486,8 @@ fn value_model_conversion() {
     assert!(err.is_err());
 }
 
-/// Normalize the identifier to use dashes
+pub(crate) fn normalize_identifier(ident: &str) -> SmolStr {
-pub(crate) fn normalize_identifier(ident: &str) -> Cow<'_, str> {
+    i_slint_compiler::parser::normalize_identifier(ident)
-    if ident.contains('_') {
-        ident.replace('_', "-").into()
-    } else {
-        ident.into()
-    }
-}
-
-pub(crate) fn normalize_identifier_smolstr(ident: &str) -> SmolStr {
-    if ident.contains('_') {
-        ident.replace_smolstr("_", "-")
-    } else {
-        ident.into()
-    }
 }
 
 /// This type represents a runtime instance of structure in `.slint`.
@@ -526,7 +512,7 @@ pub(crate) fn normalize_identifier_smolstr(ident: &str) -> SmolStr {
 /// assert_eq!(s.get_field("foo").cloned().unwrap().try_into(), Ok(45u32));
 /// ```
 #[derive(Clone, PartialEq, Debug, Default)]
-pub struct Struct(pub(crate) HashMap<String, Value>);
+pub struct Struct(pub(crate) HashMap<SmolStr, Value>);
 impl Struct {
     /// Get the value for a given struct field
     pub fn get_field(&self, name: &str) -> Option<&Value> {
@@ -534,11 +520,7 @@ impl Struct {
     }
     /// Set the value of a given struct field
     pub fn set_field(&mut self, name: String, value: Value) {
-        if name.contains('_') {
+        self.0.insert(normalize_identifier(&name), value);
-            self.0.insert(name.replace('_', "-"), value);
-        } else {
-            self.0.insert(name, value);
-        }
     }
 
     /// Iterate over all the fields in this struct
@@ -549,11 +531,7 @@ impl Struct {
 
 impl FromIterator<(String, Value)> for Struct {
     fn from_iter<T: IntoIterator<Item = (String, Value)>>(iter: T) -> Self {
-        Self(
+        Self(iter.into_iter().map(|(s, v)| (normalize_identifier(&s), v)).collect())
-            iter.into_iter()
-                .map(|(s, v)| (if s.contains('_') { s.replace('_', "-") } else { s }, v))
-                .collect(),
-        )
     }
 }
 
@@ -1258,7 +1236,7 @@ impl ComponentInstance {
             .root_element
             .borrow()
             .property_declarations
-            .get(name.as_ref())
+            .get(&name)
             .is_none_or(|d| !d.expose_in_public_api)
         {
             return Err(GetPropertyError::NoSuchProperty);
@@ -1276,10 +1254,7 @@ impl ComponentInstance {
         let comp = self.inner.unerase(guard);
         let d = comp.description();
         let elem = d.original.root_element.borrow();
-        let decl = elem
+        let decl = elem.property_declarations.get(&name).ok_or(SetPropertyError::NoSuchProperty)?;
-            .property_declarations
-            .get(name.as_ref())
-            .ok_or(SetPropertyError::NoSuchProperty)?;
 
         if !decl.expose_in_public_api {
             return Err(SetPropertyError::NoSuchProperty);
@@ -1344,7 +1319,7 @@ impl ComponentInstance {
         generativity::make_guard!(guard);
         let comp = self.inner.unerase(guard);
         comp.description()
-            .invoke(comp.borrow(), &normalize_identifier_smolstr(name), args)
+            .invoke(comp.borrow(), &normalize_identifier(name), args)
             .map_err(|()| InvokeError::NoSuchCallable)
     }
 
@@ -1380,7 +1355,7 @@ impl ComponentInstance {
         generativity::make_guard!(guard);
         let comp = self.inner.unerase(guard);
         comp.description()
-            .get_global(comp.borrow(), &normalize_identifier(global))
+            .get_global(comp.borrow(), &&normalize_identifier(global))
             .map_err(|()| GetPropertyError::NoSuchProperty)? // FIXME: should there be a NoSuchGlobal error?
             .as_ref()
             .get_property(&normalize_identifier(property))
@@ -1397,10 +1372,10 @@ impl ComponentInstance {
         generativity::make_guard!(guard);
         let comp = self.inner.unerase(guard);
         comp.description()
-            .get_global(comp.borrow(), &normalize_identifier(global))
+            .get_global(comp.borrow(), &&normalize_identifier(global))
             .map_err(|()| SetPropertyError::NoSuchProperty)? // FIXME: should there be a NoSuchGlobal error?
             .as_ref()
-            .set_property(&normalize_identifier(property), value)
+            .set_property(&&normalize_identifier(property), value)
     }
 
     /// Set a handler for the callback in the exported global singleton. A callback with that
@@ -1469,7 +1444,7 @@ impl ComponentInstance {
             .description()
             .get_global(comp.borrow(), &normalize_identifier(global))
             .map_err(|()| InvokeError::NoSuchCallable)?; // FIXME: should there be a NoSuchGlobal error?
-        let callable_name = normalize_identifier_smolstr(callable_name);
+        let callable_name = normalize_identifier(callable_name);
         if matches!(
             comp.description()
                 .original

internal/interpreter/dynamic_item_tree.rs

@@ -510,7 +510,7 @@ impl ItemTreeDescription<'_> {
     > {
         let g = self.compiled_globals.as_ref().expect("Root component should have globals");
         g.exported_globals_by_name
-            .get(crate::normalize_identifier(name).as_ref())
+            .get(&crate::normalize_identifier(name))
             .and_then(|global_idx| g.compiled_globals.get(*global_idx))
             .map(|global| internal_properties_to_public(global.public_properties()))
     }

internal/interpreter/ffi.rs

@@ -227,7 +227,7 @@ pub extern "C" fn slint_interpreter_struct_set_field<'a>(
     stru.as_struct_mut().set_field(std::str::from_utf8(&name).unwrap().into(), value.clone())
 }
 
-type StructIterator<'a> = std::collections::hash_map::Iter<'a, String, Value>;
+type StructIterator<'a> = std::collections::hash_map::Iter<'a, SmolStr, Value>;
 #[repr(C)]
 pub struct StructIteratorOpaque<'a>([usize; 5], std::marker::PhantomData<StructIterator<'a>>);
 const _: [(); std::mem::size_of::<StructIteratorOpaque>()] =
@@ -313,7 +313,7 @@ pub unsafe extern "C" fn slint_interpreter_component_instance_invoke(
     let comp = inst.unerase(guard);
     match comp.description().invoke(
         comp.borrow(),
-        &normalize_identifier_smolstr(std::str::from_utf8(&name).unwrap()),
+        &normalize_identifier(std::str::from_utf8(&name).unwrap()),
         args.as_slice(),
     ) {
         Ok(val) => Box::into_raw(Box::new(val)),
@@ -467,8 +467,7 @@ pub unsafe extern "C" fn slint_interpreter_component_instance_invoke_global(
                     args.as_slice().iter().cloned().collect(),
                 )
             } else {
-                g.as_ref()
+                g.as_ref().invoke_callback(&normalize_identifier(callable_name), args.as_slice())
-                    .invoke_callback(&normalize_identifier_smolstr(callable_name), args.as_slice())
             }
         }) {
         Ok(val) => Box::into_raw(Box::new(val)),

internal/interpreter/json.rs

@@ -184,13 +184,13 @@ pub fn value_from_json(t: &langtype::Type, v: &serde_json::Value) -> Result<Valu
             langtype::Type::Struct(s) => Ok(crate::Struct(
                 obj.iter()
                     .map(|(k, v)| {
-                        let k = k.to_smolstr();
+                        let k = crate::api::normalize_identifier(k);
                         match s.fields.get(&k) {
-                            Some(t) => value_from_json(t, v).map(|v| (k.to_string(), v)),
+                            Some(t) => value_from_json(t, v).map(|v| (k, v)),
                             None => Err(format!("Found unknown field in struct: {k}")),
                         }
                     })
-                    .collect::<Result<HashMap<String, Value>, _>>()?,
+                    .collect::<Result<HashMap<smol_str::SmolStr, Value>, _>>()?,
             )
             .into()),
             _ => Err("Got a struct where none was expected".into()),
@@ -227,7 +227,7 @@ pub fn value_to_json(value: &Value) -> Result<serde_json::Value, String> {
 
         gradient += ")";
 
-        serde_json::Value::String(gradient.into())
+        serde_json::Value::String(gradient)
     }
 
     match value {

tests/cases/properties/dashes.slint

@@ -1,11 +1,22 @@
 // Copyright © SixtyFPS GmbH <info@slint.dev>
 // SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
 
- Test-Case := Rectangle {
+struct __strange- {
+    _-indeed--: int,
+}
 
+export global _-Foo-- {
+    out property<int> _-foo-: 815;
+}
+
+Test-Case := Rectangle {
     property<length> property-x1: xxx-foo.border-width;
     property<length> property-x2: xxx_foo.border_width;
 
+    __-id- := Rectangle {
+        property<_-strange-> struct-property: { _-indeed--: 23 };
+    }
+
     xxx-foo := Rectangle {
         border-width: 42phx;
     }
@@ -14,6 +25,8 @@
     property<int> hello--world: -hello-42 - 2; // -42 - 2 = -44
     property<int> this--has-6-slashes--: 42-hello--world; // 42 - -44  = 86
 
+    out property<int> _-test_property-: 42;
+    out property<int> __test_property2: _--id_.struct-property._-indeed--;
 }
 /*
 ```cpp
@@ -22,6 +35,10 @@ const Test_Case &instance = *handle;
 assert_eq(instance.get_property_x1(), 42);
 assert_eq(instance.get_property_x2(), 42);
 assert_eq(instance.get_this__has_6_slashes__(), 86);
+assert_eq(instance.get___test_property_(), 42);
+assert_eq(instance.get___test_property2(), 23);
+
+assert_eq(handle->global<__Foo__>().get___foo_(), 815);
 ```
 
 ```rust
@@ -29,6 +46,11 @@ let instance = Test_Case::new().unwrap();
 assert_eq!(instance.get_property_x1(), 42.);
 assert_eq!(instance.get_property_x2(), 42.);
 assert_eq!(instance.get_this__has_6_slashes__(), 86);
+assert_eq!(instance.get___test_property_(), 42);
+assert_eq!(instance.get___test_property2(), 23);
+
+let foo = instance.global::<__Foo__<'_>>();
+assert_eq!(foo.get___foo_(), 815);
 ```
 
 ```js
@@ -36,5 +58,9 @@ var instance = new slint.Test_Case({});
 assert.equal(instance.property_x1, 42);
 assert.equal(instance.property_x2, 42);
 assert.equal(instance.this__has_6_slashes__, 86);
+assert.equal(instance.__test_property_, 42);
+assert.equal(instance.__test_property2, 23);
+
+assert.equal(instance.__Foo__.__foo_, 815);
 ```
 */