EnC - Report rude edit when a user adds an AsyncMethodBuild attribute#55877
EnC - Report rude edit when a user adds an AsyncMethodBuild attribute#55877davidwengier merged 6 commits intodotnet:mainfrom
Conversation
| // * When the target is a type, for any await call to a method that returns that type | ||
| // | ||
| // Therefore applying this attribute can cause unbounded changes to emitted code anywhere in a project | ||
| // which EnC wouldn't pick up, so we block it with a rude edit |
There was a problem hiding this comment.
Didn't we say we are ok with that in general, similar to changing a using directive?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Please, add a test. Probably should test all the attributes listed above as well (Theory).
There was a problem hiding this comment.
There is a test that covers this switch, but a theory for all of the cases is a good idea.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Part of C# 10 support