Ideas for Roslyn code analyzers and code fix providers

[bartdesmet]

It'd be good to augment the library with Roslyn extensions that suggest on library-specific code improvements in user code. A couple of examples come to mind immediately (see below), but I can imagine many more based on typical Rx guidance, known anti-patterns, performance pitfalls, general LINQ patterns, etc.

At the very least, these should be semantics-preserving and hence "safe" code fixes to apply; it should make the user's code strictly better (and definitely/obviously not worse) so the bar for such analyzers and code fixes will be kept very high.

This could be a longer term design theme or workstream, where we can start with just a few simple ones, at least to get the shipping and packaging logistics for such analyzers set up.

We should also use https://apisof.net/catalog/System.Reactive.Linq.Observable to figure out areas that are most commonly used when ranking possible suggestions for analyzers. The few ideas below show fairly common usage; Observable is used in about 2.8% of apps, so all operator uses should be measured relative to this. For example, some reflection-based overloads of FromEventPattern are used in 0.2% of apps, which still accounts for almost 10% of Rx apps.

FromEvent with reflection

Suggest changing Observable.FromEvent[Pattern]([expr | type], "Foo") (and variants with a scheduler parameter) to another Observable.FromEvent[Pattern] overload that doesn't use reflection. This involves deriving the event handler type, its arguments, etc. in order to construct the replacement invocation, while also creating lambdas (possibly a conversion lambda, but at least add/remove handler lambdas).

This isn't always safe to do, because the way the reflection-based lookup works, using the dynamic type of expr (for better or for worse, but that's what we have done historically and we won't change it). So this may need checks for being sealed etc. which may limit its usefulness (though such issues don't apply to the overload that takes in Type rather than object, for static events). Even if there are limits due to the dynamic nature of the reflection-based binding process, we may still be able to suggest a nameof(Bar.Foo) fix to replace the string literal if we can find a candidate event (i.e. only we, the Rx library, understand that the string represents an event).

Design TBD, but the overall idea of avoiding reflection and magic names in strings is worthwhile.

Avoid blocking operations

E.g. use of Single, First, Last, or variants thereof (as well as other blocking operations) within an async method could be replaced by an await expression using the Async variant of these operators.

Design TBD, in particular with regards to ConfigureAwait behavior (we can suggest different options to the user as well). Either way, this reflects our intent to make the blocking operations obsolete, or at the very least discourage their use.

Random ideas

• Suggest handling exceptions directly rather than using Subscribe(Action<T>) extension methods that omit the error handler (and will rethrow exceptions in possibly surprising places). The suggested replacement should be semantically equivalent, so it'd have to add the Action<Exception> parameter using ExceptionDispatchInfo.Capture(ex).Rethrow(). But it can provide alternatives that clearly state /* insert exception handling code here */.
• Specify scheduler parameters everywhere, e.g. if someone wants to make a query ready to be plugged into virtual time tests as well as a production environment. Helps to discover all the operators that take a scheduler. It should pick the semantically equivalent scheduler, or null when that's valid (and likely use named parameter scheduler: null syntax if does so, to make the code self-documented).
• Convert form query expression syntax to "fluent" interface style (e.g. where becomes .Where). Non-trivial with transparent identifiers due to multiple from clauses, let clauses, etc. But doable for simpler cases.
• Query optimizations that are worthwhile, e.g. fusion of adjacent Where clauses using && or Select clauses that can be safely combined (i.e. ordering of side-effects doesn't change, not possible in the general case). May be very limited in its applicability (or not really occur much in real life) so needs more pondering for sure.
• Warn about concurrency pitfalls around the use of ObserveOn and SubscribeOn (or lack thereof). Analysis may be involved, e.g. in the context of UI frameworks, but there may be stuff to do here. Another example that comes to mind is loss of events when having a "time gap", e.g. xs.GroupBy(...).SelectMany(g => g.SubscribeOn(...).Etc().Etc().Etc()) or similarly for Window. These are tricky bugs for users to find (but it's also not clear what a suggested fix could be, so maybe it's just a warning).

Either way, this issue can be used to brainstorm ideas. Curious to see what people come up with. Based on ideas, we can decide how to move forward.

[quinmars]

The analyzer could suggest to use a scheduler for the operator instead of a subsequent ObserveOn, e.g., Throttle(TimeSpan.FromSeconds(1), scheduler) instead of Throttle(TimeSpan.FromSeconds(1)).ObserveOn(scheduler).

[bartdesmet]

@quinmars - Great suggestion.

[JohanLarsson]

Refactoring that changes += to Observable.FromEvent I have a couple of analyzers and fixes that can be moved to this analyzer. My suggestion on moving forward is

1. Create an analyzer and test project.
2. Ship analyzers on nuget.