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

Add interceptors open design questions #67970

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions docs/features/interceptors.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,35 @@ namespace System.Runtime.CompilerServices
}
```

#### PROTOTYPE(ic): Use an assembly attribute instead?
The main reason we want something like `[Interceptable]` is so that users can better reason about *which calls might be getting intercepted or not*. We don't think there's a meaningful *security* concern which is addressed by `[Interceptable]`--if something untrusted has the ability to add source files to your build, taking away the ability for them to intercept certain calls doesn't really make a difference.

It's been suggested that `[Interceptable]` on method declarations is the wrong point of control. Some SG authors will have good reason to want to intercept a call to a method from a library they don't own--for example, EF wanting to intercept calls to IQueryable extensions. It shouldn't be necessary for those authors to have to request the library authors to add an attribute before they can start doing it.

One option to address this is to remove `[Interceptable]` from the design and not provide any mechanism for limiting which calls can be intercepted or not.

Another option would be to expose an assembly attribute instead.

```cs
namespace System.Runtime.CompilerServices
{
/// <summary>If present, indicates the set of types whose methods can have their calls intercepted in the current compilation.</summary>
/// <remarks>When this attribute is not present, methods on any type can have their calls intercepted.</remarks>
[AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false)]
public sealed class AllowInterception(params Type[] types) : Attribute { }
}

[assembly: AllowInterception(typeof(EndpointRouteBuilderExtensions))]
```

In order to intercept a classic extension method call, the extension type must be provided to this attribute, not the `this` parameter type.

We specify `AllowMultiple = false` here to imply that the attribute is supposed to be hand-authored by the user. It's not meant for generators to use this to *opt themselves in* to being able to intercept things--it's for the user to state once and comprehensively which things can be intercepted.

In such a scheme, it's not clear how often users would actually want to go to the trouble of writing the attribute--particularly since the process of writing and maintaining it would simply be to run source generators, review the "not interceptable" errors, and add the related types to this attribute. Is that useful to users in practice?
Copy link
Member

@jkotas jkotas Apr 26, 2023

Choose a reason for hiding this comment

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

I do not think it is useful.

We can add this attribute in future if we find actual need for it. It would not be breaking change to add it in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, are you in favor of the below option?

One option to address this is to remove [Interceptable] from the design and not provide any mechanism for limiting which calls can be intercepted or not.

Copy link
Member

Choose a reason for hiding this comment

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

Yep

Copy link
Member

Choose a reason for hiding this comment

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

I agree it is an item we could add later if there was need.

Copy link
Contributor Author

@RikkiGibson RikkiGibson Apr 27, 2023

Choose a reason for hiding this comment

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

I haven't heard any objection to eliminating InterceptableAttribute for the .NET 8 version of the feature, and possibly introducing some limiting mechanism in the future.

I'll merge this PR as-is, but have added a note to the test plan to go ahead and eliminate InterceptableAttribute in this design doc and from the implementation in a subsequent PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, let's get rid of it. Thanks.


Another alternative here might be to use `AllowMultiple = true`, require in practice that generators produce these attributes (i.e. disallow intercepting in the current compilation if this attribute isn't used in it), and let users search for usages of the attribute to get a sense of which calls may be getting intercepted in their project.

### InterceptsLocationAttribute

A method indicates that it is an *interceptor* by adding one or more `[InterceptsLocation]` attributes. These attributes refer to the source locations of the calls it intercepts.
Expand Down Expand Up @@ -178,6 +207,8 @@ Although interceptors are an experimental feature, there will be no explicit opt

PROTOTYPE(ic): The BCL might not ship the attributes required by this feature, instead requiring them to be declared in some library brought in by a package reference, or in the user's project. But we haven't confirmed this.

PROTOTYPE(ic): Feedback from Jared is that we need to have an opt-in step, even if it is only performed by generators, so that any teams adopting this understand the level of support they can expect for this. Most likely, we would do this by checking for `/features=interceptors` on the command line (`<Features>interceptors</Features>` in msbuild). Then generators that want to use interceptors would just ship a bit of msbuild logic to ensure the appropriate feature flag is being passed to the compiler.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is important if we end up labeling this as experimental for .NET 8. There will absolutely be generators that take advantage of this setting. I don't want to fall into a trap where we label it experimental, have no opt-in mechanism and generators using interceptors reach a critical mass such that we can't change it. Just as customers had to opt into experimental features like DIM, they need to opt into this.

At the same time, I feel like for scenarios like ASP.NET AOT this opt in is implicit. The props / targets in SDK are free to implicitly set this flag such that we have a friction free experience there. Part of deciding this is experimental would be ASP.NET agreeing to adjust the generator to new compiler behavior should we change things. Essentially we're agreeing to own keeping this overall scenario working as a team even if the implementation details change a bit.

Note: this is all still predicated on whether this is labeled experimental or not in .NET 8.


### Implementation strategy

During the binding phase, `InterceptsLocationAttribute` usages are decoded and the related data for each usage are collected in a `ConcurrentSet` on the compilation:
Expand Down