Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update vba.g4 #3880

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update vba.g4 #3880

wants to merge 3 commits into from

Conversation

Xenios91
Copy link
Contributor

bug fix for line labels, allowing line labels to have white space after the colon

bug fix for line labels, allowing line labels to have white space after the colon
@Beakerboy
Copy link
Contributor

I just submitted a VBA pull request as well. #3881

Let me know if you have any advice on what to do or not to do regarding ANTLR and Linting VBA.

@kaby76
Copy link
Contributor

kaby76 commented Dec 11, 2023

The TypeScript target environment somehow changed. TypeScript on my machine now also doesn't work.

I have to change the templates for TypeScript. See Issue #3882 . Once done, you can fast forward/rebase to include the change and try again. My fix is in #3883.

Copy link
Member

@KvanTTT KvanTTT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add/extend an example that covers suggested changes to the examples subdirecotry.

@KvanTTT KvanTTT added vb vba and removed vb labels Dec 12, 2023
@Xenios91
Copy link
Contributor Author

Please add/extend an example that covers suggested changes to the examples subdirecotry.

The sub directory should already have line numbers i added last patch of mine, this one just fixes them since before if your line numbers had white space after them they didnt parse correctly.

@Beakerboy
Copy link
Contributor

@Xenios91 would it be worthwhile adding some whitespace to one of the line numbered lines, to demonstrate that that case fails under the existing grammar, but passes under the new grammar?

@teverett
Copy link
Member

teverett commented Jan 6, 2024

@KvanTTT good to merge this?

@KvanTTT
Copy link
Member

KvanTTT commented Jan 8, 2024

No, I suggest adding a test data in the way @Beakerboy suggested.

@Beakerboy
Copy link
Contributor

Beakerboy commented Jan 19, 2024

@Xenios91 The VBA editor that ships with excel automatically strips trailing whitespace out of a line label. If the line label is on the same line as a statement, there can be whitespace between them. Should the whitespace be considered part of the label, or as a separator between different contexts?

Using underscore for trailing whitespace:

Function Foo(x)
    If x > 1 GoTo line2
'cannot produce this in the IDE
line1:__
    Foo = 5
    GoTo final

'This is valid
line2:   Foo = 1
final:
End Function

@Xenios91
Copy link
Contributor Author

Xenios91 commented Jan 20, 2024

@Xenios91 The VBA editor that ships with excel automatically strips trailing whitespace out of a line label. If the line label is on the same line as a statement, there can be whitespace between them. Should the whitespace be considered part of the label, or as a separator between different contexts?

Using underscore for trailing whitespace:

Function Foo(x)
    If x > 1 GoTo line2
'cannot produce this in the IDE
line1:__
    Foo = 5
    GoTo final

'This is valid
line2:   Foo = 1
final:
End Function

Right, I have just come across documents in the wild that it fails to parse them correctly when there is this white space, so while it should be removed, it seems like it is not. I speculate this may be due to a document being generated by something like apache poi or another library where the vba formatter doesnt clean it up. Id say for now maybe just an optional character for line labels, as making it a separator may impact other parts of the grammar. The more I think about it, i believe a lot of the parsing errors I am seeing are from automated techniques to generate documents where that VBA cleanup doesnt occur.

@Beakerboy
Copy link
Contributor

being generated by something like apache poi …

I had not heard of Apache POI, so I looked it up. I’ve been working on my own OLE file writer for quite a while. I wish I knew this was out there. Thanks!

@Xenios91
Copy link
Contributor Author

being generated by something like apache poi …

I had not heard of Apache POI, so I looked it up. I’ve been working on my own OLE file writer for quite a while. I wish I knew this was out there. Thanks!

oh yeah its pretty nice!

@Xenios91
Copy link
Contributor Author

test added

@Xenios91 Xenios91 requested a review from KvanTTT January 26, 2024 02:24
@@ -9,4 +9,5 @@ Dim Number, MyString
MyString = "Number equals 2"
3:
Debug.Print MyString
4: Debug.Print "TEST"
Copy link
Contributor

@kaby76 kaby76 Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't test the changed grammar rule "lineLabel". There's not a single test that has "lineLabel".

01/26-05:00:52 ~/issues/g4-3880/vba/Generated-CSharp
$ trparse ../examples/*.bas | trxgrep ' //lineLabel' | trtext -c
CSharp 0 ../examples/example09_typeHints.bas success 0.3759407
CSharp 0 ../examples/example1.bas success 0.0414117
CSharp 0 ../examples/example2operators.bas success 0.1742185
CSharp 0 ../examples/example3erase.bas success 0.0385953
CSharp 0 ../examples/example4fixedstring.bas success 0.006915
CSharp 0 ../examples/example5property.bas success 0.0179688
CSharp 0 ../examples/example6dateliteral.bas success 0.0156073
CSharp 0 ../examples/example7linecontinuation.bas success 0.3410573
CSharp 0 ../examples/example8_lineNumbers.bas success 0.3330462
../examples/example09_typeHints.bas:0
../examples/example1.bas:0
../examples/example2operators.bas:0
../examples/example3erase.bas:0
../examples/example4fixedstring.bas:0
../examples/example5property.bas:0
../examples/example6dateliteral.bas:0
../examples/example7linecontinuation.bas:0
../examples/example8_lineNumbers.bas:0
01/26-05:03:05 ~/issues/g4-3880/vba/Generated-CSharp
$

Here's a "coverage heat map" for the tests (rename to cover.html and view in an internet browser).
cover.html.txt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if there was a way to see coverage. Unfortunately, removing the ‘.txt’ just gives a 404 error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Download cover.html.txt (I just tried the link, it works). Save it on disk. Rename cover.html.txt to cover.html in a command-line shell. Open a browser and click to open cover.html.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaby76 Do you know if there is a way to see the coverage file from a working branch on GitHub? I’m adding examples and want to see which tokens I’m missing along the way.
I’m used to sending coverage reports to coveralls.io.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage is not run in the Github Actions build because it is time consuming. #3624

You'll have to run it locally on your machine.

@Beakerboy
Copy link
Contributor

Beakerboy commented Jan 27, 2024

does the block section need to be changed to

block
    : block-element+
    ;

block-element
    : lineLabel endOfLine
    | lineLabel? blockStmt (endOfStatement blockStmt)* endOfStatement
    

and then remove lineLabel from blockStmt. I feel this should ensure that the line label can only occur without leading whitespace, and also that is properly identified when it is alone on a line and when it is followed with statements.

can line numbers also appear on the lame line as other statements. The grammar currently states that there must be a line break at the end. Either way, lineNumber would probably be handled similarly.

@Beakerboy
Copy link
Contributor

Beakerboy commented Jan 27, 2024

One issue I see…how is the parser supposed to know that when it sees:

foo: x=1

that it is a line labeled ‘foo’ versus a call to the foo subroutine, followed by a let statement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants