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 a invalid context. It will be revised to allow collection of all possible invalid contexts, which is why it's a
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 whereasInvalidContexts
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 aIEnumerable
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 thebase.XXX
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);
}
}