fix: reorder memberDeclaration alternatives to prioritize class over field

[mshanemc]

This fixes an issue where virtual inner classes were incorrectly parsed as fields.

The grammar now tries class/interface/enum declarations before field declarations, giving priority to specific keywords (CLASS, INTERFACE, ENUM) over the general typeRef pattern used in field declarations.

repro code from one of our test cases y'all might recognize;

public class FFLIB_MetadataService{
  public virtual class GlobalPicklistValue extends Metadata {
    public String color;
    public Boolean default_x;
    public String description;
    public Boolean isActive;
  }

  public virtual class Metadata {
    public String fullName;
  }
}

am open to other approaches (ex: trying to make the selector for fieldDeclaration tighter so that it looks ahead and excludes class keyword but I'm not good enough at antlr to know how to do that.

[pwrightcertinia]

Build would most likely have passed - but another one of the sample repos has been deleted off github. It happens, not to worry.

I don't think there are fancy rules like that in antlr so this would be the best way.

In some ts visitor code I had to hand, I was struggling to repro this though - I got GlobalPicklistValue and Metadata as ClassDeclarationContexts. Is this Java or JS target going wrong? I would have to write some unit tests in here to investigate properly. It may also have been a bug in antlr itself since I was testing on antlr 4.13.2 (apex-parser v5 beta) and apex-parser v4 uses antlr 4.9.0 I think.

[mshanemc]

Build would most likely have passed - but another one of the sample repos has been deleted off github. It happens, not to worry.

I don't think there are fancy rules like that in antlr so this would be the best way.

In some ts visitor code I had to hand, I was struggling to repro this though - I got GlobalPicklistValue and Metadata as ClassDeclarationContexts. Is this Java or JS target going wrong? I would have to write some unit tests in here to investigate properly. It may also have been a bug in antlr itself since I was testing on antlr 4.13.2 (apex-parser v5 beta) and apex-parser v4 uses antlr 4.9.0 I think.

it's from the js.

[pwrightcertinia]

So I still can't repro it with this sample, on both v4 branch and main - I get class declarations. Quick test I was trying in ApexParserTest.ts:

test("Class declaration", () => {
  const [parser, errorCounter] = createParser(`
public class FFLIB_MetadataService {
  public virtual class GlobalPicklistValue extends Metadata {
    public String color;
    public Boolean default_x;
    public String description;
    public Boolean isActive;
  }

  public virtual class Metadata {
    public String fullName;
  }
}`);

  const context = parser.typeDeclaration();

  expect(errorCounter.getNumErrors()).toEqual(0);

  const pick = context.classDeclaration().classBody().classBodyDeclaration(0);
  expect(pick.memberDeclaration().children.length).toEqual(1);
  expect(pick.memberDeclaration().children[0]).toBeInstanceOf(
    ClassDeclarationContext
  );

  const meta = context.classDeclaration().classBody().classBodyDeclaration(1);
  expect(meta.memberDeclaration().children.length).toEqual(1);
  expect(meta.memberDeclaration().children[0]).toBeInstanceOf(
    ClassDeclarationContext
  );
});

[mshanemc]

good point; from what I've seen, I'll get a correct parse from doing the whole file via typeDeclaration.

I think our code is doing visitor-pattern stuff, so that when the inner classed is parsed using .memberDeclaration that's how it's hitting the fieldDefinition.

test("Language server incremental parsing fails for virtual classes", () => {
  // Scenario: LS tries to parse "public virtual class..." as isolated memberDeclaration
  // Expected: 0 errors (works in full context, should work in isolation too)
  // Received: 1 error  (fails due to grammar order when parsed in isolation)
  const parser = ApexParserFactory.createParser(
    "public virtual class TestClass { }"
  );
  const errorCollector = new ErrorCollector();
  parser.addErrorListener(errorCollector);

  const memberContext = parser.memberDeclaration();

  // Log the actual parse error details
  console.log("Number of errors:", errorCollector.getNumErrors());
  if (errorCollector.getNumErrors() > 0) {
    console.log("Parse errors:", errorCollector.getErrors());
  }

  expect(errorCollector.getNumErrors()).toEqual(0);
  expect(memberContext.classDeclaration()).toBeTruthy();
});

[pwrightcertinia]

Ah, thanks for confirming and providing the example.

This is happening because the sample text is in fact modifier* memberDeclaration going by the grammar rules.

This is why I couldn't repro it either, because typeDeclaration and classBodyDeclaration both have that rule separating the modifiers out. I would say if you are directly parsing each class member like this (because you know you are in a class), then it should be parsed using classBodyDeclaration() not memberDeclaration(). Then you can access the memberDeclaration and modifiers separately on the returned context.

[mshanemc]

I'm speculating the issue it that we're not choosing what to parse it with, but as the parser visits the things at the top level of the outer class, it'll hit a memberDeclaration, incorrectly decide it's a field because that matches, and then call our Listener's enterFieldDeclaration instead of enterClassDeclaration.

We can create some crazy workaround code to say, "oh, if the text includes class then you're in the wrong place, go back out and do it with the other one, oh, and also handle the exit and also mutate the Scope stuff to keep everything correct" but it would be better. [I have a PR open for that in our repo, and it does fix the issue, but putting class ahead of field does it much better.

[pwrightcertinia]

I'll accept this because it won't do any harm that I can see and if it fixes your problem, great (the best kind of test). I will also fix the system test (bear with) and release it to v4 just in case.

I understand the original scenario too, makes sense that a broad rule could lead to unintended behaviour.

---

Clarifying my previous comment, I was trying to point out that this:

public virtual class TestClass { }

would not be parseable with parser.memberDeclaration() because of the presence of the modifiers, and you get an error to that effect. To fix this, you would need to parse it with parser.classBodyDeclaration()/parser.typeDeclaration() etc which include the modifier* rule to separate out the public/virtual modifiers.

These lines:

class TestClass { }
String myField;

would each be parseable with memberDeclaration() - and tests for these pass with the correct type selected. So as you've previously said, we can only assume its somewhere else evaluating the rules incorrectly, interpreting class as a typeRef.