Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/1898 fix diagnostics #1928

Merged
merged 53 commits into from
May 11, 2023
Merged

Feature/1898 fix diagnostics #1928

merged 53 commits into from
May 11, 2023

Conversation

idg10
Copy link
Collaborator

@idg10 idg10 commented May 11, 2023

In order to get Rx building on current SDKs after it had not been updated for a while, we disabled several diagnostics. This was always intended as a temporary step.

This change re-enables them, and also brings the use of diagnostics into line with current practices. E.g., the old Rx.ruleset file has gone because that was associated with the old IL-based analyzers, and is not how you configure the new Roslyn-based analyzers.

Also, some of the analyzers are not quite the same in their re-implemented form, so a few changes have become necessary.

We've also currently pinned the analyzer level to 7.0, so it will take an explicit change to re-align with newer warning levels as new SDKs become available. This will reduce friction when SDK 8.0 appears, but will also require explicit work to upgrade to the newer warnings when that occurs.

idg10 added 30 commits May 3, 2023 12:21
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<T> - the rules for how method builders work require them to be generic, and to define a static Create method.
This analyzer wanted IEventSource<T>.OnNext and IEventPatternSource<T>.OnNext to use EventHandler<T>, 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.
idg10 added 21 commits May 4, 2023 15:16
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.
@idg10 idg10 self-assigned this May 11, 2023
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1014:MarkAssembliesWithClsCompliant")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1004:GenericMethodsShouldProvideTypeParameter", Scope = "member", Target = "Microsoft.Reactive.Testing.ReactiveAssert.#Throws`1(System.Action,System.String)")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1004:GenericMethodsShouldProvideTypeParameter", Scope = "member", Target = "Microsoft.Reactive.Testing.ReactiveAssert.#Throws`1(System.Action)")]
[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1004:GenericMethodsShouldProvideTypeParameter", Scope = "member", Target = "~M:Microsoft.Reactive.Testing.ReactiveAssert.Throws``1(System.Action,System.String)")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is ~M:?

@idg10 idg10 marked this pull request as ready for review May 11, 2023 13:13
Copy link
Collaborator

@HowardvanRooijen HowardvanRooijen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@idg10 idg10 merged commit b0113d5 into main May 11, 2023
@idg10 idg10 deleted the feature/1898-fix-diagnostics branch May 11, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants