Add DefaultVisit method to the visitor interface and class#13258
Add DefaultVisit method to the visitor interface and class#13258TravisEz13 merged 5 commits intoPowerShell:masterfrom
DefaultVisit method to the visitor interface and class#13258Conversation
vexx32
left a comment
There was a problem hiding this comment.
Looks good to me!
I have one question pertaining to the default implementation of DefaultVisit() -> is it better to return null there or to throw an exception?
I think if you target something like PSStd, you won't be able to override the default (you can't see the method since it's only in PS 7.1). Handling a null in that case is much simpler than an exception. |
src/System.Management.Automation/engine/parser/SymbolResolver.cs
Outdated
Show resolved
Hide resolved
| { | ||
| /// <summary/> | ||
| public virtual object VisitErrorStatement(ErrorStatementAst errorStatementAst) { return null; } | ||
| public virtual object DefaultVisit(Ast ast) => null; |
There was a problem hiding this comment.
Why do we need this if we already have it in the interface?
There was a problem hiding this comment.
Default implementation of an interface method behaves like an explicit implementation, meaning that it's not visible to an instance of DefaultCustomAstVisitor, and you have to cast it to ICustomAstVisitor in order to call the method.
So, without this explicit implementation, you will have to write code like this:
public virtual object VisitErrorStatement(ErrorStatementAst errorStatementAst) => ((ICustomAstVisitor)this).DefaultVisit(errorStatementAst);
There was a problem hiding this comment.
Thanks for clarify!
you will have to write code like this
I believe this is implied when working with interfaces.
Also I think now (in the PR context) we want to prefer interfaces.
It worries me that we are creating two entities that were supposed to be one.
|
I'd add Obsolete attribute because:
|
It's a breaking change with little or no value. We still need to keep this class around because it's part of PS 5.1 and 7.0, and thus we will have to keep updating it to avoid confusion to reading the code.
Why would you assume this? It's very possible the user code has a visitor derives from |
|
🎉 Handy links: |
|
@daxian-dbw @iSazonov If this needs documentation then it needs to be added to /// comments in the source code. |
PR Summary
Fix #12070
DefaultVisittoAstVisitor,ICustomAstVisitorandDefaultCustomAstVisitor.=> DefaultVisit(ast)to all existing methods inICustomAstVisitorandICustomAstVisitor2.AstVisitor,AstVisitor2,DefaultCustomAstVisitorandDefaultCustomAstVisitor2to be=> DefaultVisit(ast).PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.