Powershell: Ast visitor APIs missing "DefaultVisit"

[SeeminglyScience]

There was supposed to be a DefaultVisit type of method added to ICustomAstVisitor2 (with a default implementation) and AstVisitor2. It looks like that was missed.

/cc @rjmholt @daxian-dbw

[iSazonov]

For reference #10859.

[vexx32]

@rjmholt @joeyaiello @daxian-dbw this should probably be prioritised for 7.1 if not earlier; it will significantly complicate solutions that need to be compatible with more than one version, and the problem will get worse over time. (see also: https://twitter.com/IISResetMe/status/1286005066311372800?s=20)

[rjmholt]

Original discussion: https://github.com/PowerShell/PowerShell/pull/9849#discussion_r291581467

If you've got links to other discussions on this, please feel free to add them

[rjmholt]

This has been in the code since https://github.com/PowerShell/PowerShell/pull/10367:

https://github.com/PowerShell/PowerShell/blob/a6bd47f03155eeca52facd4ae5721bfaa41fe6ac/src/System.Management.Automation/engine/parser/AstVisitor.cs#L176-L180

[SeeminglyScience]

Oh it's...private?

https://github.com/PowerShell/PowerShell/blob/a6bd47f03155eeca52facd4ae5721bfaa41fe6ac/src/System.Management.Automation/engine/parser/AstVisitor.cs#L153

[daxian-dbw]

@SeeminglyScience Do you think it should be public?

[SeeminglyScience]

@SeeminglyScience Do you think it should be public?

Yeah sorry I guess I didn't make that clear enough. The big draw of it is that it lets the implementer decide how to deal with new AST types instead of blowing up. If the implementer can't implement that method then it's the same as having the default implementation be => null.

[rjmholt]

I think one question in making it public is, should it be the default implementation of all methods? I think it will be confusing if there's a public DefaultVisit() method that only works for some methods

[SeeminglyScience]

I think one question in making it public is, should it be the default implementation of _all_ methods? I think it will be confusing if there's a public DefaultVisit() method that only works for some methods

Good idea, yeah that would be great.

[daxian-dbw]

I agree the DefaultVisit should be public and it should be added to AstVisitor2 as well (_but, should a public version of it added to DefaultCustomAstVisitor and DefaultCustomAstVisitor2 instead?_).
But I'm concerns about having all methods in ICustomAstVisitor2 and ICustomAstVisitor have default implementations.

In my opinion, interface is supposed to be as explicit as possible, and the abstract classes DefaultCustomAstVisitor and DefaultCustomAstVisitor2 are provided in case you only want to implement a few selected methods. If we choose to do this then DefaultCustomAstVisitor and DefaultCustomAstVisitor2 will become redundant and needless.

Comparing to DefaultCustomAstVisitor and DefaultCustomAstVisitor2. The benefit, I guess, is that a user can derive from ICustomAstVisitor2 and another class, and only implements a few selected methods. The drawback is, in my opinion, the compiler will not notify you when you miss implementations for any of those methods in ICustomAstVisitor2 and ICustomAstVisitor because you basically make the interface an abstract class.

[SeeminglyScience]

But I'm concerns about having all methods in ICustomAstVisitor2 and ICustomAstVisitor have default implementations.

(...)

The drawback is, in my opinion, the compiler will not notify you when you miss implementations for any of those methods in ICustomAstVisitor2 and ICustomAstVisitor because you basically make the interface an abstract class.

Yeah that's fair. There's pros and cons to each and I don't really have a strong opinion either way, so whichever one y'all want to go with is fine with me.

[joeyaiello]

I hear you, @vexx32. This is especially important if we want to keep adding things to the language. I'll go ahead and throw 7.1 - Consider milestone on it

I think I saw @IISResetMe hit this in the wild too maybe? (If it was someone else, apologies to both parties, it was a fleeting tweet.)

[iSazonov]

But I'm concerns about having all methods in ICustomAstVisitor2 and ICustomAstVisitor have default implementations.

I'd prefer to have all default implementations. It simplify scripts and binaries too. I do think so because developers know that they want and, mainly, cover their code by tests.

As for DefaultCustomAstVisitor and DefaultCustomAstVisitor2, I'd prefer to have modern interfaces and mark the classes as obsolete. This simplify API and its uses.

[daxian-dbw]

@lzybkr Can you please share your opinion?

[lzybkr]

It seems the question is - what use is DefaultCustomAstVisitor given the existence of language support for default implementations?

I think there are 2 primary use cases for ICustomAstVisitor:

1. Adding "virtual" methods to the Ast hierarchy from outside System.Management.Automation.
2. Actually traverse the Ast tree in some custom way.

The second scenario is where there is value in not having a default implementation for every method - this way implementations fail to compile if a new method is added.

That said - it's a breaking api change to introduce new methods to an interface, hence we have ICustomAstVisitor2 - and implementations of ICustomAstVisitor never get a chance to handle the new methods/nodes.

So in my ideal world, I would keep ICustomAstVisitor and DefaultCustomAstVisitor, never have introduced the 2 variants, and clients would be forced to recompile when targeting newer versions,

But in practice, that seems slightly excessive. I think there are few implementations where this extra build time guarantee might be helpful and these implementations are likely actively maintained.

So in summary, the proposal to obsolete DefaultCustomAstVisitor isn't my favorite idea, but it seems acceptable all things considered.

[rjmholt]

Yeah I think in a world where:

• Interfaces already have members that don't appear everywhere, and
• Interfaces can be used as mixins (makes me think a different declaration would be nice to unlock these behaviours to force our hand)

We've already lost the compiler check on implementations; for that we need ICustomAstVisitor3 with no default implementations.

So we've lost the important distinguishing property between the interface and the abstract class there, and we seem to be investing in the interface (because that's where breaking changes will occur if we do nothing), so we should just make all its methods use the default.

