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

[API Proposal]: Custom attribute for Pre-compile AOP #59454

Open
wsq003 opened this issue Sep 22, 2021 · 25 comments
Open

[API Proposal]: Custom attribute for Pre-compile AOP #59454

wsq003 opened this issue Sep 22, 2021 · 25 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices
Milestone

Comments

@wsq003
Copy link

wsq003 commented Sep 22, 2021

Background and motivation

[MyCache(DurationSeconds = 5)]
public string GetUserName(int userID)
{
    //load from db
    return "Mike";
}
[MyPerfLog()]
public bool Login(string userName, string password)
{
    //check with db
    return true;
}

It is very annoying to add AOP to functions.
For now, you need to:
1, Create a custom Attribute
2, Create a custom Interceptor to handle your custom Attribute
3, Register your Interceptor at application startup

I think there would be a syntactic sugar that handling specific attribute.

API Proposal

[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class)]
public abstract class AopAttribute : Attribute
{
	/// <summary>
	/// </summary>
	/// <returns>if execute the real Method or not</returns>
	public abstract bool BeforeMethod(MethodInfo info);
	public abstract void AfterMethod(ResultInfo result);

	public abstract Task<bool> BeforeMethodAsync(MethodInfo info);
	public abstract Task AfterMethodAsync(ResultInfo result);
}

This is a compile-time attribute, more or less like System.Diagnostics.ConditionalAttribute.
Compiler will generate proxy class to wrap the target functions.

API Usage

public class MyPerfLogAttribute : AopAttribute
{
	ILogger _logger;
	DateTime _begin;

	public MyPerfLogAttribute(ILogger logger)
	{
		_logger = logger;
	}

	public override bool BeforeMethod(MethodInfo info)
	{
		_begin = DateTime.Now;
		return true;
	}

	public override async Task<bool> BeforeMethodAsync(MethodInfo info)
	{
		_begin = DateTime.Now;
		await Task.CompletedTask;
		return true;
	}

	public override void AfterMethod(ResultInfo result)
	{
		var span = DateTime.Now - _begin;
		_logger.LogInformation($"use {span.TotalMilliseconds:0.00}");
	}

	public override async Task AfterMethodAsync(ResultInfo result)
	{
		var span = DateTime.Now - _begin;
		_logger.LogInformation($"use {span.TotalMilliseconds:0.00}");
		await Task.CompletedTask;
	}
}
[MyPerfLog()]
public bool Login(string userName, string password)
{
    //check with db
    return true;
}

Then the compiler will take care the rest.

Risks

  1. Application Performance: should not be affected.
  2. Compiler complexity: should be fine.
  3. Extensibility: should be fine.
  4. Applicability: AOP is a very common requirement. Such compiler supported pre-compile AOP attribute should be very sweet.

Alternative

  1. A built-in interceptor can also do such things easily. A built-in interceptor will not need to change compiler, but will slow down the application startup speed. Application startup speed is important in this docker(container) era.
@wsq003 wsq003 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Sep 22, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Runtime.CompilerServices untriaged New issue has not been triaged by the area owner labels Sep 22, 2021
@ghost ghost added this to Needs triage in Triage POD for Meta, Reflection, etc Sep 22, 2021
@acaly
Copy link

acaly commented Sep 24, 2021

I think it's probably better to at least sketch how this should be implemented. For example, I can't figure out how async interceptor methods can be applied on sync methods.

Another problem is that this might be too complicated and specific to be a pure runtime feature. It might be better to propose that the runtime only exposes a more general API (for example, to raise an event that allows the program to modify the IL of a method while it is loaded), which the C# compiler can use to make this happen. Again, I think it is necessary to provide more information on how this should be designed and implemented.

I also want to remind you that C# team are known to have negative attitudes toward AOP. That's why there are so many third-party tools in this area.

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

I think it's probably better to at least sketch how this should be implemented. For example, I can't figure out how async interceptor methods can be applied on sync methods.

I am not compiler expert. There is a Java IDE plugin called Lombok which can decorate Java code before real compile. I think that would be a good reference.

Sync methods use sync interceptor. Async method use async interceptor. This is why AopAttribute need both sync and async implementation.

Another problem is that this might be too complicated and specific to be a pure runtime feature. It might be better to propose that the runtime only exposes a more general API (for example, to raise an event that allows the program to modify the IL of a method while it is loaded), which the C# compiler can use to make this happen. Again, I think it is necessary to provide more information on how this should be designed and implemented.

I suppose this AopAttribute may be supported in code-gen module. There should be a code-gen module to handle different kinds of syntactic sugar before real compile.
I am not sure where code-gen belong to. Rumetime, compiler or somewhere else.

