pcdshub / pytmc

Generate EPICS IOCs and records from TwinCAT projects - along with many TwinCAT project tools
https://pcdshub.github.io/pytmc/
Other
10 stars 11 forks source link

An edge case missed by pytmc pragmalint #323

Open ZLLentz opened 11 months ago

ZLLentz commented 11 months ago

In this example, we miss the malformed pragma because it is so malformed that pytmc doesn't even register it as a pragma. I'm not sure how wide the scope of pragmalint should be but I thought I'd note this here.

https://github.com/pcdshub/lcls-twincat-general/blob/73cdd56e72eea0bb6fcd871b207bf4f621023a57/LCLSGeneral/LCLSGeneral/DUTs/DUT_EPS.TcDUT#L31

    {atribute 'pytmc' := '
        pv: bEPS_OK
        io: i
        field: DESC check if nFlags are all true;
    '}
    bEPS_OK : BOOL := TRUE;
 {atribute 'pytmc' := '
atribute
ZLLentz commented 11 months ago

I think the ; is also invalid here, but it isn't noticed because this isn't an attribute pragma

klauer commented 11 months ago

; is actually valid (see #306 )

Our regex looks like this: https://github.com/pcdshub/pytmc/blob/2c4f4121cd109d2d2cee9bdcc357d792ddca01f7/pytmc/bin/pragmalint.py#L22-L24

So it doesn't consider "oops they might have typed attribute wrong"

I'm up for suggestions as to how to better restructure that - just it's tough to say "oh this looks kinda like a pragma"

ZLLentz commented 11 months ago

I totally agree that this is tough- I made the issue fully knowing that we can't cover for every single kind of malformed entry, and if we try then we'll start flagging false positives of things that were never intended to be pytmc pragmas.

No immediate "clearly good" ideas- maybe we can have a separate regex that's just "includes pytmc within curly brackets but doesn't get matched here"? But of course we still miss typos of pytmc and, again, if you go too far you'll find that you can no longer include the word "pytmc" in other pragmas without linter errors.

"includes the word pytmc and has a pragma type that isn't on the list of existing pragma types" is a bit of an annoying mouthful. "includes an invalid pragma type" is kind of reasonable but still annoying.

klauer commented 11 months ago

Well-stated @ZLLentz ...

For pattern matching without being excessive (I share the same concern about taking it too far and overlapping with other uses of non-pytmc pragmas), maybe checking to see if the following fail to parse as pragmas could be good enough:

ZLLentz commented 11 months ago

I need more time to think about the implications of doing those precise checks.

I think our overall goal should be to handle these two cases:

To me, misspellings are most likely to be non-obvious when the first (and to a lesser extent, the last) letters are correct but there's some mix-up in the middle.

So maybe something similar to your suggestions are all that's needed to hit these sorts of cases, without reaching too far into collateral damage areas.