dotnet / dotnet/reactive

Ideas for Roslyn code analyzers and code fix providers

Open
#1,397 3 comments 3 reactions 0 assignees View on GitHub
[area] Rx
Dominant language
C#
Stars
7.2k
Forks
798
PR merge metrics
No merged PRs in 30d

Description

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)` 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` 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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.