Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid span conversion in delegate extension receiver #74044

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Jun 18, 2024

An extension method Cᵢ.Mₑ is eligible if:

  • Cᵢ is a non-generic, non-nested class
  • The name of Mₑ is identifier
  • Mₑ is accessible and applicable when applied to the arguments as a static method as shown above
  • An implicit identity, reference or boxing , boxing, or span conversion exists from expr to the type of the first parameter of Mₑ.
    👉 Span conversion is not considered when overload resolution is performed for a method group conversion.

Speclet updates: dotnet/csharplang#8221, dotnet/csharplang#8287
Test plan: #73445

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 18, 2024
@jjonescz jjonescz marked this pull request as ready for review June 18, 2024 13:07
@jjonescz jjonescz requested a review from a team as a code owner June 18, 2024 13:07
@jjonescz jjonescz requested review from 333fred and cston June 18, 2024 13:07
@jjonescz
Copy link
Member Author

@cston @333fred for reviews, thanks.

@cston
Copy link
Member

cston commented Jun 25, 2024

It looks like there is still an open question in the speclet about whether to take the breaking change or not. Was that discussed in LDM?

@jjonescz
Copy link
Member Author

It looks like there is still an open question in the speclet about whether to take the breaking change or not. Was that discussed in LDM?

Not yet, I'm optimistically assuming we won't take a breaking change here. But if we will, we can always revert this. Or we can postpone this PR until after LDM if you prefer.

@cston
Copy link
Member

cston commented Jun 26, 2024

Or we can postpone this PR until after LDM if you prefer.

Is this change blocking other work? If not, I think my preference is to postpone this PR until after an LDM decision.

@jjonescz jjonescz marked this pull request as draft June 26, 2024 15:41
@jjonescz jjonescz marked this pull request as ready for review July 15, 2024 18:52
@jjonescz
Copy link
Member Author

@cston @333fred this is ready for reviews again, thanks

Copy link
Member

@333fred 333fred 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 2)

return IsValidExtensionMethodThisArgConversion(conversion) ? conversion : Conversion.NoConversion;
}

// Spec 7.6.5.2: "An extension method ... is eligible if ... [an] implicit identity, reference,
// or boxing conversion exists from expr to the type of the first parameter"
public Conversion ClassifyImplicitExtensionMethodThisArgConversion(BoundExpression sourceExpressionOpt, TypeSymbol sourceType, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
public Conversion ClassifyImplicitExtensionMethodThisArgConversion(BoundExpression sourceExpressionOpt, TypeSymbol sourceType, TypeSymbol destination, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo, bool isMethodGroupConversion)
Copy link
Member

Choose a reason for hiding this comment

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

Should we update the comment above this method?

@jjonescz
Copy link
Member Author

@cston for the second review, thanks

@jjonescz jjonescz merged commit 4ba990b into dotnet:features/FirstClassSpan Aug 1, 2024
24 checks passed
@jjonescz jjonescz deleted the FirstClassSpan-06-ExtReceiverDelegate branch August 1, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - First-class Span Types untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants