Feature/1898 fix diagnostics by idg10 · Pull Request #1928 · dotnet/reactive (original) (raw)
added 30 commits
This enables us to re-enable warnings CS8600, CS8602, CS8603, CS8604, and CS8632.
When support for nullable reference types was added to Rx in 2020, you were not allowed to write T? if T was an unconstrained type parameter. Although that made sense - if T could be either a value type or a reference type, T? could mean fundamentally different things for different type arguments - in practice it was unhelpful because it meant there was no straightforward way to indicate "nullable in cases where T is a reference type", which is often what was required in practice.
C# 9.0 added a feature that removed this limitation: https://github.com/dotnet/csharplang/blob/main/proposals/csharp-9.0/unconstrained-type-parameter-annotations.md
This commit takes advantage of this language feature to better express some of the existing null handling.
The analyzer was producing a spurious diagnostic for TaskObservableMethodBuilder - the rules for how method builders work require them to be generic, and to define a static Create method.
This analyzer wanted IEventSource.OnNext and IEventPatternSource.OnNext to use EventHandler, but by design these takes the unusual step of using other delegate types for an event.
No longer causing warnings, possibly because changes in earlier commits have now resolved these issues.
Add suppression to ScheduledItem because this analyzer wanted us to make its IDisposable implementation overridable, which would not really be appropriate for that specialized base type.
Suppressed spurious warnings on the TaskObservableMethodBuilder, which has to use ref because it's an async method builder.
Added suppression to CA1052 because it is designed to be inherited from, despite having no instance members.
Add suppressions for the classes where this analyzer wants us to make breaking changes to the public API.
Suppress in the test code that this analyzer identifies, whihc doesn't need the recommended change.
Make suggested changes for internal methods. Add suppression for the two public methods for which this analyzer suggests a breaking change (and for the one internal method where the signature is determined by the ISchedulerLongRunning interface and can't be changed).
Took all recommended changes.
This no longer seems to show up. Perhaps other changes have now addressed this.
Add suppressions for the public types that have existed for many years.
Suppress the one place this showed up. It was complaining about the use of 'error' as an argument name. It has been that way for years, and changing this name would break any code using named arguments.
This didn't like Single, but that's a long-established LINQ method name, so we just have to suppress the warning.
Added suppressions in the various tests for which this was a spurious message, because they all expect the relevant constructor to throw.
Fixed one actual bug! (ForkJoin_Nary_Immediate called SequenceEqual but never inspected the result. It should have been AssertEqual.)
Suppressed on task builder where it's not required.
Add suppression for ScheduledItem for same reason as other IDisposable-related suppressions.
Now disabled in test projects only, because it would take a lot of work to adjust the test projects to fit this rule, with little benefit.
Remains disabled in test projects, because the impact on readability is not justified by the minimal performance benefits there.
Now disabled for tests only. This is a fairly low-level change that won't have a measurable impact on test performance, so we only want this enabled for the production libraries.
A few tests were deliberately not forwarding it in order to test scenarios where that happens. These now do so explicitly to avoid the warning (or, in the one case where we specifically want to test an Rx overload that doesn't take a CancellationToken argument, we just suppress the message).
Added a couple of suppressions because for backwards compatibility, we need to continue to throw NullReferenceException in a couple of places.
Also add comment explaining why we're leaving CA2213 disabled.
Set to silent suggest in .editorconfig, because the analyzer tends to be too keen: it suggests it even for individual properties, with the result that its 'simpler' suggestion is more verbose than what it replaces.
Enabled IDE0018, IDE0034, IDE0039, IDE0056, IDE0057, IDE0090 and IDE0180
Also brought a few straggling non-var declarations into line with the (obviously wrong, but sadly established) policy of using var everywhere.
IDE0019, IDE0020, IDE0038 and IDE0083.
Used modern patterns where suggested.
But we're downgrading it from suggest to silent, because in all the cases I looked at where this suggested a change, the existing use of fields made it easier to see how the internal state of the various objects worked.
Also change a corresponding use of anonymous types to tuples.
IDE0040 and IDE0044.
Also added various missing private and readonly modifiers.
Added suppressions in cases where unused parameters are present by design.
Removed a load of CA1062 suppressions because that warning applies to public members (it checks that arguments are checked) but it had been applied to a load of non-public members.
Updated the SuppressMessage attributes in question to use the modern format.
Move over more comprehensively to Roslyn analyzers. (It's not clear whether the old style analyzers were still running before. But some of the suppressions were causing problems.)
Tweaks to scoping rules in C# 11.0 mean we can now use nameof in a few cases where it used not to work. (Method parameter names are now in scope in parameter and return: attributes.)
I think this one was a typo - currently there is no such diagnostic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})