VerifyFlowGraph [](start = 12, length = 15)

It looks like all tests adjusted in the second commit are using target-typed new. I am assuming regular new had the same issue. Please confirm. #Closed

It would be good to add verification for a couple of interesting scenarios with a regular new.

In reply to: 528940856 [](ancestors = 528940856)

It looks like it doesn't, because the existing code uses a syntax check rather than IOperation check, which I missed on my first pass. I'll update the syntax check instead, we already have coverage of these scenarios for regular object creation.

In reply to: 528943004 [](ancestors = 528943004,528940856)

|| operation.Instance.HasErrors(operation.SemanticModel.Compilation) [](start = 62, length = 69)

It feels like this breaks one of the invariants that we have. Why the original condition was fine for a regular member access and a problem for patterns? #Closed

It feels to me that we should fix the product instead and maintain the invariant.

In reply to: 528948352 [](ancestors = 528948352)

Why the original condition was fine for a regular member access and a problem for patterns?

Patterns include the implicit instance reference on the object being pattern matched against, even though it's a static member. I can adjust the implementation to instead not include that reference, but it may be useful for things introspecting the operation tree?

In reply to: 528952013 [](ancestors = 528952013,528948352)

TestOperationVisitor.Singleton.Visit(instanceReceiver); [](start = 12, length = 55)

It is not clear to me what is this doing which VerifyOperationTreeForTest wouldn't already do. Could you please elaborate? #Closed

VerifyOperationTreeForTest does not call the TestOperationVisitor on operation nodes. Perhaps it should, but that is what this is accomplishing.

In reply to: 528951526 [](ancestors = 528951526)

#49267 [](start = 67, length = 45)

It looks like this issue is been actively worked on. Please coordinate @chsienki to make sure the test are in enabled state after both PRs are merged. #Closed

Foreach_ExtensionGetEnumeratorWithParams [](start = 20, length = 40)

Do we have a similar test for VB? #Closed

We didn't add extension foreach support to VB, did it already have support for the feature?

In reply to: 529091269 [](ancestors = 529091269)

We didn't add extension foreach support to VB, did it already have support for the feature?

Extension methods were always considered for patterns like that in VB:

Module Ext
    <System.Runtime.CompilerServices.Extension>
    Function GetEnumerator(this As MyEnumerable, ParamArray array As Integer()) As System.Collections.Generic.IEnumerator(Of Integer)
    End Function
End Module

Class MyEnumerable
    Sub Test()
        For Each x In New MyEnumerable
        Next
    End Sub
End Class

In reply to: 533002410 [](ancestors = 533002410,529091269)

enumeratorMethod.Parameters.Any(p => p.IsParams) [](start = 74, length = 48)

This calculation feels fragile and possibly incorrect in a general case. Presence of params modifier doesn't actually imply that the argument always has an expanded form. For example, the parameter might not have the right type. Or params, which is just an attribute in metadata, can be on a "wrong" parameter. Etc. See, OverloadResolution.IsValidParams for example. Rather than trying to re-infer this information, I think we should find a way to thread it through from the binding. Also, with recent work to move creation of default values to binding, I would expect that would also take care of param arrays, when necessary.

#Closed

params int[] x [](start = 58, length = 14)

Consider testing scenario when this parameter is optional in metadata and doesn't have an array type. Or, for example, has the right type, but it is not the last parameter, there is also another optional parameter on the method. #Closed

Some error scenarios from source could be interesting too.

In reply to: 529094019 [](ancestors = 529094019)

indexes [](start = 45, length = 7)

I am not sure what "indexes" is referring to. Could you please elaborate? #Closed

associatedSymbol [](start = 97, length = 16)

Perhaps we should assert somewhere that associatedSymbol is not a local function or a lambda, otherwise the condition might not be accurate. #Closed

It's fine if it's a local function or lambda, but we likely want to skip only if that local function or lambda isn't itself part of the top-level main.

In reply to: 529792485 [](ancestors = 529792485)

while (parent is PostfixUnaryExpressionSyntax { OperatorToken: { RawKind: (int)CSharp.SyntaxKind.ExclamationToken } }) [](start = 28, length = 118)

I think this logic should be incorporated into applyParenthesizedIfAnyCS and the helper renamed accordingly #Closed

if (((CSharp.Syntax.ConditionalAccessExpressionSyntax)syntax.Parent).Expression == syntax) [](start = 36, length = 90)

I think we should see if we want to fix the builder instead. Looking at the added NestedConditionalAccess unit-test, the life-time of capture 0 is extended unnecessarily. #Closed

In any case I do not think we should be treating it as long lived (i.e. something that is known to span multiple statements). So the relaxation should probably be applied elsewhere

In reply to: 529818891 [](ancestors = 529818891)

After much offline investigation and discussion, I will be tightening the assertion, but we will be keeping the builder as is. It's not ideal behavior, but would require significant effort to make ideal with significant maintenance burden, and is not worth it for this case.

In reply to: 530039773 [](ancestors = 530039773,529818891)

Block[B1] - Block [](start = 8, length = 17)

I think we actually should have a nested region here. it should enclose B1, B2 and own capture [0]. #Closed

Timeouts [](start = 67, length = 8)

Should we have an issue to track disable tests like that? #Closed

I don't think that's particularly useful, as I'm not currently thinking that we'll be attempting to renable them in the future.

In reply to: 529820501 [](ancestors = 529820501)

BindToNaturalType [](start = 70, length = 17)

BindToTypeForErrorRecovery?
#Closed

We also have a BuildArgumentsForErrorRecovery, see if we can use that helper.

In reply to: 529823025 [](ancestors = 529823025)

BindToNaturalType [](start = 75, length = 17)

BindToTypeForErrorRecovery? #Closed

constructorArguments.Arguments [](start = 72, length = 30)

Are we leaking this builder? #Closed

analyzedArguments.NamedArguments [](start = 66, length = 32)

See if we want to change BindAttributeArguments to keep it as a builder since we are going to allocate a new array here. #Closed

true [](start = 239, length = 4)

Why is this true? #Closed

true [](start = 82, length = 4)

Can we run into an instance method with default values and/or param array? #Closed

This commit is no longer necessary, dropping from the updated PR.

In reply to: 529836543 [](ancestors = 529836543)

defaultArguments [](start = 28, length = 16)

Is there a test where we can observe the difference? #Closed

&& syntax.Parent.Parent is InitializerExpressionSyntax { Parent: CSharp.Syntax.ObjectCreationExpressionSyntax }) [](start = 40, length = 112)

Would it be possible to tighten the condition by also checking something other than syntax? Perhaps check symbols involved? #Closed

I do not think we should recognize this as long lived, it doesn't escape the statement.

In reply to: 530036537 [](ancestors = 530036537)

Could isObjectOrArrayInitializerInitializedObjectTarget help for this scenario?

In reply to: 530040111 [](ancestors = 530040111,530036537)

binder: this [](start = 24, length = 12)

Is the problem that we didn't pass a binder when it was needed? Can we add an assert somewhere that we got null binder when non-null is expected to be passed? #Closed

Is the problem that we didn't pass a binder when it was needed?

Yes, it's needed for DeriveArguments.

Can we add an assert somewhere that we got null binder when non-null is expected to be passed?

It's actually an assertion in DeriveArguments that caught this, we just didn't have a test case that verified the IOperation tree for this code path. I'm going to need to incorporate this change into the branch changes. We still have the bug in master, but the assertions I added when nullable enabling are blocking my ability to run benchmarks.

In reply to: 530043147 [](ancestors = 530043147)

This iteration is no longer needed.

In reply to: 533822002 [](ancestors = 533822002,530043147)