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

RedundantNewKeyword is raised on a type inheriting from an interface that inherits IDisposable #572

Open
jp-fournier-dev opened this issue Oct 17, 2022 · 5 comments · May be fixed by #577

Comments

@jp-fournier-dev
Copy link
Contributor

Description

Given the following code :

open System

type ISomeInterfaceWithDisposable =
    interface
        inherit IDisposable
    end

type SomeDisposableType() =
    interface ISomeInterfaceWithDisposable with
        member _.Dispose() = ()

module Program =
    let foo = new SomeDisposableType() :> ISomeInterfaceWithDisposable

Considering that the type is a disposable through the ISomeInterfaceWithDisposable interface, my expectations is that this needs a new keyword.

Repro steps

Run FSharpLint on the following code with 0.21.3

Expected behavior

The redundant key word error to not be raised (this is what I would expect)

Actual behavior

The redundant key word error is raised

Known workarounds

Remove the new keyword from all occurence of this pattenr

Related information

Issue occurs on 0.21.3
Doesn't occur on 0.21.2
Windows 10
dotnet 6

@abelbraaksma
Copy link
Member

abelbraaksma commented Oct 21, 2022

Remove the new keyword from all occurence of this pattenr

Yes, that may be a workaround, but the F# Style Guide, and even the F# compiler, wants the new keyword for cases that implement IDisposable. So, the better workaround is to disable to rule altogether, as it currently just gives bad advice.

@abelbraaksma
Copy link
Member

abelbraaksma commented Oct 21, 2022

Interestingly, there's this function:

FSharpLint.Rules.RedundantNewKeyword.doesNotImplementIDisposable(FSharpCheckFileResults checkFile, LongIdentWithDots ident, Unit unitVar0)

which suggests that this check was actually enabled, but apparently doesn't quite find whether the interface is there.

@jp-fournier-dev jp-fournier-dev linked a pull request Oct 21, 2022 that will close this issue
@jp-fournier-dev
Copy link
Contributor Author

jp-fournier-dev commented Oct 21, 2022

Might be related to how it is inside of an upcast expression ? I've added a test for that case, but it passes 👀 event without my modifications. I'll investigate a bit maybe next week, I'll temporarily disable it in my projects for the moment

@webwarrior-ws
Copy link
Contributor

Could not reproduce this bug with or without upcast. If type implements IDisposable, no error is raised. This makes me think the bug was fixed.

@webwarrior-ws
Copy link
Contributor

Hey jp-fournier-dev, thanks for reporting this issue.

I couldn't reproduce what you found (see this commit where I've added a test with your code snippet, and check that CI status
is still green: webwarrior-ws@88ce98e). That commit is based on version 0.21.3.

It would be great if you provide us with more details or even a failure test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants