Skip to content

Conversation

@jcouv
Copy link
Member

@jcouv jcouv commented Aug 13, 2025

Addresses parts of #78827

Note: an implementation method for a non-static new extension method is an extension method, and therefore not imported as a static method (existing behavior, see IsValidLookupCandidateInUsings) and cannot be invoked directly. It can only be called by extension invocation.

Relates to test plan #76130

@jcouv jcouv self-assigned this Aug 13, 2025
@jcouv jcouv added Area-Compilers Feature - Extension Everything The extension everything feature labels Aug 13, 2025
@jcouv jcouv marked this pull request as ready for review August 13, 2025 08:50
@jcouv jcouv requested a review from a team as a code owner August 13, 2025 08:50
internal void GetExtensionContainers(ArrayBuilder<NamedTypeSymbol> extensions)
{
// Consider whether IsClassType could be used instead. Tracked by https://github.com/dotnet/roslyn/issues/78275
if (!IsReferenceType || !IsStatic || IsGenericType || !MightContainExtensionMethods) return;
Copy link
Member

@jjonescz jjonescz Aug 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check !IsReferenceType || !IsStatic || IsGenericType inside MightContainExtensionMethods instead? #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would make sense.
As part of this PR, I'd identified a couple other questions/follow-ups regarding MightContainExtensionMethods. I captured them to this follow-up issue and captured your suggestion there as well.
I prefer to keep this PR very focused and keep this moved code as-is for now. I'll take a stab at this in next PR, as it's bothering me too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked around a bit and it's already clear the answer is not obvious, so would rather not open a possible can of worms. See PENamedTypeSymbol.MightContainExtensionMethods:

        public override bool MightContainExtensionMethods
        {
            get
            {
                var uncommon = GetUncommonProperties();
                if (uncommon == s_noUncommonProperties)
                {
                    return false;
                }

                if (!uncommon.lazyContainsExtensionMethods.HasValue())
                {
                    var contains = ThreeState.False;
                    // Dev11 supports extension methods defined on non-static
                    // classes, structs, delegates, and generic types.
                    switch (this.TypeKind)
                    {
                        case TypeKind.Class:
                        case TypeKind.Struct:
                        case TypeKind.Delegate:
                        ...

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (commit 1)

@jcouv jcouv enabled auto-merge (squash) August 22, 2025 17:37
@jcouv jcouv merged commit 49cf64c into dotnet:main Aug 22, 2025
23 of 24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Aug 22, 2025
@akhera99 akhera99 modified the milestones: Next, 18.0 P1, 18.0 P2 Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Feature - Extension Everything The extension everything feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants