dotnet / dotnet/fsharp

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

Open
#11,528 2 comments 0 reactions 0 assignees View on GitHub
Area-LangService-Colorization Bug Impact-Medium
Dominant language
F#
Stars
4.3k
Forks
876
Avg merge
4d 11h
Merged PRs (30d)
131

Description

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](https://github.com/dotnet/fsharp/pull/11521/files#diff-bff3f7337de1c9a878112e73f774cc564bcae3e1e4785c8b1beb2af6cb8f6de9R6149)

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](https://github.com/dotnet/fsharp/blob/dbf9a625d3188184ecb787a536ddb85a4ea7a587/src/fsharp/service/ServiceAnalysis.fs#L338) 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](https://github.com/dotnet/fsharp/blob/dbf9a625d3188184ecb787a536ddb85a4ea7a587/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs#L36) 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.