EnC - Report rude edit when a user adds an AsyncMethodBuild attribute

[davidwengier]

Part of C# 10 support

[tmat]

Didn't we say we are ok with that in general, similar to changing a using directive?

[tmat]

It's true that adding/removing the attribute wouldn't have the desired effect unless one also changes the call sites, so it might be a bit confusing. But I wonder if we have to block it because of that. Maybe the user is actually planning on updating the call site. Perhaps file an issue to improve the experience by using FAR to locate the call sites and report warnings? Not worth doing since it's unlikely that someone cares, but just so we track it.

[davidwengier]

There is certainly an argument to just do nothing here, like with using directives, but I feel like with those the most likely scenarios are either directly modifying code (eg, adding a using via completion etc.) or an ambiguous type error. It seems pretty convoluted to get a situation where a using is added, which changes semantic meaning, but causes no diagnostics.

With this attribute every scenario falls in to the latter bucket: You add it, and there is no indication that it won't do what you expect.

I don't feel strongly about it either way, so I'm fine to just abandon this if you don't think its worth it.

[tmat]

Actually, you are right that this should be rude edit - an existing unupdated call site would throw MethodMissingException if this is allowed since we change the method signature, right?

[tmat]

Please, add a test. Probably should test all the attributes listed above as well (Theory).

[davidwengier]

There is a test that covers this switch, but a theory for all of the cases is a good idea.

[davidwengier]

an existing unupdated call site would throw MethodMissingException if this is allowed since we change the method signature, right?

I don't think I follow. An unupdated call site would just continue to use the old method builder, not the user specified one.
See: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-10.0/async-method-builders#execution

[tmat]

Oh, for some reason I thought it has effect on the caller. Never mind. If you change it on a method then the method is recompiled so it makes a difference, right? If you change it on a type we don't recompile all of the type's async methods, so that might be problematic.

[davidwengier]

For a method I think you're right, and actually I was mistaken and my comment is wrong, so we could allow it for methods. I read it as called to the method for some reason.

If you change it on a type we don't recompile all of the type's async methods

It's worse actually, for types we would need to recompile any async method that returns that type (though that isn't a C# 10 change)

[tmat]