[PATCH v6 0/4] MR10960: scrrun/dictionary: Match native semantics for Empty and Boolean keys.

[Francis De Brabandere (＠francisdb)]

An uninitialized VBScript variable is Empty, so scripts can end up using
Empty as a dictionary key alongside a numeric 0 or "". Native treats
those as the same key; Wine kept Empty distinct from every other type.
Match an Empty key against any numeric zero or the empty string, while
keeping it distinct from Null and from non-zero/non-empty keys.

A Boolean key was rejected outright (E_NOTIMPL), breaking Exists/Remove when a script uses a comparison result as a key; hash it by its numeric value like the other numeric types.

--
  v6: scrrun/dictionary: Match native key semantics for Boolean keys.
      scrrun/tests: Add tests for Boolean dictionary keys.
      scrrun/dictionary: Match Empty keys against zero-valued and empty-string keys.
      scrrun/tests: Add tests for Empty dictionary keys.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10960

[PATCH v6 1/4] scrrun/tests: Add tests for Empty dictionary keys.

[Francis De Brabandere]

From: Francis De Brabandere <[e-mail]>

An Empty key matches the zero/default value of the other key's type: any
numeric zero or the empty string, but not a non-zero number, a non-empty
string, or Null.
---
 dlls/scrrun/tests/dictionary.c | 107 +++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/dlls/scrrun/tests/dictionary.c b/dlls/scrrun/tests/dictionary.c
index 8fc369e7062..8878b528381 100644
--- a/dlls/scrrun/tests/dictionary.c
+++ b/dlls/scrrun/tests/dictionary.c
@@ -1134,6 +1134,112 @@ static void test_Add(void)
     IDictionary_Release(dict);
 }
 
