Add trimming-safe messaging overloads and migration analyzer - #7889
Add trimming-safe messaging overloads and migration analyzer#7889danielmarbach wants to merge 31 commits into
Conversation
| } | ||
|
|
||
| var eventTypeNames = new HashSet<string>(StringComparer.Ordinal); | ||
| foreach (var typeName in messageHierarchy.Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)) |
There was a problem hiding this comment.
Yes this is allocations, but I'm not concerned about those given it is the learning transport
| return eventTypeNames; | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("Trimming", "IL2070", Justification = "Used only for legacy or custom multicast operations without the enclosed message hierarchy header. Normal pipeline and outbox operations carry message metadata.")] |
There was a problem hiding this comment.
This is technically little it of a cheat but I think it is the better alternative compared to passing through interface DAM through everything and the kitchen sync for the learning transport. If you are doing super fancy stuff with the learning transport and you have no enclosed header we then fall back and in the worst case if you have not made sure the types have been preserved they might be lost here but I think that is acceptable and super unlikely.
| [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("Registering publishers by assembly or namespace requires assembly scanning and is" + | ||
| " not supported in trimming scenarios. Register publishers by message type instea" + | ||
| "d.")] |
There was a problem hiding this comment.
For context, which transports in NServiceBus 10+ even do message-driven pubsub anymore? Is it just ASQ?
My thinking is I can see how we could replace this with a source generator in the future but if it's a minority of transports and the depenencies of said transports don't show any signs of becoming trim-capable, then it's easy to justify. But I also wonder if we need to document that somewhere.
| [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("Routing messages by assembly or namespace requires assembly scanning and is not s" + | ||
| "upported in trimming scenarios. Register routes by message type instead.")] |
There was a problem hiding this comment.
In comparison, this one I would want to immediately raise an issue for creating an interceptor for this capability so a future version would not need to leave this method forbidden for AOT scenarios.
There was a problem hiding this comment.
I think the first simple step is to have an AddMessageType<>
| return Publish(context, typeof(T), messageMapper.CreateInstance(messageConstructor), options); | ||
| } | ||
|
|
||
| [UnconditionalSuppressMessage("Trimming", "IL2072", Justification = TypeErasedMessageSuppressionJustification)] |
There was a problem hiding this comment.
So this is interesting. The object overloads are actually "bad". The most honest thing to do here is to actually annotate with RequiresUnreferencedCode but then we don't have a generic path. But I think it would be possible to introduce one by adding a generic method with overload resolution that gets deprioritized by the compiler so that the object overloads take precedence to not break routing. When someone then switches to trimming, they get a warning that tells them to use the generic overload by explicitely specifying the message type. In v11 we can then kill the object overloads and remove the ordering of the APIs which then means people have potentially "redundant" type defintions they can resolve with the IDE automatically if they want or they can keep the generics defined because it then clearly expresses the "type to route to"
|
Converted back to draft since there are a few things I need to finish up |
danielmarbach
changed the title
8cc473b to
876ecc1
Compare
| public Task Timeout(DelayReply state, IMessageHandlerContext context) => | ||
| //reply to originator must be used here since the sender of the incoming message is the TimeoutManager and not the requesting saga | ||
| ReplyToOriginator(context, new ResponseFromOtherSaga | ||
| ReplyToOriginator<ResponseFromOtherSaga>(context, new ResponseFromOtherSaga |
There was a problem hiding this comment.
This is a little unfortunate but I think it is the only way to have a trim safe API surface and not break routing as well as getting rid of the old overload eventually. The thing is once the other one is gone they can simply remove again the generic type specification. I'm all ears if someone has better suggestions
|
@andreasohlund @DavidBoike if you could this a first look over I'd appreciate it. I added some more inline thoughts and I'm leaving it as draft for now although I think "this is ready". I will definitely also do another self-review a bit laster but the most important part is that we agree on the direction this is going and decide what should stay or what should move out FYI The trimming approval test is a "slight lie". Everytime we annotate something else the warnings are moving so you cannot say "oh nice we are almost done because the file looks empty". |
b981bc9 to
876ecc1
Compare
|
waiting for another approval then I merge and smoke test a few samples |
DavidBoike
left a comment
DavidBoike
left a comment
There was a problem hiding this comment.
I tried the alpha in InternalAutomation and immediately got 31 instances of NSB0040 warnings, which there is no fixer for.
Even if there was a fixer for NSB0040, this is a bad user experience for a minor. I thought the overload priorities were all to make it so that an end user who is not interested in trimming or AOT could happily continue with Send(object, options) until the non-trimmable object overloads were deprecated or removed in NServiceBus 11 and would never have to change to the "of T" version only to have to later change back. I thought that dance would only be necessary for those who wanted trimming.
The analyzer elevating this to a warning changes that.
If the analyzer only lit up when trimming was enabled, then that would probably be a different story.
But forcing all existing users (assuming they are fine without trimming) to either ignore warnings, pragma them all, add an editor config to ignore NSB0040, or update to a code shape (without help from a fixer) that won't be necessary in the future, in support of a feature they didn't want and that this PR doesn't even fully deliver seems too much.
| using System; | ||
| using System.Collections.Concurrent; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Runtime.CompilerServices; |
There was a problem hiding this comment.
Seeing the changes to these fakes, how much of these changes will also need to be immediately replicated to NServiceBus.Testing?
| using Particular.AnalyzerTesting; | ||
|
|
||
| [TestFixture] | ||
| public class MessagingMigrationAnalyzerTests : AnalyzerTestFixture<MessagingMigrationAnalyzer> |
There was a problem hiding this comment.
Will benefit from Particular/Particular.AnalyzerTesting#41
…o fall back to any form of reflection
618972f to
e983544
Compare
e983544 to
d543f05
Compare
3 participants
What this changes
This PR gives trimming and NativeAOT users a safe, explicit path to typed message APIs without changing routing or creating warning noise for every existing NServiceBus application.
The core rule is compatibility: an ordinary call such as
session.Send(message, options)continues to select the existing object overload and route using the runtime type. A call enters the typed path only when it explicitly selects a generic overload or supplies an explicit logicalType.Who needs to act now
Applications that do not use trimming or NativeAOT
No action is required.
Existing object-overload calls keep their routing behavior and do not receive
NSB0039orNSB0040warnings by default. This avoids requiring an explicit<T>migration now only for ordinary generic inference to make that syntax unnecessary in a later major version.Applications that use trimming or NativeAOT
Migrate object-overload calls to a trimming-safe alternative:
Send<T>,Publish<T>,Reply<T>,SendLocal<T>, orUpdateMessage<T>when the intended logical type is known at compile time.Send(object, Type, ...),Publish(object, Type, ...), orReply(object, Type, ...)when middleware or platform code has lost the generic type but still has a known logical type.NSB0040as a routing decision, not a mechanical migration. It identifies calls where the runtime type can differ from the static type, so changing to<T>can change routing, subscriptions, or logical message identity.The migration diagnostics activate when a project enables
PublishTrimmed,PublishAot,IsAotCompatible,IsTrimmable,EnableTrimAnalyzer, ornservicebus_enable_message_overload_migration_diagnostics = truein.editorconfig.What this PR contains
Trimming-safe message APIs
The object overloads of
Send,Publish,Reply,SendLocal,UpdateMessage, andSaga.ReplyToOriginatordiscover a message type throughmessage.GetType()and are now annotated withRequiresUnreferencedCode.Generic overloads carry
typeof(T)statically through the pipeline. Explicit-Type overloads cover type-erased middleware and platform scenarios without requiringMethodInfo.MakeGenericMethod.The generic existing-instance overloads use
[OverloadResolutionPriority(-1)], so existing source keeps selecting object overloads and preserves runtime-type routing on recompilation.Default interface bodies preserve binary compatibility for third-party implementations by falling back to the existing object method. Built-in NServiceBus implementations override the typed members and preserve the declared logical type. Test fakes expose concrete typed and explicit-Type overloads.
Saga.ReplyToOriginator(object)is the exception: it is a concrete protected method without an interface mirror, so it receives a normal[Obsolete]warning. Interface members usePreObsolete, because a warning-level[Obsolete]would trap third-party implementers who still have to implement the interface member.Migration analyzer and code fix
MessagingMigrationAnalyzerprovides three diagnostics:NSB0039identifies object-overload calls that can be safely converted to a typed overload and offers a code fix.NSB0040identifies calls where runtime routing may differ from static routing. It intentionally has no fixer.NSB0041identifies explicitSend<object>(...)-style typed calls. It remains enabled for all projects because this is independently incorrect.NSB0039andNSB0040are intentionally quiet in ordinary 10.x builds. They are active for trimming/AOT or explicit migration audits. This is the first stage of a two-major deprecation path:Trimming annotations on routing and subscription registration
Assembly- and namespace-based route/publisher registration scans assemblies at startup and cannot be made trimming-safe. The public configuration boundary and internal scanning implementation are annotated with
RequiresUnreferencedCoderather than relying on local suppressions.The runtime-type-routing warning text now explicitly applies when trimming is enabled and points to generic or explicit-Type alternatives.
Trimmability warning cleanup
The approved warning list shrank to four intentional boundaries:
The legacy
System.Webhosting detection was removed because it reflected over an assembly that most endpoints no longer reference.Out of scope
RegisterStep), covered by separate convention-based pipeline behavior work.Tradeoffs
OverloadResolutionPriority(-1)<T>migration.NSB0040no code fix[Obsolete]without trapping third-party implementers.