search

dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.82k stars 773 forks source link

Inconsistent 'missing `new`' warnings #16468

Open auduchinok opened 6 months ago

auduchinok commented 6 months ago

Consider this sample:

open System

type AsyncDisposable() =
    interface IAsyncDisposable with
        member this.DisposeAsync() = failwith "todo"

type Disposable() =
    interface IDisposable with
        member this.Dispose() = failwith "todo" 

AsyncDisposable() |> ignore
Disposable() |> ignore

There's an inconsistency in how different kinds of disposables are checked in terms of requiring new keyword. I think we both interfaces should probably be treated equally:

Screenshot 2023-12-25 at 11 57 20
vzarytovskii commented 6 months ago

I don't think it's a bug, IAD wasn't there when the rule was introduced. There were ideas of not requiring new even for IDisposable.

auduchinok commented 6 months ago

IAD wasn't there when the rule was introduced.

Yes, I understand that. But currently it's there 🙂

There were ideas of not requiring new even for IDisposable.

I think it could be nice. Somewhat independently of this proposal, I think the best thing we could do is to make it consistent for these two types.

vzarytovskii commented 6 months ago

IAD wasn't there when the rule was introduced.

Yes, I understand that. But currently it's there 🙂

Yeah, what I meant is it's not a bug, but very much on purpose.

There were ideas of not requiring new even for IDisposable.

I think it could be nice. Somewhat independently of this proposal, I think the best thing we could do is to make it consistent for these two types.

I personally think we should just remove it, and have it as analyzer (when the sdk is ready). Adding it for IAD will be a breaking change.

smoothdeveloper commented 6 months ago

What about the semantic coloring that takes care of highlighting IDisposable in a different manner?

https://github.com/dotnet/fsharp/blob/99514c0fafa1f4a9ddf63e0439ec8804d87276eb/src/Compiler/Service/SemanticClassification.fs#L39

IIUC, we'd also like the same semantic classification to apply to IAsyncDisposable.

abelbraaksma commented 1 month ago

I personally think we should just remove it, and have it as analyzer (when the sdk is ready).

I'm not entirely against that. However, currently the missing new is the only thing that reminds users that something implements IDisposable, which isn't always immediately obvious.

I think the better fix would be to have an analyzer that warns for forgetting to use use or use! in cases like these. Such warning is currently not present and I've seen too much code that I care to remember that had resource leaks. Specifically F# should be good at warning for missing resource cleanup, no?

Missing use could be a compiler-supported warning, and missing new sounds more like an analyzer feature to me.

(but as with many things, it is probably not as cut and dry to implement as I'd like it to be)