Boolean literals are parsed as identifiers

[alajovic]

Description of the issue

Boolean literals end up being parsed as IDENTIFIER terminals rather than boolean_literal instances.

Steps to reproduce

Write the following source to test.txt:

VAR_GLOBAL
variable : BOOL := TRUE;
END_VAR

Run

blark parse --print-tree test.txt

Expected result

The string TRUE gets parsed as a boolean literal:

iec_source
  global_var_declarations
    [... non-relevant part of the output omitted ...]
                            unary_expression
                              None
                              constant
                                boolean_literal TRUE

Actual result

The string TRUE gets parsed as an identifier. Note that the full meaning of the TRUE in the output below is, in lark's notation, Token('IDENTIFIER', 'TRUE').

iec_source
  global_var_declarations
    [... non-relevant part of the output omitted ...]
                            unary_expression
                              None
                              variable_name
                                TRUE
                                None

Additional information

I am able to get the expected parse result by increasing the priority of the constant rule in the grammar, i.e., by replacing this line:

constant: time_literal

with this:

constant.1: time_literal

Please let me know if this seems a sensible solution. I will gladly prepare a PR if you agree.

[klauer]

Good find! Thanks for reporting and trying out a solution.

The priority increase could be the right way to go. Running it against the test suite would give me some more confidence. If you have the time, please do open the PR.

There is also potentially some overlap with the redefinition of IDENTIFIER in #111 as it is another reserved word. Perhaps it could also be part of the large regex there. (edit: if you saw the previous version of this, my apologies, I thought blark had one new contributor, but it has two.)

[alajovic]

Well, well, well. Who would have thought. I implemented the change, ran the test suite – all tests passed, pushed the change, created a PR ... and then realized there was one additional test that did not run by default. But it did run in github's CI pipeline, and it failed. It was the uncovered grammar test, and it said that two grammar rules evaded use in the transformer test suite: double_byte_string_var_declaration and single_byte_string_var_declaration.

I looked into it now, and I think I see what's going on. Consider this example code:

VAR
    wsValue : WSTRING := "abc";
END_VAR

Each variable declaration is parsed according to var_init_decl:

?var_init_decl: array_var_init_decl
              | structured_var_init_decl
              | string_var_declaration
              | var1_init_decl
              | fb_decl

Let's now transform this by choosing specific alternatives and expanding rules into their definitions. Indented lines give information about which choice to take and what to expand.

array_var_init_decl | structured_var_init_decl | string_var_declaration | var1_init_decl | fb_decl
    Step 1: choose "string_var_declaration" and expand it
single_byte_string_var_declaration | double_byte_string_var_declaration
    Step 2: choose "double_byte_string_var_declaration" and expand it
var1_list ":" double_byte_string_spec
    Step 3: expand "double_byte_string_spec"
var1_list ":" WSTRING [ string_spec_length ] [ ":=" DOUBLE_BYTE_CHARACTER_STRING ]

Okay. Let's do this one more time, again starting with var_init_decl, but this time going down another path:

array_var_init_decl | structured_var_init_decl | string_var_declaration | var1_init_decl | fb_decl
    Step 1: choose var1_init_decl and expand it
var1_list ":" ( simple_spec_init | subrange_spec_init | enumerated_spec_init )
    Step 2: choose "simple_spec_init" and expand it
var1_list ":" ( simple_specification | indirect_simple_specification ) [ ":=" expression ]
    Step 3: choose "simple_specification" and expand it
var1_list ":" (elementary_type_name | simple_type_name | DOTTED_IDENTIFIER) [ ":=" expression ]
    Step 4: choose "elementary_type_name" and expand it
var1_list ":" (NUMERIC_TYPE_NAME | DATE_TYPE_NAME | BIT_STRING_TYPE_NAME | string_type_specification) [ ":=" expression ]
    Step 5: choose "string_type_specification" and expand it
var1_list ":" (STRING | WSTRING) [ string_spec_length ] [ ":=" expression ]
    Step 6: choose "WSTRING"
var1_list ":" WSTRING [ string_spec_length ] [ ":=" expression ]
    Step 7: recursively expand "expression" down to "constant"
var1_list ":" WSTRING [ string_spec_length ] [ ":=" constant ]
    Step 8: expand constant
var1_list ":" WSTRING [ string_spec_length ] [ ":=" (time_literal | numeric_literal | string_literal | bit_string_literal | boolean_literal) ]
    Step 9: choose "string_literal" and expand it
var1_list ":" WSTRING [ string_spec_length ] [ ":=" SINGLE_BYTE_CHARACTER_STRING | DOUBLE_BYTE_CHARACTER_STRING ]
    Step 10: choose "DOUBLE_BYTE_CHARACTER_STRING"
var1_list ":" WSTRING [ string_spec_length ] [ ":=" DOUBLE_BYTE_CHARACTER_STRING ]

The result is the same! var1_init_decl is, under suitable choices, exactly equivalent to double_byte_string_var_declaration. Which means that in absence of other cues, the parser resolves the ambiguity in some arbitrary way. However, if the priority of the constant grammar term is bumped, the parser gets the chance to take the higher priority path in step 7, and so it starts preferring this interpretation.

The obvious question here is whether the string_var_declaration rule (along with its subordinates) is even needed. Just out of curiosity, I tried throwing it out of var1_init_decl (this is the only place where it's used), and the entire test suite still passed.

[klauer]

Excellent breakdown of what's happening there. I think you're right - string_var_declaration and subordinates are redundant and just lead to ambiguity.

So in addition to the test suite, I ran the removal of that rule through what I'm now calling the blark TwinCAT corpus (i.e., a bunch of repos I want blark to parse) and I didn't notice any issues related to this change.

Details

diff --git a/blark/iec.lark b/blark/iec.lark
index e942aae..a6a771b 100644
--- a/blark/iec.lark
+++ b/blark/iec.lark
@@ -430,7 +430,6 @@ edge_declaration: var1_list ":" _BOOL ( R_EDGE | F_EDGE )

 ?var_init_decl: array_var_init_decl
               | structured_var_init_decl
-              | string_var_declaration
               | var1_init_decl
               | fb_decl

@@ -499,11 +498,6 @@ location: _AT direct_variable

 global_var_list: global_var_name ( "," global_var_name )*

-?string_var_declaration: single_byte_string_var_declaration
-                       | double_byte_string_var_declaration
-
-single_byte_string_var_declaration: var1_list ":" single_byte_string_spec
-
 bracketed_expression: "[" expression "]"

 string_spec_length: parenthesized_expression
@@ -511,8 +505,6 @@ string_spec_length: parenthesized_expression

 single_byte_string_spec: STRING [ string_spec_length ] [ ":=" SINGLE_BYTE_CHARACTER_STRING ]

-double_byte_string_var_declaration: var1_list ":" double_byte_string_spec
-
 double_byte_string_spec: WSTRING [ string_spec_length ] [ ":=" DOUBLE_BYTE_CHARACTER_STRING ]

 incomplete_located_var_declarations: _VAR [ variable_attributes ] incomplete_located_var_decl* _END_VAR ";"*

I'm happy to see some rules removed from the grammar, admittedly, as it's rather complicated. I'd be OK with merging the above along with #114

[alajovic]

I went ahead and removed those grammar rules, together with the corresponding callback in transform.py.

Thanks for compiling the corpus! It's a really neat idea, and I will surely be testing any future changes against it.

[alajovic]

I see that the CI pipeline for #114 has failed again, but it appears to be due to rate limiting of uploads to codecov.io. Please re-run the pipeline after a while and see if it passes then.