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.9k stars 785 forks source link

open FSharp.Core isn't marked as unused #6975

Open auduchinok opened 5 years ago

auduchinok commented 5 years ago

This namespace is implicitly added to all F# files and it is not accounted when checking symbols from that namespace in Unused Opens analysis logic.

Screenshot 2019-06-11 at 11 00 43
TysonMN commented 4 years ago

Here is a MWE of this bug in text...

module Top
open FSharp.Core
let [<Literal>] x = 0

...and as a failing test in this commit.

abelbraaksma commented 4 years ago

But is this really a bug? The lib is used in this example through the attribute, and there a valid reasons you may have to open it explicitly, ie to prevent shadowing from other opens.

cartermp commented 4 years ago

FSharp.Core is special yet again here. Typically the answer for shadowing is to fully qualify, but I guess I could see why someone would want to have it opened for a different scope. Given that I don't think this is a clear bug anymore.

auduchinok commented 4 years ago

In this particular case Literal attribute isn't shadowed by anything else in the scope and it would be resolved even without open FSharp.Core, so I think it's an analyzer bug.

And FSharp.Core assembly is nothing that special here too. It just has several AutoOpen assembly-level attributes (which all F# assemblies are allowed to have), and one of these namespaces brings Literal into the scope implicitly.

TysonMN commented 4 years ago

Excellent explanation @auduchinok. I have confirmed this. Specifically...

module Top
[<AutoOpen>]
module M1 =
  let v1 = ()
module M2 =
  open M1  // should be marked as unused but is not
  let v2 = v1

Tested with: Operating system: Windows 10 .NET Runtime kind: .NET Core 3.1 Editing Tools: Visual Studio Enterprise 2019 version 16.7.1

I will review the existing tests and report back.

TysonMN commented 4 years ago

The following test is nearly the same and asserts that no open is unused. However, if open M.N (on line 699) is removed, then the code still compiles and means the same thing.

https://github.com/dotnet/fsharp/blob/50e7f16806975c55c7a9ff65fcde410a8b427308/vsintegration/tests/UnitTests/UnusedOpensTests.fs#L692-L704

abelbraaksma commented 4 years ago

I agree to the points where it isn't related to FSharp.Core, as since that's treated specially, it'll be difficult to analyze, and requires to flow through all other opens to check whether something that's in scope was shadowed by any of the modules and types that are the transitive closure of FSharp.Core.

For AutoOpen on anything else than the defaults, this makes more sense, and analysis will be simpler by just excluding the in scope types if they belong to an AutoOpen.

(btw, I believe a similar bug exists with type aliases, where an open is marked as unused, where it actually is used).