resharper-fsharp icon indicating copy to clipboard operation
resharper-fsharp copied to clipboard

require/dont suggest removing `new` for IAsyncDisposable construction

Open bartelink opened this issue 2 years ago • 3 comments

The plugin greys out new on the construction of an IAsyncDisposable in https://github.com/bartelink/FSharp.Control.TaskSeq/blob/idisposable/src/FSharp.Control.TaskSeq/TaskSeqInternal.fs#L928

I blindly followed that in https://github.com/fsprojects/FSharp.Control.TaskSeq/pull/220/commits/cfc96240233e5a9cc4822e6a43f342102c64d629, but

Thankfully @abelbraaksma caught the fact that this was erroneous for me to remove it (i.e. new should be mandatory for Disposables inc Async Disposables)

In this instance I'll sidestep it by using IDisposable in https://github.com/fsprojects/FSharp.Control.TaskSeq/pull/222

Ideally, the compiler would trap/complain/warn about the missing new, but I'll settle for Rider not suggesting to remove it.

bartelink avatar Dec 23 '23 15:12 bartelink

I don't think this should be rider feature instead of compiler feature. E.g. things like that should be consistent across all ides. It's not a refactoring feature, it's more of a design space/code style stuff and should be in the compiler.

Personally, tho, I always suppress new everywhere as I never found it useful for anything. It just outlines a single case where disposable is created out of 100 possible ways and manages to break some syntax constructs for no reason.

I do have a custom plugin that highlights disposables differently (both variables and types) and I find that working much much better for me personally.

En3Tho avatar Dec 23 '23 17:12 En3Tho

Not suggesting it should be a Rider feature - just that it shouldn't be a misfeature. Right now:

  • if something is IDisposable, it does not suggest removing the new
  • if it's IAsyncDisposable, it does (and there is an autofix)

The suggestions and fixes from the plugin are of a consistently high quality, so I highlight all anomalies (inc esoteric ones e.g. #587)

I agree that there's a big language level discussion outside of this, and in a prioritization discussion, doing stuff upstream is absolutely the only way here (I want to know a use is going to work for both, and be consistent about insisting on new or not).

Your plugin sounds like something that would be excellent to have worked into default rendering (and as part of that, supporting IAsyncDisposable and IDisposable equally would be a concern)

bartelink avatar Dec 23 '23 17:12 bartelink

Not suggesting it should be a Rider feature - just that it shouldn't be a misfeature. Right now:

if something is IDisposable, it does not suggest removing the new if it's IAsyncDisposable, it does (and there is an autofix)

Unfortunately, it's inline with the current language design, i.e. there's a compiler warning about a missing new reported for IDisposable, but there's none for IAsyncDisposable. If we add a special case for IAsyncDisposable, then it'll be somewhat incompatible with the compiler warnings design. We can probably do that, but I'm also interested in knowing what the compiler team thinks about the main issue, so I've reported it there: https://github.com/dotnet/fsharp/issues/16468.

auduchinok avatar Dec 25 '23 11:12 auduchinok