I also want to remind you that C# team are known to have negative attitudes toward AOP. That's why there are so many third-party tools in this area.

Yeah, more or less I know that. But AOP is a big area and have many types.
https://stackoverflow.com/questions/39448543/run-time-aop-vs-compile-time-aop
I believe Source code weaving should be sweet and will cause minimal side effect.

@davidfowl
Copy link
Member

Why not use something like https://github.com/vescon/MethodBoundaryAspect.Fody

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

Why not use something like https://github.com/vescon/MethodBoundaryAspect.Fody

Three reasons:

  1. MethodBoundaryAspect.Fody do not have async version wrapper, while async is very import and common used today.
  2. MethodBoundaryAspect.Fody use Binary weaving with certain draw back.
  3. MethodBoundaryAspect.Fody has too few stars/forks, means it's maturity is not enough. I would prefer an official implementation which will be more promising.

Edited: MethodBoundaryAspect.Fody can support async method, but itself do not have async version.
Edited: MethodBoundaryAspect.Fody use Binary weaving not Reflection

@davidfowl
Copy link
Member

I would prefer an official implementation which will be more promising.

I'm pretty sure we're not going to build AOP into the runtime. Everyone would prefer everything to be built into the runtime even though they are packages on nuget that can do similar things.

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

I would prefer an official implementation which will be more promising.

I'm pretty sure we're not going to build AOP into the runtime. Everyone would prefer everything to be built into the runtime even though they are packages on nuget that can do similar things.

I agree that the runetime should not combine with dynamic-inject style AOP.

  1. That will cause real action separated from source code. And such separation will make programmer confused.
  2. Performance issue.

But some kine of Source code weaving may be acceptable.

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

https://github.com/vescon/MethodBoundaryAspect.Fody#asynchronous-sample

https://github.com/vescon/MethodBoundaryAspect.Fody#asynchronous-sample

MethodBoundaryAspect.Fody can support async method, but the OnEntry do not have async version. This would cause sync-over-async if someone want to do some async job in OnEntry, which is very possible.

https://github.com/vescon/MethodBoundaryAspect.Fody#asynchronous-sample

Are you sure it uses reflection?

Sorry my bad, seems like it uses Binary weaving like PostSharp.

@acaly
Copy link

acaly commented Sep 24, 2021

Another reason to avoid Fody related stuff is its strange License.

Actually I think the C# source generator can be a good replacement to Fody in many cases. For example, the async interception can be designed as:

//Ask the source generator to generate a method with name "YourPublicMethod"
[GenerateInterceptedMethod("YourPublicMethod")]
[Log] //Add anything you want as attributes here.
private async Task YourImplementation() //The name of this method is only used by the source generator.
{
  //Actual implementation.
}

Then the generator will generate:

public async Task YourPublicMethod()
{
    await Log.BeforeMethod(...);
    await YourImplementation();
    await Log.AfterMethod(...);
}

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

Another reason to avoid Fody related stuff is its strange License.

Actually I think the C# source generator can be a good replacement to Fody in many cases. For example, the async interception can be designed as:

//Ask the source generator to generate a method with name "YourPublicMethod"
[GenerateInterceptedMethod("YourPublicMethod")]
[Log] //Add anything you want as attributes here.
private async Task YourImplementation() //The name of this method is only used by the source generator.
{
  //Actual implementation.
}

Then the generator will generate:

public async Task YourPublicMethod()
{
    await Log.BeforeMethod(...);
    await YourImplementation();
    await Log.AfterMethod(...);
}

Yes! Some kind of source code generator before real compile would be straightforward and elegant.

@acaly
Copy link

acaly commented Sep 24, 2021

Yes! Some kind of source code generator before real compile would be straightforward and elegant.

Source generator is very versatile. The only issue is that it's still new and you probably have to write your own generator for your specific task, including something like this AOP feature.

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

Yes! Some kind of source code generator before real compile would be straightforward and elegant.

Source generator is very versatile. The only issue is that it's still new and you probably have to write your own generator for your specific task, including something like this AOP feature.

This is why I proposal a abstract class AopAttribute: let built-in code-gen or compiler to do such souce code generator staff in a limited pre-designed way.
This would be super simple for programmer to understand and use.

@davidfowl
Copy link
Member

Source generators won't help you as they can't change the call site. You can experiment with them anyways because this issue isn't something we plan to do

@acaly
Copy link

acaly commented Sep 24, 2021

Source generators won't help you as they can't change the call site.

You don't need to. The actual implementation can be another private method, while all call sites use the generated, public one.

