Review all remaining callers with regards to new extensions

Is the goal to remove the enclosing method or CheckConstraintsIncludingExtension at the end? If so, consider saying this explicitly. If not, could you please elaborate what is the goal? #Closed

The question was whether we want any of the callers be affected by extensions. If not, they'll continue to use this method. Otherwise, their logic needs to be updated (lookup logic) and they should call CheckConstraintsIncludingExtension.
After reviewing the callers, I see that none of them accounts for classic extension methods, so I don't think they should be affected by new extension methods either.
Will remove this follow-up comment

I am sorry, but the response doesn't make sense to me. We don't want the callers to be affected , but we call CheckConstraintsIncludingExtension here. So, why is it a good thing to have two methods after you reviewed all callers?

Resolved offline. Brought back original name

CheckMethodConstraintsIncludingExtension

Why do we want to add a different method instead of simply adjusting behavior of the original method? #Closed

We discussed offline that we probably want to keep the original name of the method and push the logic that checks constraints on the extension down to the "root" CheckConstraints helper (so that no one could miss that logic no matter what overload is used).

ConstructedFrom

Why is it necessary to go through ConstructedFrom wouldn't method.TypeParameters return the right thing? #Closed

Debug.Assert(property.GetIsNewExtensionMember());

It feels inconsistent that properties and methods get different treatment. For methods we check this condition, but for properties we assume it is true. #Closed

This thread was marked as resolved, but I do not see any related code change and there was no other response.

The resolution is the added assertion at the top of the method: Debug.Assert(member.GetMemberArityIncludingExtension() != 0);

MakeOrdinalsIfNeeded

"MakeAdjustedTypeParameterOrdinalsIfNeeded"? #Closed

ordinals = PooledDictionary<TypeParameterSymbol, int>.GetInstance();

I think the map that we create here is supposed to use identity comparison. Is this what we get? #Closed

This thread was resolved, but I do not think changing the key to object changes anything about how keys are compared

CheckConstraintsIncludingExtension

Is this change covered by a test? #Closed

Will undo this change and the one in GetCandidatesPassingFinalValidation. In the language versions where we include new extension members in overload resolution, we filter bad candidates (which fail constraint checks) early, so the final validation check doesn't make a difference.
But new extensions can make it here, so the helper with assertion cannot be used.

CheckConstraintsIncludingExtension

Is this change covered by a test? #Closed

GetParametersIncludingExtensionParameter

It might be better to have an API that returns combined count and avoids unnecessary allocations. #Closed

parameterListList.Add(m.Parameters);

Shouldn't we also include extension parameter? #Closed

Added and tried to observe the change, but I wasn't able to

CheckConstraintsIncludingExtension

Is this change covered by a test? #Closed

We don't report detailed overload resolution diagnostics for extensions, so I'll undo this.

return false;

This defeats the purpose of the method to enable handling of long chains of fluent calls. Note that for classic extension methods we return true (line 6388). #Closed

bool reportedDiagnostic = enumeratorInfoOpt?.GetEnumeratorInfo.Method is { IsExtensionMethod: true } // Tracked by #76130: Test this code path with new extensions // Tracked by #76130: Test this code path with new extensions

Is this change really necessary? #Closed

comp.VerifyEmitDiagnostics();

Consider calling CompileAndVerify(...).VerifyDiagnostics() instead #Closed

oNull

Do we have tests covering nullability of arguments (both for static and instance extensions)? #Closed

TypeParameterSymbol

It is not clear why we are relaxing the type #Closed

In order to get reference equality. We could allocate, we could introduce a new pool, or we could use object as the key type.

parameterListList.Add(m.GetParametersIncludingExtensionParameter());

Are we testing this code path with static extensions? #Closed

RequiresCheckingIncludingExtension

I don't think we need to change the name of this method. #Closed

parametersOpt = method.GetParametersIncludingExtensionParameter();

Are we testing this code path for static extensions? #Closed

Yes, Nullability_Invocation_09, the object.M2() invocation

CheckMethodConstraintsIncludingExtension

It doesn't look like there is a need to rename this method #Closed

if (method.GetIsNewExtensionMember() && method.ContainingType is { Arity: > 0 } extension

As we discussed offline, I think this logic should be moved into the method called above, and quite possibly into much lower level helper, the "root". Therefore, I do not expect implementation of the enclosing method to change. #Closed

method.IsStatic ? method.Parameters : method.GetParametersIncludingExtensionParameter()

It feels like there could be a helper for this common pattern. Even better, the existing helper could have a non-optional parameter specifying what should happen in the static case so callers don't forget to handle that properly. #Resolved

Why don't we need to do anything special for new extensions here? Consider adding a comment. #Resolved

Good catch. I forgot to revisit this when enabling the stack-based optimization.
We need to set extensionReceiverResult properly as it is used as firstArgumentResult: argument in the next iteration of the loop. That way, when we get to visit arguments in reinferMethodAndVisitArguments (and ultimately in VisitArgument) we skip visiting that argument, so that we don't end up visiting it twice.
The added tests verify this.

On the flip side, I couldn't come up with any test case where extensionReceiverResult = _visitResult; wouldn't be good enough. The call to VisitArgumentEvaluateEpilogue is right as far as I can tell, but I was not able to observe it (and neither do existing nullable tests).

I pushed one more commit for a test that covers this properly. It took me some time to come up with a test but it's possible :-)

Shouldn't this check receiverRefKind? Otherwise if extensionParameter.RefKind is In for example, we construct the array unnecessarily. #Resolved

I think this came up previously, can you remind me why we check only Ref and not other ref kinds? #Resolved

This follows what we do for classic extension methods. See OverloadReolution.IsApplicable:

                    if (forExtensionMethodThisArg)
                    {
                        Debug.Assert(argumentRefKind == RefKind.None);
                        if (parameterRefKind == RefKind.Ref)
                        {
                            // For ref extension methods, we omit the "ref" modifier on the receiver arguments
                            // Passing the parameter RefKind for finding the correct conversion.
                            // For ref-readonly extension methods, argumentRefKind is always None.
                            argumentRefKind = parameterRefKind;
                        }
                    }

So when we need to do inference or argument conversions, we add a ref if the parameter is ref, and we leave none for ref readonly/in. out is a declaration error.

Why are post conditions skipped for new extension members? #Resolved

The are not just "post-conditions", they are "member post-conditions". The attributes that are processed below are MemberNotNull and MemberNotNullWhen.
Normally those apply to members of the containing type, which holds states. My thinking was that extensions don't hold state, so those attributes don't apply.
But extension members could be stateful, so it may make sense after all.
I'll add a follow-up comment to confirm whether we want this to work. If so, we'll need to spec it first. Thanks!

Consider testing the suppression operator (!) as well for at least a few cases. #Resolved