+/* Add two keys to a fresh dictionary; returns TRUE if the second key is
+ * treated as a duplicate of the first. */
+static BOOL keys_match(VARIANT *key1, VARIANT *key2)
+{
+    IDictionary *dict;
+    VARIANT item;
+    HRESULT hr;
+
+    if (FAILED(CoCreateInstance(&CLSID_Dictionary, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
+            &IID_IDictionary, (void**)&dict)))
+        return FALSE;
+
+    V_VT(&item) = VT_I2;
+    V_I2(&item) = 1;
+    if (FAILED(IDictionary_Add(dict, key1, &item)))
+    {
+        IDictionary_Release(dict);
+        return FALSE;
+    }
+
+    hr = IDictionary_Add(dict, key2, &item);
+    IDictionary_Release(dict);
+    return hr == CTL_E_KEY_ALREADY_EXISTS;
+}
+
+static void test_empty_key(void)
+{
+    static const VARTYPE zero_keys[] = {
+        VT_I2, VT_I4, VT_UI1, VT_R4, VT_R8, VT_DATE,
+    };
+    VARIANT_BOOL exists;
+    IDictionary *dict;
+    VARIANT a, b, item;
+    HRESULT hr;
+    unsigned i;
+
+    /* Empty matches the zero/default value of any numeric type. */
+    for (i = 0; i < ARRAY_SIZE(zero_keys); i++)
+    {
+        V_VT(&a) = VT_EMPTY;
+        V_VT(&b) = zero_keys[i];
+        if (zero_keys[i] == VT_R8 || zero_keys[i] == VT_DATE)
+            V_R8(&b) = 0.0;
+        else if (zero_keys[i] == VT_R4)
+            V_R4(&b) = 0.0f;
+        else
+            V_I4(&b) = 0;
+        todo_wine ok(keys_match(&a, &b), "Empty should match zero key vt %d\n", zero_keys[i]);
+    }
+
+    /* Empty matches the empty string, but not a non-empty one. */
+    V_VT(&a) = VT_EMPTY;
+    V_VT(&b) = VT_BSTR;
+    V_BSTR(&b) = SysAllocString(L"");
+    todo_wine ok(keys_match(&a, &b), "Empty should match empty string\n");
+    VariantClear(&b);
+
+    V_VT(&b) = VT_BSTR;
+    V_BSTR(&b) = SysAllocString(L"0");
+    ok(!keys_match(&a, &b), "Empty should not match \"0\"\n");
+    VariantClear(&b);
+
+    /* Empty does not match a non-zero number or Null. */
+    V_VT(&b) = VT_I2;
+    V_I2(&b) = 5;
+    ok(!keys_match(&a, &b), "Empty should not match I2 5\n");
+
+    V_VT(&b) = VT_NULL;
+    ok(!keys_match(&a, &b), "Empty should not match Null\n");
+
+    /* A numeric zero does not match the empty string (only Empty bridges). */
+    V_VT(&a) = VT_I2;
+    V_I2(&a) = 0;
+    V_VT(&b) = VT_BSTR;
+    V_BSTR(&b) = SysAllocString(L"");
+    ok(!keys_match(&a, &b), "I2 0 should not match empty string\n");
+    VariantClear(&b);
+
+    /* Null matches only Null. */
+    V_VT(&a) = VT_NULL;
+    V_VT(&b) = VT_NULL;
+    ok(keys_match(&a, &b), "Null should match Null\n");
+
+    /* Exists and Remove accept an Empty key, matching a zero-valued entry. */
+    hr = CoCreateInstance(&CLSID_Dictionary, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
+            &IID_IDictionary, (void**)&dict);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    V_VT(&a) = VT_I2;
+    V_I2(&a) = 0;
+    VariantInit(&item);
+    hr = IDictionary_Add(dict, &a, &item);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    V_VT(&b) = VT_EMPTY;
+    exists = VARIANT_FALSE;
+    hr = IDictionary_Exists(dict, &b, &exists);
+    ok(hr == S_OK, "Exists with Empty key: %#lx.\n", hr);
+    todo_wine ok(exists == VARIANT_TRUE, "Empty should find I2 0, got %x\n", exists);
+
+    hr = IDictionary_Remove(dict, &b);
+    todo_wine ok(hr == S_OK, "Remove with Empty key: %#lx.\n", hr);
+
+    IDictionary_Release(dict);
+}
+
 static void test_IEnumVARIANT(void)
 {
     IUnknown *enum1, *enum2;
@@ -1315,6 +1421,7 @@ START_TEST(dictionary)
     test_Item();
     test_Add();
     test_object_key_hashfail();
+    test_empty_key();
     test_IEnumVARIANT();
     test_putref_Item();
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/10960

[PATCH v6 2/4] scrrun/dictionary: Match Empty keys against zero-valued and empty-string keys.

[Francis De Brabandere]

From: Francis De Brabandere <[e-mail]>

An uninitialized VBScript variable is Empty, so scripts can end up using
Empty as a dictionary key alongside a numeric 0 or "". Native treats
those as the same key; Wine kept Empty distinct from every other type.
Match an Empty key against any numeric zero or the empty string, while
keeping it distinct from Null and from non-zero/non-empty keys.
---
 dlls/scrrun/dictionary.c       | 35 +++++++++++++++++++++++++++++++---
 dlls/scrrun/tests/dictionary.c |  8 ++++----
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/dlls/scrrun/dictionary.c b/dlls/scrrun/dictionary.c
index 1b618865360..8137855b592 100644
--- a/dlls/scrrun/dictionary.c
+++ b/dlls/scrrun/dictionary.c
@@ -162,9 +162,38 @@ static inline BOOL numeric_key_eq(const VARIANT *key1, const VARIANT *key2)
     return V_R4(&v1) == V_R4(&v2);
 }
 
+/* Empty matches the zero/default value of the other key's type: any numeric
+ * zero, the empty string, or another Empty. */
+static BOOL empty_matches_key(const VARIANT *key)
+{
+    VARIANT v;
+
+    switch (V_VT(key))
+    {
+        case VT_EMPTY:
+            return TRUE;
+        case VT_BSTR:
+        {
+            const WCHAR *str = get_key_strptr(key);
+            return !str || !*str;
+        }
+        default:
+            if (!is_numeric_key(key))
+                return FALSE;
+            VariantInit(&v);
+            if (FAILED(VariantChangeType(&v, key, 0, VT_R4)))
+                return FALSE;
+            return V_R4(&v) == 0.0f;
+    }
+}
+
 static BOOL is_matching_key(const struct dictionary *dict, const struct keyitem_pair *pair, const VARIANT *key, DWORD hash)
 {
-    if (is_string_key(key) != is_string_key(&pair->key))
+    if (V_VT(key) == VT_EMPTY || V_VT(&pair->key) == VT_EMPTY)
+    {
+        return empty_matches_key(V_VT(key) == VT_EMPTY ? &pair->key : key);
+    }
+    else if (is_string_key(key) != is_string_key(&pair->key))
     {
         return FALSE;
     }
@@ -188,9 +217,9 @@ static BOOL is_matching_key(const struct dictionary *dict, const struct keyitem_
     {
         return hash == pair->hash && numeric_key_eq(key, &pair->key);
     }
-    else if (V_VT(&pair->key) == VT_EMPTY || V_VT(&pair->key) == VT_NULL)
+    else if (V_VT(&pair->key) == VT_NULL)
     {
-        return V_VT(&pair->key) == V_VT(key);
+        return V_VT(key) == VT_NULL;
     }
     else
     {
diff --git a/dlls/scrrun/tests/dictionary.c b/dlls/scrrun/tests/dictionary.c
index 8878b528381..bc5fbd878a3 100644
--- a/dlls/scrrun/tests/dictionary.c
+++ b/dlls/scrrun/tests/dictionary.c
@@ -1181,14 +1181,14 @@ static void test_empty_key(void)
             V_R4(&b) = 0.0f;
         else
             V_I4(&b) = 0;
-        todo_wine ok(keys_match(&a, &b), "Empty should match zero key vt %d\n", zero_keys[i]);
+        ok(keys_match(&a, &b), "Empty should match zero key vt %d\n", zero_keys[i]);
     }
 
     /* Empty matches the empty string, but not a non-empty one. */
     V_VT(&a) = VT_EMPTY;
     V_VT(&b) = VT_BSTR;
     V_BSTR(&b) = SysAllocString(L"");
-    todo_wine ok(keys_match(&a, &b), "Empty should match empty string\n");
+    ok(keys_match(&a, &b), "Empty should match empty string\n");
     VariantClear(&b);
 
     V_VT(&b) = VT_BSTR;
@@ -1232,10 +1232,10 @@ static void test_empty_key(void)
     exists = VARIANT_FALSE;
     hr = IDictionary_Exists(dict, &b, &exists);
     ok(hr == S_OK, "Exists with Empty key: %#lx.\n", hr);
-    todo_wine ok(exists == VARIANT_TRUE, "Empty should find I2 0, got %x\n", exists);
+    ok(exists == VARIANT_TRUE, "Empty should find I2 0, got %x\n", exists);
 
     hr = IDictionary_Remove(dict, &b);
-    todo_wine ok(hr == S_OK, "Remove with Empty key: %#lx.\n", hr);
+    ok(hr == S_OK, "Remove with Empty key: %#lx.\n", hr);
 
     IDictionary_Release(dict);
 }
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/10960

[PATCH v6 3/4] scrrun/tests: Add tests for Boolean dictionary keys.

[Francis De Brabandere]

From: Francis De Brabandere <[e-mail]>

A Boolean key hashes by its numeric value and matches the equal numeric
value across types (True is -1, False is 0); False is the zero/default
value, so it matches an Empty key. A script can use a comparison result
(a Boolean) as the key to Exists and Remove, which must not fail.
---
 dlls/scrrun/tests/dictionary.c | 76 ++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/dlls/scrrun/tests/dictionary.c b/dlls/scrrun/tests/dictionary.c
index bc5fbd878a3..8da22a24101 100644
--- a/dlls/scrrun/tests/dictionary.c
+++ b/dlls/scrrun/tests/dictionary.c
@@ -828,6 +828,22 @@ if (0) { /* crashes on native */
     ok(V_VT(&hash) == VT_I4, "got %d\n", V_VT(&hash));
     ok(V_I4(&hash) == expected, "got hash %#lx, expected %#lx\n", V_I4(&hash), expected);
 
+    V_VT(&key) = VT_BOOL;
+    V_BOOL(&key) = VARIANT_FALSE;
+    V_I4(&hash) = 5678;
+    hr = IDictionary_get_HashVal(dict, &key, &hash);
+    todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(V_VT(&hash) == VT_I4, "Unexpected hash type %d.\n", V_VT(&hash));
+    todo_wine ok(V_I4(&hash) == get_num_hash(0.0f), "Unexpected hash value %#lx.\n", V_I4(&hash));
+
+    V_VT(&key) = VT_BOOL;
+    V_BOOL(&key) = VARIANT_TRUE;
+    V_I4(&hash) = 5678;
+    hr = IDictionary_get_HashVal(dict, &key, &hash);
+    todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(V_VT(&hash) == VT_I4, "Unexpected hash type %d.\n", V_VT(&hash));
+    todo_wine ok(V_I4(&hash) == get_num_hash((FLOAT)VARIANT_TRUE), "Unexpected hash value %#lx.\n", V_I4(&hash));
+
     V_VT(&key) = VT_EMPTY;
     V_I4(&key) = 1234;
     V_I4(&hash) = 5678;
@@ -1240,6 +1256,65 @@ static void test_empty_key(void)
     IDictionary_Release(dict);
 }
 
+static void test_bool_key(void)
+{
+    VARIANT_BOOL exists;
+    IDictionary *dict;
+    VARIANT a, b, item;
+    HRESULT hr;
+
+    /* Boolean keys match the equal numeric value across types: True is -1,
+     * False is 0. */
+    V_VT(&a) = VT_BOOL;
+    V_BOOL(&a) = VARIANT_TRUE;
+    V_VT(&b) = VT_I2;
+    V_I2(&b) = -1;
+    todo_wine ok(keys_match(&a, &b), "True should match I2 -1\n");
+
+    V_VT(&a) = VT_BOOL;
+    V_BOOL(&a) = VARIANT_FALSE;
+    V_VT(&b) = VT_I2;
+    V_I2(&b) = 0;
+    todo_wine ok(keys_match(&a, &b), "False should match I2 0\n");
+
+    /* False is the zero/default value, so it matches an Empty key. */
+    V_VT(&a) = VT_BOOL;
+    V_BOOL(&a) = VARIANT_FALSE;
+    V_VT(&b) = VT_EMPTY;
+    todo_wine ok(keys_match(&a, &b), "False should match Empty\n");
+
+    /* True and False are distinct keys. */
+    V_VT(&a) = VT_BOOL;
+    V_BOOL(&a) = VARIANT_TRUE;
+    V_VT(&b) = VT_BOOL;
+    V_BOOL(&b) = VARIANT_FALSE;
+    ok(!keys_match(&a, &b), "True should not match False\n");
+
+    /* A script can use a comparison result (a Boolean) as the key to Exists
+     * and Remove. */
+    hr = CoCreateInstance(&CLSID_Dictionary, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
+            &IID_IDictionary, (void**)&dict);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    V_VT(&a) = VT_I2;
+    V_I2(&a) = -1;
+    VariantInit(&item);
+    hr = IDictionary_Add(dict, &a, &item);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+    V_VT(&b) = VT_BOOL;
+    V_BOOL(&b) = VARIANT_TRUE;
+    exists = VARIANT_FALSE;
+    hr = IDictionary_Exists(dict, &b, &exists);
+    todo_wine ok(hr == S_OK, "Exists with Boolean key: %#lx.\n", hr);
+    todo_wine ok(exists == VARIANT_TRUE, "True should find I2 -1, got %x\n", exists);
+
+    hr = IDictionary_Remove(dict, &b);
+    todo_wine ok(hr == S_OK, "Remove with Boolean key: %#lx.\n", hr);
+
+    IDictionary_Release(dict);
+}
+
 static void test_IEnumVARIANT(void)
 {
     IUnknown *enum1, *enum2;
@@ -1422,6 +1497,7 @@ START_TEST(dictionary)
     test_Add();
     test_object_key_hashfail();
     test_empty_key();
+    test_bool_key();
     test_IEnumVARIANT();
     test_putref_Item();
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/10960

[PATCH v6 4/4] scrrun/dictionary: Match native key semantics for Boolean keys.

[Francis De Brabandere]

From: Francis De Brabandere <[e-mail]>

Boolean keys were rejected outright by the hash function, returning
E_NOTIMPL, so Exists and Remove failed for them. An uninitialized or
comparison-result VBScript value can be a Boolean, so a script can end up
using one as a dictionary key. Hash it by its numeric value like the
other numeric types, so True and False match the equal numeric value
across types (and False matches an Empty key).
---
 dlls/scrrun/dictionary.c       |  5 +++++
 dlls/scrrun/tests/dictionary.c | 20 ++++++++++----------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/dlls/scrrun/dictionary.c b/dlls/scrrun/dictionary.c
index 8137855b592..def6a3684ed 100644
--- a/dlls/scrrun/dictionary.c
+++ b/dlls/scrrun/dictionary.c
@@ -119,6 +119,7 @@ static inline BOOL is_numeric_key(const VARIANT *key)
         case VT_DATE:
         case VT_R4:
         case VT_R8:
+        case VT_BOOL:
             return TRUE;
         default:
             return FALSE;
@@ -979,6 +980,10 @@ static HRESULT WINAPI dictionary_get_HashVal(IDictionary *iface, VARIANT *key, V
     case VT_R8|VT_BYREF:
     case VT_R8:
         return get_flt_hash(V_VT(key) & VT_BYREF ? *V_R8REF(key) : V_R8(key), &V_I4(hash));
+    case VT_BOOL|VT_BYREF:
+    case VT_BOOL:
+        V_I4(hash) = get_num_hash(V_VT(key) & VT_BYREF ? *V_BOOLREF(key) : V_BOOL(key));
+        break;
     case VT_EMPTY:
     case VT_NULL:
         V_I4(hash) = 0;
diff --git a/dlls/scrrun/tests/dictionary.c b/dlls/scrrun/tests/dictionary.c
index 8da22a24101..8ff452079de 100644
--- a/dlls/scrrun/tests/dictionary.c
+++ b/dlls/scrrun/tests/dictionary.c
@@ -832,17 +832,17 @@ if (0) { /* crashes on native */
     V_BOOL(&key) = VARIANT_FALSE;
     V_I4(&hash) = 5678;
     hr = IDictionary_get_HashVal(dict, &key, &hash);
-    todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
     ok(V_VT(&hash) == VT_I4, "Unexpected hash type %d.\n", V_VT(&hash));
-    todo_wine ok(V_I4(&hash) == get_num_hash(0.0f), "Unexpected hash value %#lx.\n", V_I4(&hash));
+    ok(V_I4(&hash) == get_num_hash(0.0f), "Unexpected hash value %#lx.\n", V_I4(&hash));
 
     V_VT(&key) = VT_BOOL;
     V_BOOL(&key) = VARIANT_TRUE;
     V_I4(&hash) = 5678;
     hr = IDictionary_get_HashVal(dict, &key, &hash);
-    todo_wine ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+    ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
     ok(V_VT(&hash) == VT_I4, "Unexpected hash type %d.\n", V_VT(&hash));
-    todo_wine ok(V_I4(&hash) == get_num_hash((FLOAT)VARIANT_TRUE), "Unexpected hash value %#lx.\n", V_I4(&hash));
+    ok(V_I4(&hash) == get_num_hash((FLOAT)VARIANT_TRUE), "Unexpected hash value %#lx.\n", V_I4(&hash));
 
     V_VT(&key) = VT_EMPTY;
     V_I4(&key) = 1234;
@@ -1269,19 +1269,19 @@ static void test_bool_key(void)
     V_BOOL(&a) = VARIANT_TRUE;
     V_VT(&b) = VT_I2;
     V_I2(&b) = -1;
-    todo_wine ok(keys_match(&a, &b), "True should match I2 -1\n");
+    ok(keys_match(&a, &b), "True should match I2 -1\n");
 
     V_VT(&a) = VT_BOOL;
     V_BOOL(&a) = VARIANT_FALSE;
     V_VT(&b) = VT_I2;
     V_I2(&b) = 0;
-    todo_wine ok(keys_match(&a, &b), "False should match I2 0\n");
+    ok(keys_match(&a, &b), "False should match I2 0\n");
 
     /* False is the zero/default value, so it matches an Empty key. */
     V_VT(&a) = VT_BOOL;
     V_BOOL(&a) = VARIANT_FALSE;
     V_VT(&b) = VT_EMPTY;
-    todo_wine ok(keys_match(&a, &b), "False should match Empty\n");
+    ok(keys_match(&a, &b), "False should match Empty\n");
 
     /* True and False are distinct keys. */
     V_VT(&a) = VT_BOOL;
@@ -1306,11 +1306,11 @@ static void test_bool_key(void)
     V_BOOL(&b) = VARIANT_TRUE;
     exists = VARIANT_FALSE;
     hr = IDictionary_Exists(dict, &b, &exists);
-    todo_wine ok(hr == S_OK, "Exists with Boolean key: %#lx.\n", hr);
-    todo_wine ok(exists == VARIANT_TRUE, "True should find I2 -1, got %x\n", exists);
+    ok(hr == S_OK, "Exists with Boolean key: %#lx.\n", hr);
+    ok(exists == VARIANT_TRUE, "True should find I2 -1, got %x\n", exists);
 
     hr = IDictionary_Remove(dict, &b);
-    todo_wine ok(hr == S_OK, "Remove with Boolean key: %#lx.\n", hr);
+    ok(hr == S_OK, "Remove with Boolean key: %#lx.\n", hr);
 
     IDictionary_Release(dict);
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/10960

[Francis De Brabandere (＠francisdb)]

@nsivov this is ready for review

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10960#note_141832

[PATCH v6 0/4] MR10960: scrrun/dictionary: Match native semantics for Empty and Boolean keys. - approved

[Nikolay Sivov (＠nsivov)]

This merge request was approved by Nikolay Sivov.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10960

[Francis De Brabandere (＠francisdb)]

Thanks @nsivov !

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/10960#note_141879