@acaly
Copy link

acaly commented Sep 24, 2021

The problem with integrating this into the runtime is that only very few apps can benefit from it. In this case, a third-party library might be a better way to go.

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

The problem with integrating this into the runtime is that only very few apps can benefit from it. In this case, a third-party library might be a better way to go.

As mentioned in MethodBoundaryAspect.Fody, with AOP:

You can easily write your own aspects for

transaction handling
logging
measuring method execution time
exception wrapping
displaying wait cursor
and much more ...

Transaction ,logging, caching, is very common in daily programming.
A third-party library can not do souce code weaving.

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

Source generators won't help you as they can't change the call site. You can experiment with them anyways because this issue isn't something we plan to do

Maybe AOP is not a good name for this issue.
What we need here is a pre-defined source generator. Just like that compiler will expand using block to try...finally with dispose.

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

Found one another possible alternative https://github.com/pamidur/aspect-injector

@wsq003
Copy link
Author

wsq003 commented Sep 24, 2021

https://github.com/vescon/MethodBoundaryAspect.Fody#asynchronous-sample

https://github.com/vescon/MethodBoundaryAspect.Fody#asynchronous-sample

MethodBoundaryAspect.Fody can support async method, but the OnEntry do not have async version. This would cause sync-over-async if someone want to do some async job in OnEntry, which is very possible.

https://github.com/vescon/MethodBoundaryAspect.Fody#asynchronous-sample

Are you sure it uses reflection?

Sorry my bad, seems like it uses Binary weaving like PostSharp.

I strongly against third-party compile-time weaving or binary weaving.
Because they are very likely to break things: debugging information, call stack, compile tricks, IL rules, backward compatibility, etc.
I prefer do such things before compile.

@HaloFour
Copy link

I'd much rather continuing to lobby the C# team to make source generators handle AOP scenarios better. Generators themselves might be more difficult to write, but theoretically you could write a general purpose generator that would recognize attributes that follow a specific convention (or implement an interface) and have that generator emit the code to call into those attributes, which would behave similarly to what you've proposed here.

@davidfowl
Copy link
Member

I don't know how you'd build this with what source generators offer today but here's something roslyn based https://github.com/Jishun/RoslynWeave

@wsq003
Copy link
Author

wsq003 commented Sep 26, 2021

I don't know how you'd build this with what source generators offer today but here's something roslyn based https://github.com/Jishun/RoslynWeave

He is trying to achieve the same goal. He use source code template to create a new wrapper class in another namespace, then all caller need to change their code to call the new class.
He did not fully accomplish the goal, but he did not have much chioce, because Roslyn doesn't support code-rewritting
.
So Roslyn may not be our chioce.

I guess there should already be some code-gen that support code-rewriting, inside or outside compiler.
This code-gen had processed some syntactic sugar such as using:

using (var conn = new Connection())
{
}

rewrited to

Connection conn;
try
{
  conn = new Connection()
}
finally
{
  conn.Dispose();
}

So I am thinking if such code-gen can do another similiar rewrite:

[MyPerfLog()]
public bool Login(string userName, string password)
{
    //check with db
    return true;
}

rewrite to:

public bool Login(string userName, string password)
{
    MyPerfLog.BeforeMethod();
    
    var resultInfo = Login_juedhrygjddhdgd(userName, password);
    
    MyPerfLog.AfterMethod();
}

public bool Login_juedhrygjddhdgd(string userName, string password)
{
    //check with db
    return true;
}

If such code-gen is already physically existed, maybe we can do some work there.
If such code-gen is only logically existed, maybe we should extract such functionalities to a dedicate module. This dedicate module can be called 'code preprocessing module' or something.

@wsq003
Copy link
Author

wsq003 commented Sep 26, 2021

Maybe JSR 269 Pluggable Annotation Processing API is what we need.
Based on JSR-269, they created Lombok.

@joperezr joperezr added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels Sep 28, 2021
@joperezr joperezr added this to the Future milestone Sep 28, 2021
@joperezr joperezr moved this from Needs triage to Future in Triage POD for Meta, Reflection, etc Sep 28, 2021
@wsq003
Copy link
Author

wsq003 commented Sep 22, 2022

https://github.com/postsharp/Metalama
Looks interesting. It is designed to replace or enhance hand-written code. Just like what I want.

@wsq003
Copy link
Author

wsq003 commented Apr 29, 2024

https://github.com/dotnet/roslyn/blob/main/docs/features/interceptors.md

This looks like what I had wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices
Projects
No open projects
Development

No branches or pull requests

5 participants