[PATCH v3 0/1] MR4615: vbscript: For for loop coerce right bound string to real.

[Robert Wilhelm (＠sloper42)]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55052

--
  v3: vbscript: For for loop bounds coerce string to real.

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

[PATCH v3 1/1] vbscript: For for loop bounds coerce string to real.

[Robert Wilhelm]

From: Robert Wilhelm

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=55052
---
 dlls/vbscript/compile.c      |  6 ++++--
 dlls/vbscript/interp.c       | 32 ++++++++++++++++++++++++++++++++
 dlls/vbscript/tests/lang.vbs | 27 +++++++++++++++++++++++++++
 dlls/vbscript/vbscript.h     |  1 +
 4 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/dlls/vbscript/compile.c b/dlls/vbscript/compile.c
index 683c71e2f4e..baddc51d7e4 100644
--- a/dlls/vbscript/compile.c
+++ b/dlls/vbscript/compile.c
@@ -875,6 +875,8 @@ static HRESULT compile_forto_statement(compile_ctx_t *ctx, forto_statement_t *st
     hres = compile_expression(ctx, stat->from_expr);
     if(FAILED(hres))
         return hres;
+    if(!push_instr(ctx, OP_numval))
+        return E_OUTOFMEMORY;

/* FIXME: Assign should happen after both expressions evaluation. */
     instr = push_instr(ctx, OP_assign_ident);
@@ -887,7 +889,7 @@ static HRESULT compile_forto_statement(compile_ctx_t *ctx, forto_statement_t *st
     if(FAILED(hres))
         return hres;

-    if(!push_instr(ctx, OP_val))
+    if(!push_instr(ctx, OP_numval))
         return E_OUTOFMEMORY;

if(stat->step_expr) {
@@ -895,7 +897,7 @@ static HRESULT compile_forto_statement(compile_ctx_t *ctx, forto_statement_t *st
         if(FAILED(hres))
             return hres;

-        if(!push_instr(ctx, OP_val))
+        if(!push_instr(ctx, OP_numval))
             return E_OUTOFMEMORY;
     }else {
         hres = push_instr_int(ctx, OP_int, 1);
diff --git a/dlls/vbscript/interp.c b/dlls/vbscript/interp.c
index 2d979f07605..bbe5ca7a6b2 100644
--- a/dlls/vbscript/interp.c
+++ b/dlls/vbscript/interp.c
@@ -1088,6 +1088,38 @@ static HRESULT interp_val(exec_ctx_t *ctx)
     return stack_push(ctx, val.owned ? val.v : &v);
 }

+static HRESULT interp_numval(exec_ctx_t *ctx)
+{
+    variant_val_t val;
+    VARIANT v,vcoerced,*r;
+    HRESULT hres;
+    BOOL hastocoerce = FALSE;
+
+    TRACE("\n");
+
+    hres = stack_pop_val(ctx, &val);
+    if(FAILED(hres))
+        return hres;
+
+    r = val.v;
+    if(!val.owned) {
+        V_VT(&v) = VT_EMPTY;
+        hres = VariantCopy(&v, val.v);
+        if(FAILED(hres))
+            return hres;
+        r = &v;
+    }
+
+    if (V_VT(r) == VT_BSTR) {
+        hastocoerce = TRUE;
+        V_VT(&vcoerced) = VT_EMPTY;
+        hres = VariantChangeType(&vcoerced, r, 0, VT_R8);
+        if(FAILED(hres))
+            return hres;
+    }
+    return stack_push(ctx, hastocoerce? &vcoerced : r);
+}
+
 static HRESULT interp_pop(exec_ctx_t *ctx)
 {
     const unsigned n = ctx->instr->arg1.uint;
diff --git a/dlls/vbscript/tests/lang.vbs b/dlls/vbscript/tests/lang.vbs
index 42af6df0e76..3c6ce656f1c 100644
--- a/dlls/vbscript/tests/lang.vbs
+++ b/dlls/vbscript/tests/lang.vbs
@@ -679,6 +679,33 @@ for x = 5 to 8
 next
 Call ok(y = "for8: 5 7", "y = " & y)

+function testfor( startvalue, endvalue, stepvalue, steps)
+    Dim s
+    s=0
+    for x=startvalue to endvalue step stepvalue
+        s = s + 1
+    Next
+    Call ok( s = steps, "counted " & s & " steps in for loop, expected " & steps)
+end function
+
+Call testfor (1, 2, 1, 2)
+Call testfor ("1", 2, 1, 2)
+Call testfor (1, "2", 1, 2)
+Call testfor (1, 2, "1", 2)
+Call testfor ("1", "2", "1", 2)
+if (isEnglishLang) then
+    Call testfor (1, 2, 0.5, 3)
+    Call testfor (1, 2.5, 0.5, 4)
+    Call testfor ("1", 2,  0.5, 3)
+    Call testfor ("1", 2.5, 0.5, 4)
+    Call testfor (1, "2",  0.5, 3)
+    Call testfor (1, "2.5", 0.5, 4)
+    Call testfor (1, 2, "0.5", 3)
+    Call testfor (1, 2.5, "0.5", 4)
+    Call testfor ("1", "2", "0.5", 3)
+    Call testfor ("1", "2.5", "0.5", 4)
+end if
+
 for x = 1.5 to 1
     Call ok(false, "for..to called when unexpected")
 next
diff --git a/dlls/vbscript/vbscript.h b/dlls/vbscript/vbscript.h
index 24915a5ca04..56e68411f45 100644
--- a/dlls/vbscript/vbscript.h
+++ b/dlls/vbscript/vbscript.h
@@ -278,6 +278,7 @@ typedef enum {
     X(not,            1, 0,           0)          \
     X(nothing,        1, 0,           0)          \
     X(null,           1, 0,           0)          \
+    X(numval,         1, 0,           0)          \
     X(or,             1, 0,           0)          \
     X(pop,            1, ARG_UINT,    0)          \
     X(redim,          1, ARG_BSTR,    ARG_UINT)   \

[Robert Wilhelm (＠sloper42)]

Hi Jacek, thanks for review. I added new opcode "numval" and it seems to work fine.

[Jacek Caban (＠jacek)]

Jacek Caban (@jacek) commented about dlls/vbscript/interp.c:
> - if(!val.owned) {
> -
>    V_VT(&v) = VT_EMPTY;
> -
>    hres = VariantCopy(&v, val.v);
> -
>    if(FAILED(hres))
> -
>        return hres;
> -
>    r = &v;
> - }
> -
> - if (V_VT(r) == VT_BSTR) {
> -
>    hastocoerce = TRUE;
> -
>    V_VT(&vcoerced) = VT_EMPTY;
> -
>    hres = VariantChangeType(&vcoerced, r, 0, VT_R8);
> -
>    if(FAILED(hres))
> -
>        return hres;
> - }
> - return stack_push(ctx, hastocoerce? &vcoerced : r);

This looks correct now, but you could avoid extra copy and simplify things a bit if you handled VT_BSTR first, separately. You could move `VT_BSTR` check earlier, pass `val.v` as `VariantChangeType` source, call `release_val` and return early with `stack_push`. The rest of the function could then look just like `interp_val`.