Accessibility computations in tooling are not taking into account signature hiding, leading to glitches

[dsyme]

We have various bugs like this

• "Renaming something hidden by a signature unnecessarily applies renaming across the entire project or solution"
• "A declaration hidden by a signature that is unused in an implementation is not reported as unused in colorization"
• "The IsPublic property on FSharpEntity in the FCS API of a symbol drawn from the implementation file doesn't take into account whether the declaration is hidden by a signature, nor does it take into account the accessibility in the signature."

In more detail, @TIHan and I were doing some code review related to #11521 related to this line

The Accessibility property on Val and Entity does not complete information unfortunately and most code relying on it alone is probably buggy. To be accurate it should be renamed to CombinedAccessiblityIgnoringHidingOrAccessibilitiesInSignatureFile and not used except in one other helper which combines this information signature hiding information.

This matters when a signature takes a thing which appears "public" (when looking at the implementation file alone) and either constrains it to be internal or hides it altogether (the latter causes an emit of an "internal"/"assembly" accessibility in the actual IL). For example consider signature:

val internal x: int

for implementation

let x = 1

Here x is "public" given the implementation file alone (because in F# declarations default to public), but the final computed accessiblity is internal. For better or worse for historical reasons we don't apply this computation during type checking (we likely could) but rather apply it late in IlxGen.fs.

For example in IlxGen.fs we combine this thing with IsHiddenVal to give the final actual IL accessibility https://github.com/dotnet/fsharp/blob/main/src/fsharp/IlxGen.fs#L5593. Similarly we use IsHiddenVal in PostInferenceChecks.fs. We should have another single overall helper for this kind of thing giving an actual final F# accessibility.

Does this matter? The answer is yes

1. Scanning through uses of Val/Entity Accessiblity we see it is used to compute IsPublic, IsPrivate etc on symbols. This means these are in turn actually inaccurate and likely to lead to bugs, e.g. should be called IsPublicIgnoringHidingOrAccessibilitiesInSignatureFile. This is further consumed to give us dodgy versions of IsPrivateToFile and IsInternalToSolution which again should really be renamed or fixed for accuracy.

2. This matters in places like this where we will fail to detect that something hidden by a signature is actually unused across a file. This would actually be a really useful fix.

3. Another example where this is causing sub-optimal feature implementation is here where IsPrivateToFile will not be true if something is hidden by the signature. This will mean SymbolDeclarationLocation.CurrentDocument is not returned (note SymbolDeclarationLocation would be better named SymbolDeclarationScope), which means the rename of something hidden by a signature will unnecessarily apply across the entire project or solution. This again would be a great thing to fix.

[smoothdeveloper]

This is a wealth of information to dig around and understand better what lays under FSharpEntity metadata.

Regarding aspects of visibility being implemented in a "hackish" way (not conducive to add protected any time soon :D), I faced and fixed this one #8354 (mixed visibility on property setter called as extra argument in method invocation syntax), which maybe something to review / revisit / refine if there are refactorings to better the design of this "visbility" concern.

Aside, with all the potential "against signature" optimization that is coming into FCS, this "PLTalk" ¹ discusses how the Flow type checker works ².

At minute 38 the usage of signatures to enable great optimization (up to parallel type checking) had me a bit excited (if we could "transpose" these optimizations), seeing that work is put in generating signatures, and fixing the underlying information the compiler assembles during type checking is encouraging!

¹: twitch hosted by @jeanqasaur & @hongyihu
²: detailed discussion by two contributors, @mvitousek and @jbrown215

[cartermp]

@smoothdeveloper You can read more here about the impact of parallel typechecking with explicit signatures to get a sense for optimizations like that: #11152

It's good, but not on the order of 2x better times or anything.

I consider this to be largely of the topic of the issue though. Probably better to look at the PR linked and see if there's more to add/consider there.