Antlr Visitor with complex result

Question

I'm currently helping out on Rubberduck open source project, specifically on the Extract Method functionality.

I wrote a purpose-built visitor to assist with validating the selection. Basically we have to ensure that when the user select a piece of VBA code, the selection only encompasses a set of complete statements that are also self-contained. By "self-contained", we mean:

• it is within a single procedure
• does not contain any control-of-flow statements such as GoTo, On Error, Exit XXX that would cause the execution to leave the selection entirely or jump into middle of the selection
• each statements are complete (e.g. not partially selected)

Note there are 2 issues already present that can be ignored:

• Hard-coded literals reporting error. Those need to be in the resource files and will be moved later. I need more thoughts on what I need to collect before I will refactor those.
• Currently, the visitor stops as soon as it encounters an invalid context. It will be revised to allow collection of all possible invalid contexts, which is why it's an IList rather than a single instance.

Specific questions to assess:

• To avoid storing a state in the visitor, I opted to create a class ExtractMethodValidationResult which importantly has 2 separate lists. ValidContexts gives me all contexts (of one type) that are valid for a user's selection whereas InvalidContexts gives me any other contexts (regardless of type) where there might be a problem. I then pass that into the generic visitor.In a typical sample, a generic visitor will take an IEnumerable of some certain contexts and return that. This is quite different as we take a class and return the results. As a result, the process will create many new instances of the class which is then aggregated. Are there any potential pitfalls with the approach?
• I'm not exactly thrilled with the high amount of duplicative code, notably with those that collects invalid contexts. Can this be done better?
• I'm a little fuzzy whether I need to return base.XXX when visiting contexts that are considered invalid. In case when we visit a invalid context that isn't a part of the selection, we execute the base.XXX

• I'm a bit stumped on where the responsibility of the visitor starts and stops. Right now it returns a raw set of contexts which requires additional processing outside the visit. I'm considering whether I should override the Visit method to something similar to this:

   public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> Visit(IParseTree tree)
   {
   var result = base.Visit(tree);
   return PostProcessing(result);
   }
  

The advantage would be that the visitor now returns a result that needs no more additional processing but it also takes on the responsibility of analyzing the returned contexts from the base.Visit. Have I then violated the SRP here? Thoughts?

Any other feedbacks & suggestions welcome.

Result code:

 private class ExtractMethodValidationResult<Result> where Result : ParserRuleContext
 {
 public IList<Result> ValidContexts { get; set; }
 public IList<Tuple<ParserRuleContext, String>> InvalidContexts { get; set; }
 public ExtractMethodValidationResult()
 {
 ValidContexts = new List<Result>();
 InvalidContexts = new List<Tuple<ParserRuleContext, String>>();
 }
 public ExtractMethodValidationResult(ParserRuleContext InvalidContext, String ErrorMessage) : this()
 {
 InvalidContexts.Add(Tuple.Create(InvalidContext, ErrorMessage));
 }
 }

Visitor code:

private class ExtractMethodValidatorVisitor : VBAParserBaseVisitor<ExtractMethodValidationResult<VBAParser.BlockStmtContext>>
 {
 private readonly QualifiedSelection _qualifiedSelection;
 public ExtractMethodValidatorVisitor(QualifiedSelection qualifiedSelection)
 {
 _qualifiedSelection = qualifiedSelection;
 }
 protected override ExtractMethodValidationResult<VBAParser.BlockStmtContext> DefaultResult => new ExtractMethodValidationResult<VBAParser.BlockStmtContext>();
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitBlockStmt([NotNull] VBAParser.BlockStmtContext context)
 {
 var children = base.VisitBlockStmt(context);
 if (children.InvalidContexts.Count == 0)
 {
 children.ValidContexts.Add(context);
 }
 return children;
 }
 protected override ExtractMethodValidationResult<VBAParser.BlockStmtContext> AggregateResult(ExtractMethodValidationResult<VBAParser.BlockStmtContext> aggregate, ExtractMethodValidationResult<VBAParser.BlockStmtContext> nextResult)
 {
 if (aggregate.InvalidContexts.Count == 0)
 {
 if (nextResult != null)
 {
 aggregate.ValidContexts = aggregate.ValidContexts.Concat(nextResult.ValidContexts).ToList();
 aggregate.InvalidContexts = aggregate.InvalidContexts.Concat(nextResult.InvalidContexts).ToList();
 }
 }
 return aggregate;
 }
 protected override bool ShouldVisitNextChild(IRuleNode node, ExtractMethodValidationResult<VBAParser.BlockStmtContext> currentResult)
 {
 // Don't visit any more children if we have any invalid contexts
 return (currentResult.InvalidContexts.Count == 0);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitErrorStmt([NotNull] VBAParser.ErrorStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an Error statement.");
 }
 return base.VisitErrorStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitEndStmt([NotNull] VBAParser.EndStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an End statement.");
 }
 return base.VisitEndStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitExitStmt([NotNull] VBAParser.ExitStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains an Exit statement");
 }
 return base.VisitExitStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitGoSubStmt([NotNull] VBAParser.GoSubStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a GoSub statement");
 }
 return base.VisitGoSubStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitGoToStmt([NotNull] VBAParser.GoToStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a GoTo statement");
 }
 return base.VisitGoToStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnErrorStmt([NotNull] VBAParser.OnErrorStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On Error statement");
 }
 return base.VisitOnErrorStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitIdentifierStatementLabel([NotNull] VBAParser.IdentifierStatementLabelContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Line Label statement");
 }
 return base.VisitIdentifierStatementLabel(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitCombinedLabels([NotNull] VBAParser.CombinedLabelsContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Line Label statement");
 }
 return base.VisitCombinedLabels(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnGoSubStmt([NotNull] VBAParser.OnGoSubStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On ... GoSub statement");
 }
 return base.VisitOnGoSubStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitOnGoToStmt([NotNull] VBAParser.OnGoToStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a On ... GoTo statement");
 }
 return base.VisitOnGoToStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitResumeStmt([NotNull] VBAParser.ResumeStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Resume statement");
 }
 return base.VisitResumeStmt(context);
 }
 public override ExtractMethodValidationResult<VBAParser.BlockStmtContext> VisitReturnStmt([NotNull] VBAParser.ReturnStmtContext context)
 {
 if (_qualifiedSelection.Selection.Contains(context))
 {
 return new ExtractMethodValidationResult<VBAParser.BlockStmtContext>(context, "Extract method cannot extract methods that contains a Return statement");
 }
 return base.VisitReturnStmt(context);
 }
 }

• Mathieu Guindon: Excluding control flow statements makes for a simpler implementation, but eventually if a GoTo jump refers to a line label/number that's within the selection, the selection should be deemed valid: the visitor will eventually need to use a DeclarationFinder to find the declaration for the line label/number being referred to by a GoTo statement, and then verify that the line label/number isn't being jumped-to by statements that aren't within the selection.
• this: That's exactly correct. This will be addressed in a future iteration. For right now it's more important to have a functional Extract Method.
• dfhwze: When I read "To avoid storing a state in the visitor", I'm tempted to ask: Could you use a listener instead?

Answer 1

The way Antlr visitors and listeners work, there's little we can do to avoid redundant code - you've done quite a good job at keeping the redundancies minimal.

Essentially the logic is the same in all overrides: return a new ExtractMethodValidationResult when context is contained in the user's selection, or the no-op base return value otherwise.

You could remove the dots and extract a generic IsSelected<TContext> function to turn this:

if (_qualifiedSelection.Selection.Contains(context)
{
 return ...
}
return ...

into a single return statement:

return IsSelected(context) 
 ? new ExtractMethodValidationResult<SomeParserRuleType>(context, RubberduckUI.SomeResourceKey)
 : base.VisitSomeNode(context);

It would be nice to be able to infer the generic type parameter from the context, and note the use of RubberduckUI resources instead of a hard-coded string literal.

That constructor's signature doesn't look right:

public ExtractMethodValidationResult(ParserRuleContext InvalidContext, String ErrorMessage) : this()

Parameter names should be camelCase, and constructor chaining usually go the other way around (least parameters calling overloads with more parameters), but I can see why it was done this way.

Encapsulation is broken here:

 public IList<Result> ValidContexts { get; set; }
 public IList<Tuple<ParserRuleContext, String>> InvalidContexts { get; set; }

These properties shouldn't expose setters, these references should be immutable. You could use private readonly backing fields, or since Rubberduck is now written in C#7, leverage truly immutable get-only autoproperties:

public IList<TResult> ValidContexts { get; } = new List<TResult>();
public IList<Tuple<ParserRuleContext, string>> InvalidContexts { get; } = new List<Tuple<ParserRuleContext, string>>();

This would remove the need to chain the parameterized constructor overload; you'd still need to have an explicit default constructor though.

Note the use of the string language keyword/alias for System.String; Rubberduck's code base uses language aliases whenever they are available.

I'm not sure exposing Tuple<ParserRuleContext, string> makes the greatest API for the consuming class. The class itself being private tells me this would be a nested class and as such, some sort of "implementation detail" of something bigger (which, indeed it is) - so it might not matter much. But if the class were public or internal, I'd avoid exposing a Tuple like this.

Might be just a copy/paste artifact, but watch brace position:

private class ExtractMethodValidatorVisitor : VBAParserBaseVisitor<ExtractMethodValidationResult<VBAParser.BlockStmtContext>>
 {

Should be:

private class ExtractMethodValidatorVisitor : VBAParserBaseVisitor<ExtractMethodValidationResult<VBAParser.BlockStmtContext>> 
{

This check works against IList<T>:

if (children.InvalidContexts.Count == 0)

Not wrong in itself, but then further encapsulating the lists and eventually exposing the contexts as IEnumerables would make that code require adjustments. I'd go with LINQ here, and work against the more general-purpose IEnumerable<T> interface:

if (!children.InvalidContexts.Any())

The List<T> is an implementation detail anyway.

---

I need to learn to leverage Antlr visitors myself, to I hope another reviewer will be commenting on the general approach.

• this: Regarding your comments about encapsulation being broken --- this is not easy (or at least not obvious to me) to fix if you look at the AggregateResult method where I assign the result of Concat() to the fields. Originally I only did the Concat() but was getting exceptions about the lists being empty. Thus, assigning the result of Concat() was necessary to ensure the list is kept populated. If you can suggest another way, I'm all eyes.