Otherwise we end up with an API that ages more and more badly as we add new AST types, since there will be more and more methods that happen to have a default because the defaults were added at some language version; the nice thing about ICustomAstVisitor[n] is that we know there are n versions of the AST API, but with an interface it will one day just look like some methods use the default and others don't.

[daxian-dbw]

OK, it sounds there's a consensus :) I will go submit a PR that

1. Make DefaultVisit public in ICustomAstVisitor
2. Make all methods in ICustomAstVisitor and ICustomAstVisitor2 have default implementation => DefaultVisit(ast)
3. Do the same to AstVisitor and AstVisitor2.
4. ~Mark DefaultCustomAstVisitor and DefaultCustomAstVisitor2 obsolete.~

[daxian-dbw]

The PR is out #13258. I decided to not mark DefaultCustomAstVisitor and DefaultCustomAstVisitor2 obsolete for 3 reasons:

1. Even if we use error: false, it's a breaking change if a build is configured to be "treat warning as error"
2. Update code that use DefaultCustomAstVisitor to ICustomAstVisitor is not simply change the type to be derived, but you also need to change each method to remove the override keyword.
3. Default implementation of an interface is treated as explicit implementation, meaning that for A : ICustomAstVisitor, unless A explicitly implement a method, that method is hidden from an instance of A (unless casting the instance to ICustomAstVisitor). Usually such a visitor will be passed to Ast.Accept which will be cast to ICustomAstVisitor anyway, so I'm uncertain how many uses will directly call the methods on the instance of A. But it's better to not break that.

[iSazonov]

Even if we use error: false, it's a breaking change if a build is configured to be "treat warning as error"

Developers can suppress the warning. Notification is better because developers can skip the warning in docs.

Although we have a separate discussion, I think we should take the next steps in time:

• update docs
• add Obsolete attribute with warning
• add Obsolete attribute with error

[rjmholt]

Thinking on it a bit more, I'm leaning on the side of adding the obsolete attribute:

• It's a dev-time thing, so it's not going to suddenly appear for lots of people in the middle of doing something unrelated
• It's a warning, and developers deliberately choose whether that breaks their build or not, they can choose to suppress it too
• It's pretty easy to address this warning; migrating to the preferred API is not a major cross-cutting shift, just a move from one implementing class to another that still gets used and passed around the same way
• We need to take every opportunity to notify developers about what we expect from them, otherwise we may cause much subtler, nastier things down the track; we must be cruel to be kind
• Developers targeting 7.1 are targeting a higher-cadence release, and are probably set up to manage changes like this. And I think part of the reason for targeting a newer, non-LTS release is to pick up changes and adapt to them quickly

[daxian-dbw]

Default implementation of an interface is treated as explicit implementation, meaning that for A : ICustomAstVisitor, unless A explicitly implement a method, that method is hidden from an instance of A (unless casting the instance to ICustomAstVisitor). Usually such a visitor will be passed to Ast.Accept which will be cast to ICustomAstVisitor anyway, so I'm uncertain how many uses will directly call the methods on the instance of A. But it's better to not break that.

^ I want to call out again this behavior change MAY affect user's code.
Today, for A: DefaultCustomAstVisitor, the user can run objOfA.VisitSwitchStatement(xxx) even if A doesn't override/implement VisitSwitchStatement, but with A : ICustomAstVisitor the user won't be able to do so.

I see the work to add DefaultVisit an enhancement to allow a developer to do things more easily, no matter the developer is using ICustomAstVisitor or DefaultCustomAstVisitor.

Personally, I still think that adding the obsolete attribute is a breaking change with little or maybe no value.
I don't see using the interface offers you any extra benefits over the other, except for allowing you to derive from another class at the same time, which is not needed anyway for code that is using DefaultCustomAstVisitor today.
You may argue that having 2 types doing similar things is not optimal. I agree it's not optimal, but they have been there for a long time and I don't think it really confuses anyone. For a user that uses DefaultCustomAstVisitor today, there is nothing bad for them to continue using it.

The only benefit of deprecating DefaultCustomVisitor/DefaultCustomVisitor2 that I can see is we don't need to update DefaultCustomVisitor2 when adding a new AST type. But it's trivial to keep that type update-to-date -- just adding a new template-like virtual method and that's all.

[rjmholt]

Ok, I'm happy with there being two types

[iSazonov]

In base library we could define only interfaces to avoid complication in support and confusing consumers. Helper classes like DefaultCustomAstVisitor developers can define in their projects if they need.

I think we will come to this in the future.

[IISResetMe]

I think I saw @IISResetMe hit this in the wild too maybe?

I realize I'm late to the party, but basically due to (not actually so non-)breaking modifications made to ICustomAstVisitor2 between 6 and 7, my ICustomAstVisitor2 (written for S.M.A 5.x/6.x) compiles against S.M.A 7.x without error - but then fails with contextually nonsensical exceptions at runtime whenever an AST contains any of the new conditional operators because I haven't provided an implementation (because that would make the entire class implementation invalid pre-7, since the injected AST types don't exist) 😒

In an ideal world we would have extended the interfaces and base classes with AstVisitor3 and ICustomAstVisitor3, so that I _could_ have simply done:

# Root module always imports this one 
class MyVisitor2 : ICustomAstVisitor2
{
  <# all 61 ICustomAstVisitor + ICustomAstVisitor2 declared Visit methods implemented here #>
}

# Root module only imports this one when major version -ge 7
class MyVisitor3 : MyVisitor2, ICustomAstVisitor3
{
  <# only need 2 additional method implementations here #>
}

... and then instantiate the version-appropriate visitor type in my commands at runtime - fairly easy to maintain.

But now that we've apparently decided to ignore the O in SOLID, I have to maintain:

# Root module only imports this one when major version -lt 7
class MyVisitor2 : ICustomAstVisitor2
{
  <# all 61 ICustomAstVisitor + ICustomAstVisitor2 declared Visit methods implemented here #>
}

# Root module only imports this one when major version -ge 7
class MyVisitor3 : MyVisitor2, ICustomAstVisitor3
{
  <# all 61 ICustomAstVisitor + ICustomAstVisitor2 declared Visit methods implemented here, again #>
  <# 2 additional ICustomVisitor2 method implementations here #>
}

... lest I want to turn the entire type definition into a web of code generation - in both cases not very maintainable anymore.

I appreciate that the visitor model as architected here isn't really super fit for _incrementally evolving the language_, and I don't expect this particular design decision to be reverted, but please keep the above in mind for the next time we need to expand the language grammar, because this sucks

[SeeminglyScience]

In an ideal world we would have extended the interfaces and base classes with AstVisitor3 and ICustomAstVisitor3, so that I _could_ have simply done:

Honestly I think the prospect of adding more of these versioned visitors is the reason we didn't get any new AST types for so long. DIM makes adding new ones much more palatable imo.

re: the issue with non-sensical exceptions, that's what DefaultVisit was added for (granted, not yet helpful). That will allow us to choose how we want to handle new AST types. In the future we can polyfill AST types to older versions, ignore, throw a custom exception, or use reflection to find child nodes.

... and then instantiate the version-appropriate visitor type in my commands at runtime - fairly easy to maintain.

If you swap ICustomAstVisitor2 for DefaultCustomAstVisitor2 you can still do that since the DIM's will be virtual, e.g.

class MyVisitor : DefaultCustomAstVisitor2 {
    # PSv5/6 impl
}

if ($PSVersionTable.PSVersion.Major -ge 7) {
    class MyVisitorFor7 : MyVisitor {
        [object] VisitTernaryExpression([TernaryExpressionAst] $ternaryExpressionAst) {
            throw [NotImplementedException]::new()
        }
    }
}

Ideally PowerShell classes would implement all DIM's as virtual methods that call their default impl, then you wouldn't need that.

Also in C# you can do the same or use compiler directives/partial classes.

[IISResetMe]

class MyVisitor : DefaultCustomAstVisitor2 {
    # PSv5/6 impl
}

if ($PSVersionTable.PSVersion.Major -ge 7) {
   class MyVisitorFor7 : MyVisitor {
       [object] VisitTernaryExpression([TernaryExpressionAst] $ternaryExpressionAst) {
           throw [NotImplementedException]::new()
       }
   }
}

@SeeminglyScience That would be great, but unfortunately not viable - we generate and emit all type definitions and using bindings as one of the first compilation step, so dot-sourcing a script simply _containing_ a class definition with a non-existing parameter type will fail long before we can evaluate if ($PSVersionTable.PSVersion.Major -ge 7)

[SeeminglyScience]

Sorry I did it that way for brevity, but I meant put it in a different file and conditionally dot source it. The same way you'd have to do it if it were ICustomAstVisitor3.

[daxian-dbw]

The polyfill idea is pretty neat 👍

[msftbot[bot]]

:tada:This issue was addressed in #13258, which has now been successfully released as v7.1.0-preview.6.:tada:

Handy links:

• Release Notes