Interceptors open issues for C# 13 #7835
Conversation
|
|
||
| Having looked at several strategies to address here, including adjusting when the build system passes `/pathmap` to the compiler, it seems the most straightforward way is to permit relative paths. This is a strategy which is already proven out by `#line` directives. | ||
|
|
||
| When a relative path appears in an `[InterceptsLocation]`, we will resolve it relative to the path of the containing file. |
There was a problem hiding this comment.
Need to add a note that we will normalize / and / during this process that way paths are portable across operating system builds. Effectively the resolution for relative paths here will match what happens in #line today. What works there will work in interceptors
There was a problem hiding this comment.
Since source generators are already creating the #line directives with relative paths, even before they were run as Roslyn extensions, I'm wondering how they are doing that. Perhaps this is something generators already get wrong regularly, but isn't noticed as much, as the only impact is on debugging in a different environment than we generated the code in, rather than completely failing to build.
| - it's in the expression of expression statement `Call();` | ||
| - it's in the receiver of invocation expression `Call()`. | ||
|
|
||
| This sequence could be encoded somehow and stuffed into the attribute. This would make it so that edits in other method bodies etc. will not necessarily require regenerating the interceptor. However, it's not clear how source generators, or the generator driver, could make use of this information in order to reduce the frequency of rerunning generators. Also, public API would likely be needed for this in order for generators to create the expected InterceptsLocationAttribute argument. |
There was a problem hiding this comment.
Another potential is we could both
- Largely take the recommendations here about using relative file paths
- Declare that the identifier passed to
InterceptsLocationis opaque
Essentially tell customers
The value passed into
InterceptsLocationis opaque. The current implementation is an encoding of relative paths + line column but that could change at any time. Conforming generators should not depend on this encoding. Instead all arguments toInterceptsLocationshould be generated by callingGetInterceptsLocation(syntaxNode, hintName).
That would require using the API I suggested above but taking a SyntaxNode (thus has line / column + path) instead of SyntaxTree.
I'm not saying we should do this but it is an item to consider.
|
|
||
| public static class CSharpExtensions | ||
| { | ||
| + public static IMethodSymbol? GetInterceptorMethod(this SemanticModel model, InvocationExpressionSyntax invocation, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Some feedback I got includes:
- What about IOperation? Do we have to get the .Syntax out of the operation node and cast it? Or can we just add a property to IInvocationOperation?
- What about future additional member kinds? Will we add additional public API for additional kinds?
| } | ||
| ``` | ||
|
|
||
| `GetFilePath()` will return the absolute file path which the generated file *would be written to* if `$(EmitCompilerGeneratedFiles)` is true in the project. Additionally, the syntax tree for the file added by a subsequent call to `AddSource` using the same `hintName` will have a `FilePath` which exactly matches the result of `GetFilePath()`. |
There was a problem hiding this comment.
This is assuming that as a generator author my AddSource calls look like this:
context.AddSource(context.GetFilePath("GeneratedRouteHandlers.g.cs"), mySourceText)If so, can we make it implicit so that I can just write:
context.AddSource("GeneratedRouteHandlers.g.cs", mySourceText)There was a problem hiding this comment.
Yes my intent here matches what you're saying. AddSource will always take a hintName, not an absolute path. Also, it's feeling unclear to me now if GetFilePath() API is worth it, if we also have to add GetInterceptsFilePath() or similar API anyway.
|
|
||
| For method invocations like `receiver.M(args)`, we use the location of the `M` token to intercept the call. Perhaps for property accesses like `receiver.P` we can use location of the `P` token. For constructors should use the location of the `new` token. | ||
|
|
||
| For member usages such as user-defined implicit conversions which do not have any specific corresponding syntax, we expect that interception will never be possible. |
There was a problem hiding this comment.
Should a section on dotnet/roslyn#68218 be included somewhere in the doc?
There was a problem hiding this comment.
Yes, and it will also be useful to get a sense of how urgent it is to get it done in C# 13.
There was a problem hiding this comment.
BTW, it feels like the real kicker here is not just that we want generics support (through something more sophisticated like a unification approach), but that we actually want the interceptor to be able to have a more specific signature than the original method. e.g. in this case where in order for type parameters to propagate through the system, the interceptor parameter needs to be Func<TypeParam> even though the original parameter type is Delegate.
|
@jaredpar I consider this ready to merge, please give a review and/or assign reviewers. I'd like to send a follow up PR which essentially migrates the speclet from the Roslyn repo (linked in this document), into this same document, and renames it to match the other proposal docs (e.g. renaming it to interceptors.md, including the speclet content first, then as a subsequent section include these open questions.) |
Related to #7009