klauer / blark

Beckhoff TwinCAT ST (IEC 61131-3) code parsing in Python using Lark (Earley)
https://klauer.github.io/blark/
GNU General Public License v2.0
42 stars 5 forks source link

Array Initializer #56

Closed engineerjoe440 closed 1 year ago

engineerjoe440 commented 1 year ago

Related Issues

Changes

Closing Thoughts

Hopefully with these additions, some of the "in-the-wild" code we've come across will get patched up and parse as we expect. :smile:

codecov-commenter commented 1 year ago

Codecov Report

Merging #56 (043f20e) into master (c6e0e18) will increase coverage by 0.3%. The diff coverage is 100.0%.

Additional details and impacted files [![Impacted file tree graph](https://codecov.io/gh/klauer/blark/pull/56/graphs/tree.svg?width=650&height=150&src=pr&token=QZDTKF30TH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=K+Lauer)](https://codecov.io/gh/klauer/blark/pull/56?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=K+Lauer) ```diff @@ Coverage Diff @@ ## master #56 +/- ## ======================================== + Coverage 72.1% 72.5% +0.3% ======================================== Files 18 18 Lines 3783 3797 +14 ======================================== + Hits 2730 2755 +25 + Misses 1053 1042 -11 ``` | [Impacted Files](https://codecov.io/gh/klauer/blark/pull/56?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=K+Lauer) | Coverage Δ | | |---|---|---| | [blark/tests/test\_transformer.py](https://codecov.io/gh/klauer/blark/pull/56?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=K+Lauer#diff-YmxhcmsvdGVzdHMvdGVzdF90cmFuc2Zvcm1lci5weQ==) | `100.0% <ø> (ø)` | | | [blark/transform.py](https://codecov.io/gh/klauer/blark/pull/56?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=K+Lauer#diff-YmxhcmsvdHJhbnNmb3JtLnB5) | `99.0% <100.0%> (+0.1%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://codecov.io/gh/klauer/blark/pull/56/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=K+Lauer)
klauer commented 1 year ago

OK, I've confirmed this parses all of our codebase at least as well as the previous commit. Only a single unexpected failure which (if I recall correctly - correct me if I'm wrong!) goes through a separate grammar rule than what you've introduced here:

https://github.com/pcdshub/lcls-twincat-motion/blob/7ed79934840b38b71bb126255fefcf97422d1e71/lcls-twincat-motion/Library/POUs/Motion/FB_MotionPneumaticActuator.TcPOU#L43-L46

(UnexpectedCharacters) No terminal matches '#' in the current parser context, at line 43 col 25

        i_TypeCode := 10#1010);
                        ^

This is worthy of a separate issue + PR to fix, if so

engineerjoe440 commented 1 year ago

OK, I've confirmed this parses all of our codebase at least as well as the previous commit. Only a single unexpected failure which (if I recall correctly - correct me if I'm wrong!) goes through a separate grammar rule than what you've introduced here:

https://github.com/pcdshub/lcls-twincat-motion/blob/7ed79934840b38b71bb126255fefcf97422d1e71/lcls-twincat-motion/Library/POUs/Motion/FB_MotionPneumaticActuator.TcPOU#L43-L46

(UnexpectedCharacters) No terminal matches '#' in the current parser context, at line 43 col 25

        i_TypeCode := 10#1010);
                        ^

This is worthy of a separate issue + PR to fix, if so

I was hoping to have reviewed this more closely today, but haven't had a chance to... yet. This seems like a new issue to me. Was this not appearing with master-branch blark? Only with these changes? I'd like to better wrap my head around that, and potentially get a fix, anyway.

klauer commented 1 year ago

Sorry, that was poorly phrased on my part!

blark master and this PR fail in the same way parsing that source code file. It was closely related in grammar rules to this PR so it gave me pause. Looking at its tree (avoiding the base-specifying 10# part that's throwing it off) gives:

            structure_initialization
              structure_element_initialization
                i_DevName
                constant
                  string_literal        'MPA'
              structure_element_initialization
                i_Desc
                constant
                  string_literal        'Fault occurs when the device is moving'
              structure_element_initialization
                i_TypeCode
                constant

And... it turns out we don't have a rule for explicit base 10 integer literals. D'oh! https://github.com/klauer/blark/blob/043f20ed36c25ac793886181b0a5c0232f218930/blark/iec.lark#L61-L65

Will TwinCAT accept any base or just binary/hex/octal/decimal, I wonder... Hmm...

In any case, this PR is great! Thanks again. I'm going to merge and make an issue for the above.

engineerjoe440 commented 1 year ago

Aha!!!! I see your point!!! Thank you so much, @klauer! :smile: