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

Generic interceptors #68602

Merged
merged 8 commits into from
Jul 1, 2023
Merged

Generic interceptors #68602

merged 8 commits into from
Jul 1, 2023

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Jun 13, 2023

Test plan: #67421
Continuation of #68218

Addresses point 1 in #68218 (comment)

We want to deliver a critical level of generics support in .NET 8. Namely, in addition to permitting interceptors with arity 0, we will also permit interceptors whose arity equals the net arity of the original method (sum of arity of containing types plus arity of original method). The type arguments from the original method and containing types are passed along to the interceptor in source order, i.e. starting from the outermost generic type, and inwards to the original method.

It might also be worthwhile to spend a little more time investigating the "unification" approach. But it's something that could be added later without breaking things.

@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 13, 2023
@RikkiGibson RikkiGibson marked this pull request as ready for review June 14, 2023 17:11
@RikkiGibson RikkiGibson requested review from a team as code owners June 14, 2023 17:11
@RikkiGibson RikkiGibson requested review from cston and jcouv June 14, 2023 17:12
@jcouv jcouv self-assigned this Jun 14, 2023
// gather the type arguments starting from outermost containing type in to the method.
typeArgumentsBuilder.AddRange(stack.Pop().GetMemberTypeArgumentsNoUseSiteDiagnostics());
}
while (stack.Count != 0);
Copy link
Member

@cston cston Jun 16, 2023

Choose a reason for hiding this comment

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

Consider using the existing extension method, and also use TypeWithAnnotations since that type is used for Construct().

var builder = ArrayBuilder<TypeWithAnnotations>.GetInstance();
method.ContainingType.GetAllTypeArgumentsNoUseSiteDiagnostics(builder);
builder.AddRange(method.TypeArgumentsWithAnnotations);
interceptor = interceptor.Construct(builder.ToImmutableAndFree());
``` #Resolved

@RikkiGibson
Copy link
Contributor Author

@jcouv Please take a look when you get the chance.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 2)

@RikkiGibson RikkiGibson requested a review from jcouv June 30, 2023 23:14
@@ -55,6 +55,8 @@ namespace Microsoft.CodeAnalysis.CSharp.LanguageServer
"CS9159", // ErrorCode.WRN_NullabilityMismatchInParameterTypeOnInterceptor
"CS9160", // ErrorCode.ERR_InterceptorCannotInterceptNameof
"CS9163" // ErrorCode.ERR_SymbolDefinedInAssembly
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a comma is missing here

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 7) modulo missing comma

@jcouv jcouv added this to the 17.8 milestone Jun 30, 2023
@RikkiGibson RikkiGibson merged commit 10fbb5f into dotnet:main Jul 1, 2023
@ghost ghost modified the milestones: 17.8, Next Jul 1, 2023
@allisonchou allisonchou modified the milestones: Next, 17.8 P1 Jul 24, 2023
@RikkiGibson RikkiGibson deleted the ic-generics branch April 15, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers 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.

4 participants