[Bug] Top module Const Declaration causes <missing EOF token> error

[DecimalTurn]

Describe the bug
There is a parsing error with Const at the top of the module

To Reproduce

Code

Attribute VB_Name = "Test"

Option Explicit

Private Const IMPORT_DELAY As String = "00:00:05"

Note that removing the "Private" access modifier fixes the error. However the following still causes an error:

Attribute VB_Name = "Test"

Option Explicit

Const IMPORT_DELAY As String

Expected behavior
There shouldn't be an error

[DecimalTurn]

Thanks, that was quick! I had actually started to look into debugging antlr to see if I could find the cause of the issue.

While looking at this video, they suggest to skip whitespaces, would that be helpful for debugging such issues to avoid inflating the tree?

[SSlinky]

Because I knew exactly what I had done once I saw your screen shot. 😄

I did look at doing this a while back but it broke some other things at the time. The one thing that immediately springs to mind is that white space is significant to some things, e.g., line continuations, but that could (probably) be overcome by rule order like the below.

LINECONTINUATION: WS UNDERSCORE WS? '\r'? '\n';
WHITESPACE: WS --> skip;

fragment WS: ([ \t\u0019\u3000])+;

The second thing is that it would be nice to implement the same sort of silent adjustments that VBA does. Auto capitalisation of variables. Removing extraneous spacing or adding it where it's missing. And it's this last one that would be a lot easier if the rules expected spaces. Otherwise, I believe you'd be reduced to inspecting the context ranges for everything.

I do agree life would be simpler without them.

On the topic of parsing though, I somehow managed to implement synchronous asynchronous parsing. That is, I have implemented async parsing with cancellation tokens and such, but they do nothing because the listener pattern is not async. The cancellation events fire after parsing has been completed anyway. So the plan is to move to the more manual visitor pattern, but before I can do that, antlr4ng doesn't even generate a visitor for the main parser for reasons I've not been able to yet determine. So we're stuck with sync for now...

[SSlinky]

Never mind, I found it... the alphabet strikes again...

[DecimalTurn]

I did look at doing this a while back but it broke some other things at the time. The one thing that immediately springs to mind is that white space is significant to some things, e.g., line continuations, but that could (probably) be overcome by rule order like the below.

I see, thanks for the explanation.

Never mind, I found it... the alphabet strikes again...

Oh nice, async parsing would make things smoother I guess. Personnally, I didn't feel any sluggishness while using the extension, but I haven't worked on very large VBA projects with it yet.

[SSlinky]

Yeah, it's not particularly sluggish, but it could be better. Mostly it catches up when you pause for a bit so you only notice it when you're actually watching the log output.

Senipah mentioned the file limitation size almost immediately, so people are working with large files. The good news is that I've just mocked up a visitor that does nothing at all except visit every child recursively, so essentially the same as the listener. Cancellation seems to work out of the box. Just a matter of redoing the functionality already built into the listener and wiring up the walk functionality manually.

export class VbaVisitor extends vbaVisitor<Promise<void>> {
	private _logger?: Logger;
	private nodesProcessed = 0;

	get logger(): Logger {
		if (!this._logger) {
			this._logger = Services.logger;
		}
		return this._logger;
	}

	constructor(private cancellationToken: CancellationToken) {
		super();
		this.cancellationToken.onCancellationRequested((e) => {
			this.logger.debug(`Cancellation requested at ${this.nodesProcessed} nodes processed.`);
		});
	}

	visit = async (tree: ParseTree): Promise<void>  => {
		if (this.cancellationToken.isCancellationRequested) {
			return;
		}
		this.nodesProcessed += 1;
		await this.yieldThread();
		await super.visit(tree);
		await this.visitChildren(tree);
	};

	async visitChildren(node: ParseTree) {
		for (let i = 0; i < node.getChildCount(); i++) {
			const child = node.getChild(i);
			if (child) await this.visit(child);
		}
	}

	visitStartRule = async (ctx: StartRuleContext) => {
		if (this.cancellationToken.isCancellationRequested) {
			return;
		}
		await this.yieldThread();
		this.logger.debug(`Visited visitStartRule`);
		await this.visit(ctx.module());
	};


	private async yieldThread(): Promise<void> {
		await new Promise(res => setImmediate(res));
	}
}

[SSlinky]

Related to this issue somewhat: I managed to fix the "Missing " diagnostic. Now when it encounters something it has no suggestions for, i.e., it thinks the only next valid token is EOF, it will eat tokens until either actual EOF or the end of the line. It will then generate an error node which is reported through as a diagnostic advising invalid syntax. Required refactoring base element so it was quite a bit more effort than I expected. Result is a much better diagnostic though.

[DecimalTurn]

That's great news, it's much clearer indeed.

[SSlinky]

Well it works, but once it has advised the issue, parsing simply ends there. After faffing around with it for far longer than I would have liked, I'm admitting defeat on the recovery for now. Unless I go down the route of writing a bespoke parser (which I eventually may do).

Sub Foo()
   Bad Syntax
End Sub

We hit a NoViableAlternative exception at the end of the line above the bad line. This puts the parser into recovery so we consume \r\n and tell it we're all good. It happens immediately again on the white space because we're still parsing the bad statement. Then we go into recoverInline which consumes the line and returns the error token which the listener handles by adding an error diagnostic (causing the red line).

The problem is that suddenly we're at EOF and I have no idea why. Nothing else is parsed, and indeed Foo isn't even fully parsed. The symbol is returned to the document outline but the error appears to be outside its scope. Additionally, the folding range only goes up to the error line.

It's definitely better than it was, it's just not amazing.