Since you refactor the code you could move the Equals in separate if block and convert the switch to a switch expression.

Please use s_ prefix for statics. Below too.

We think we should explicitly use InvariantCulture for formatting in the two places. It is our common pattern in Engine.

AppendJoin() doesn't appear to have an overload that takes a format provider, but will do for AppendFormat().

I think @iSazonov is referring to the string interpolation used in the Select delegate.

I meant both - AppendFormat and the string interpolation.

The same about InvariantCulture.

This probably isn't needed -- it's probably simpler to let the GC just collect the whole string builder in one go when the method returns

There's some interspersal of string building and formatting in here, along with some LINQ, all inside a non-threadsafe lazy logic block embedded in a property.

I think some possibilities to consider are:

• Move the string generation into its own method that just returns a value
• Use a Lazy<string> like EventDefinition => _eventDefinitionLazy.Value
• Fully embrace either the string builder or the formatter, depending on whether we're trying to optimise for performance or readability

So for example:

private Lazy<string> _eventDefinitionLazy = new Lazy<string>(CreateEventDefinition);
internal string EventDefinition => _eventDefinitionLazy.Value;

private string CreateEventDefinition()
{
    var invokeMethod = new MethodInformation(
        this.events[0].EventHandlerType.GetMethod(nameof(Action.Invoke)),
        parametersToIgnore: 0);

    var sb = new StringBuilder()
        .Append("event ")
        .Append(ToStringCodeMethods.Type(events[0].EventHandlerType))
        .Append(' ')
        .Append(events[0].Name)
        .Append('(');

    ParameterInfo[] invocationParameters = invokeMethod.GetParameters();
  
    if (invocationParameters.Length > 0)
    {
        ParameterInfo currParam = invocationParameters[0];
        sb.Append(ToStringCodeMethods.Type(currParam.ParameterType))
            .Append(' ')
            .Append(currParam.Name);

        for (int i = 1; i < invocationParameters.Length; i++)
        {
            currParam = invocationParameters[i];

            sb.Append(", ")
                .Append(ToStringCodeMethods.Type(currParam.ParameterType))
                .Append(' ')
                .Append(currParam.Name);
        }
    }

    return sb.Append(')').ToString();
}

Any reason to make this static, if we just have to pass this back in?

Not anymore! I wrote it this way initially as I wasn't sure if I'd need to callback to this from the binder or some such witchcraft, but that's definitely not required here.

This suggests that this will only work when events are given in the form:

$x.Event += $handler

But in the comment below you give an alternate form:

$e = $x.Event
$e += $handler

Should we expect that to behave differently? If so, why? Is that consistent with other similar behaviours in PowerShell today?

Hmm, that's a good point! I'm not sure, I guess I'd expect that that behaves the same. 🤔

I was mostly mimicking C#'s syntax, not sure saving the event to a var works there, but I guess I'd expect that works also.

Looking back at this, the example you're referring to is more illustrating how the $object.EventName += $sb expression is compiled (it's saved to a temporary variable first) rather than what would literally work.

Do we want to allow this to work for PSEvent instances saved in a variable as well? My concern here was mainly that the $object.EventName += $b is likely to be the most common usage, and I didn't want to bog down every $var += $sb expression with a runtime check for $var -is [PSEvent] if I could avoid it.

Looking at Sharplab, I can see the C# compiler actually completely prevents you assigning an event handler reference to a variable in the first place:

I don't think we need to be quite so strict in PS, but I think folks trying to use += / -= for event handling will expect similar behaviour.

Right, absolutely. I actually think you're right, but wanted to ask the question. (I referred to the compilation example because I knew it was a compilation example, but it was why I thought of it).

Events are strange second-class citizens in C# (I don't believe "event handler reference" is a meaningful concept for C#). But with that said, I'm not sure how many people we help by making them first-class in PowerShell (at the cost of things like runtime performance).

👍 for how it's currently implemented. It's consistent with C# and also with how PSMethod's are handled. Like you can $method = $obj.ToString but you can't then do $method()

What is the cast needed for here?

I don't recall... I'll try it without, but I probably copied this pattern from elsewhere in the compiler.

@daxian-dbw may know why it's needed

What is the cast needed for here?

More than likely it's because expressions don't implicitly box, the cast here will do that. Also matching expected types is super finicky (like a Expression.Block's return type, or a variable) which casting everything to object greatly simplifies.

That's why there's a ton of casting, but I don't know off the top of my head of getExpr would already be cast. (Side note, it's also why byref/likes will probably never be supported 🙁)

Looking at how this is called, I think it does make sense the way it's currently written, but I just want to raise it for discussion.

A new possibility in one of the callers in this method now forces itself on all the other callers, leading to a number of calls where the caller must pass null in here. This seems to be common in the PowerShell codebase, but in my opinion leads to decohesion of methods, both from the perspective of the caller (which must now carefully select the appropriate subset of parameters) and from the perspective of the method itself (which must now carefully step between shared and scenario-specific logic).

In some respects this new parameter is a pseudo-boolean; it's either null or has a particular value that causes us to follow another path within the method.

That suggests to me that really we have two methods on our hands, with some shared logic best represented by breaking things up into smaller, composable methods.

Definitely a good point. I can have a look at this and try to refactor things a bit.

I think I have a pretty good way to do this. There's some duplication of code, but it separates the code paths much more neatly. I'll commit this change shortly; any additional suggestions you have to improve what I managed to get to are more than welcome! 😊