[PATCH] vbscript: Treat tokens after a '.' as identifier

[Brendan McGrath]

Currently, a vbscript using an identifier that is also a keyword fails
with a parser error. For example:
Set oLocator = CreateObject("Wbemscripting.SWbemLocator")
Set oReg = oLocator.ConnectServer("", "root\default", "", "").Get("StdRegProv")

results in:
0009:fixme:vbscript:parse_script parser failed around L"efault", "", "").Get("StdRegProv")\n"

The 'Get' method here causes the parser error as it is also a vbscript
keyword.

This patch treats any token after a '.' as an identifier.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46318
Signed-off-by: Brendan McGrath [email protected]
---

I _think_ this patch is OK - but I don't have a lot of experience with
vbscript. There may be a scenario where a keyword can immediately follow
a '.'.

I've also created the bug linked above (where this patch is
proposed).

dlls/vbscript/lex.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dlls/vbscript/lex.c b/dlls/vbscript/lex.c
index 571854db58e..0d27c2423c7 100644
--- a/dlls/vbscript/lex.c
+++ b/dlls/vbscript/lex.c
@@ -412,7 +412,10 @@ static int parse_next_token(void *lval, parser_ctx_t *ctx)
         return parse_numeric_literal(ctx, lval);

if(isalphaW(c)) {
-        int ret = check_keywords(ctx);
+        int ret = 0;
+
+        if(ctx->last_token != '.')
+            ret = check_keywords(ctx);
         if(!ret)
             return parse_identifier(ctx, lval);
         if(ret != tREM)

[Jacek Caban]

Hi Brendan,

Thanks for looking at this.

On 19/12/2018 01:52, Brendan McGrath wrote:
> Currently, a vbscript using an identifier that is also a keyword fails
> with a parser error. For example:
> Set oLocator = CreateObject("Wbemscripting.SWbemLocator")
> Set oReg = oLocator.ConnectServer("", "root\default", "", "").Get("StdRegProv")
>
> results in:
> 0009:fixme:vbscript:parse_script parser failed around L"efault", "", "").Get("StdRegProv")\n"
>
> The 'Get' method here causes the parser error as it is also a vbscript
> keyword.
>
> This patch treats any token after a '.' as an identifier.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46318
> Signed-off-by: Brendan McGrath [email protected]
>
> I _think_ this patch is OK - but I don't have a lot of experience with
> vbscript. There may be a scenario where a keyword can immediately follow
> a '.'.

I think the issue is more general than an identifier after '.' token. We 
already have code for similar problem with 'property' keyword (see 
Identifier rule in parser.y), we probably could extend that. Also 
jscript has a more complete solution for similar problem (see 
ReservedAsIdentifier) that you could use as an example.

Also please include some tests in the patch.

Thanks,
Jacek

[PATCH v2 1/2] vbscript/tests: Test 'Get' keyword used as id

[Brendan McGrath]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46318 
Signed-off-by: Brendan McGrath [email protected]
---
 dlls/vbscript/tests/run.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/dlls/vbscript/tests/run.c b/dlls/vbscript/tests/run.c
index 191f5a79a0..bc3e0742df 100644
--- a/dlls/vbscript/tests/run.c
+++ b/dlls/vbscript/tests/run.c
@@ -2406,6 +2406,10 @@ static void run_tests(void)
     CHECK_CALLED(global_success_d);
     CHECK_CALLED(global_success_i);

+    hres = parse_script_ar("Set oLocator = CreateObject("Wbemscripting.SWbemLocator")\r"
+                           "Set oReg = oLocator.ConnectServer("", "root\default", "", "").Get("StdRegProv")");
+    todo_wine ok(hres == S_OK, "parse_script failed: %08x\n", hres);
+
     run_from_res("lang.vbs");
     run_from_res("api.vbs");
     run_from_res("regexp.vbs");

[PATCH v2 2/2] vbscript: Allow GET, SET and LET to be used as id

[Brendan McGrath]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46318
Signed-off-by: Brendan McGrath [email protected]
---
I took a look at the way jscript did this - but I couldn't work out how it
was using '$1' as a value. So I ended up following the approach used for
'property'.

This meant I had to add a new constant definition for each keyword that
could be used as an indentifier. As a result I went with a conservative
approach and only added three more (in addition to 'property').

As a result - this passes the test in the bug report but perhaps doesn't
address the other keywords.

dlls/vbscript/parser.y | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/dlls/vbscript/parser.y b/dlls/vbscript/parser.y
index 020109998e..9e9f5ab932 100644
--- a/dlls/vbscript/parser.y
+++ b/dlls/vbscript/parser.y
@@ -72,6 +72,9 @@ static class_decl_t *add_dim_prop(parser_ctx_t*,class_decl_t*,dim_decl_t*,unsign
 static statement_t *link_statements(statement_t*,statement_t*);

static const WCHAR propertyW[] = {'p','r','o','p','e','r','t','y',0};
+static const WCHAR getW[] = {'g','e','t',0};
+static const WCHAR setW[] = {'s','e','t',0};
+static const WCHAR letW[] = {'l','e','t',0};

#define STORAGE_IS_PRIVATE    1
 #define STORAGE_IS_DEFAULT    2
@@ -138,7 +141,7 @@ static const WCHAR propertyW[] = {'p','r','o','p','e','r','t','y',0};
 %type <dim_decl> DimDeclList DimDecl
 %type <dim_list> DimList
 %type <const_decl> ConstDecl ConstDeclList
-%type <string> Identifier
+%type <string> Identifier ReservedAsIdentifier
 %type <case_clausule> CaseClausules

%%
@@ -445,7 +448,13 @@ ArgumentDecl

/* 'property' may be both keyword and identifier, depending on context */
 Identifier
-    : tIdentifier    { $$ = $1; }
+    : tIdentifier           { $$ = $1; }
+    | ReservedAsIdentifier  { $$ = $1; }
+
+ReservedAsIdentifier
+    : tGET           { $$ = getW; }
+    | tSET           { $$ = setW; }
+    | tLET           { $$ = letW; }
     | tPROPERTY      { $$ = propertyW; }

/* Most statements accept both new line and ':' as separators */
diff --git a/dlls/vbscript/tests/run.c b/dlls/vbscript/tests/run.c
index bc3e0742df..2cb52f4145 100644
--- a/dlls/vbscript/tests/run.c
+++ b/dlls/vbscript/tests/run.c
@@ -2408,7 +2408,6 @@ static void run_tests(void)

-    hres = parse_script_ar("Set oLocator = CreateObject("Wbemscripting.SWbemLocator")\r"
-                           "Set oReg = oLocator.ConnectServer("", "root\default", "", "").Get("StdRegProv")");
-    todo_wine ok(hres == S_OK, "parse_script failed: %08x\n", hres);
+    parse_script_a("Set oLocator = CreateObject("Wbemscripting.SWbemLocator")\r"
+                   "Set oReg = oLocator.ConnectServer("", "root\default", "", "").Get("StdRegProv")");

run_from_res("lang.vbs");
     run_from_res("api.vbs");

[PATCH v2 2/2] vbscript: Allow GET, SET and LET to be used as id

[Jacek Caban]

Hi Brendan,

On 2/10/19 1:45 AM, Brendan McGrath wrote:
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46318
> Signed-off-by: Brendan McGrath [email protected]
>
> I took a look at the way jscript did this - but I couldn't work out how it
> was using '$1' as a value. So I ended up following the approach used for
> 'property'.

jscript can do that, because it declares tokens to have <identifier> 
type (see line 170 of its parser) and lexer returns appropriate value 
(see check_keywords lval handling). Anyway, I'd be fine with your 
version once it has proper tests.

BTW, it's probably a good idea to merge test and fix into one patch in 
this case.
> This meant I had to add a new constant definition for each keyword that
> could be used as an indentifier.As a result I went with a conservative
> approach and only added three more (in addition to 'property').

Conservative approach is fine and even preferred. Some of other keywords 
may cause parser conflicts, so we need to be careful about that.

Thanks,

Jacek

[PATCH v2 1/2] vbscript/tests: Test 'Get' keyword used as id

[Jacek Caban]

Hi Brendan,

On 2/10/19 1:45 AM, Brendan McGrath wrote:
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46318
> Signed-off-by: Brendan McGrath [email protected]
>
> dlls/vbscript/tests/run.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/dlls/vbscript/tests/run.c b/dlls/vbscript/tests/run.c
> index 191f5a79a0..bc3e0742df 100644
> --- a/dlls/vbscript/tests/run.c
> +++ b/dlls/vbscript/tests/run.c
> @@ -2406,6 +2406,10 @@ static void run_tests(void)
>       CHECK_CALLED(global_success_d);
>       CHECK_CALLED(global_success_i);
> - hres = parse_script_ar("Set oLocator = CreateObject("Wbemscripting.SWbemLocator")\r"
> -
>                       "Set oReg = oLocator.ConnectServer(\"\", \"root\\default\", \"\", \"\").Get(\"StdRegProv\")");
> - todo_wine ok(hres == S_OK, "parse_script failed: %08x\n", hres);

Please integrate the test into existing lang.vbs. Note that you don't 
really need to create external objects to test that. You could, for 
example, try declare a variable called 'get'.

Thanks,

Jacek

[PATCH v2 1/2] vbscript/tests: Test 'Get' keyword used as id

[Brendan McGrath]

I tried 'Dim Get' and 'Get = "xx"' in Windows, but they both create a 
"compilation error". So I think 'Get' can be used as a Function on an 
external object - but maybe can not be used as a declaration in vbscript.

I'm not really familiar with the grammatical notation of vbscript - but 
I did find this:
https://rosettacode.org/wiki/BNF_Grammar#VBScript

It looks like after a 'dot', all the Keywords can be used (referred to 
as a QualifiedIDTail in the link above), but after a 'Dim', only an 
ExtendedID can be used (which only includes a subset of Keywords 
referred to as 'SafeKeywordID' - which includes 'Property'). So I think 
I should only be including the SafeKeywordIDs as an Identifier (which 
does not include 'Get', 'Set' or 'Let').

For items after a dot, I guess I should introduce a new token (maybe 
called DotIdentifier) which will include all the Keywords? I believe I 
would therefore need to use an external object to test those.

On 12/2/19 10:45 pm, Jacek Caban wrote:
> Hi Brendan,
>
> On 2/10/19 1:45 AM, Brendan McGrath wrote:
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=46318
> > Signed-off-by: Brendan McGrath [email protected]
> >
> > dlls/vbscript/tests/run.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/dlls/vbscript/tests/run.c b/dlls/vbscript/tests/run.c
> > index 191f5a79a0..bc3e0742df 100644
> > --- a/dlls/vbscript/tests/run.c
> > +++ b/dlls/vbscript/tests/run.c
> > @@ -2406,6 +2406,10 @@ static void run_tests(void)
> >       CHECK_CALLED(global_success_d);
> >       CHECK_CALLED(global_success_i);
> >   +    hres = parse_script_ar("Set oLocator = 
> > CreateObject("Wbemscripting.SWbemLocator")\r"
> > +                           "Set oReg = oLocator.ConnectServer("", 
> > "root\default", "", "").Get("StdRegProv")");
> > +    todo_wine ok(hres == S_OK, "parse_script failed: %08x\n", hres);
>
> Please integrate the test into existing lang.vbs. Note that you don't 
> really need to create external objects to test that. You could, for 
> example, try declare a variable called 'get'.
>
> Thanks,
>
> Jacek

[PATCH v2 1/2] vbscript/tests: Test 'Get' keyword used as id

[Jacek Caban]

Hi Brendan,

On 2/13/19 1:33 AM, Brendan McGrath wrote:
> I tried 'Dim Get' and 'Get = "xx"' in Windows, but they both create a 
> "compilation error". So I think 'Get' can be used as a Function on an 
> external object - but maybe can not be used as a declaration in vbscript.
>
> I'm not really familiar with the grammatical notation of vbscript - 
> but I did find this:
> https://rosettacode.org/wiki/BNF_Grammar#VBScript
>
> It looks like after a 'dot', all the Keywords can be used (referred to 
> as a QualifiedIDTail in the link above), but after a 'Dim', only an 
> ExtendedID can be used (which only includes a subset of Keywords 
> referred to as 'SafeKeywordID' - which includes 'Property'). So I 
> think I should only be including the SafeKeywordIDs as an Identifier 
> (which does not include 'Get', 'Set' or 'Let').
>
> For items after a dot, I guess I should introduce a new token (maybe 
> called DotIdentifier) which will include all the Keywords?

Yes, it looks like we will need something like that.
> I believe I would therefore need to use an external object to test those.

Note that we already have such object in tests. It's accessible as 
testObj in scripts and implemented in run.c (mostly testObj_GetDispID 
and testObj_InvokeEx). We could probably have a single dispid for 
multiple keyword identifiers there to simplify things.

Thanks,

Jacek