fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Misc. unused opens analyzer fixes

Open brianrourkeboll opened this issue 8 months ago • 4 comments

Description

  • Fix https://github.com/dotnet/fsharp/issues/17629.
  • Fix https://github.com/dotnet/fsharp/issues/17929 (and probably fixes https://github.com/dotnet/fsharp/issues/18389?).
  • Fix https://github.com/dotnet/fsharp/issues/16226.

Checklist

  • [x] Test cases added.
  • [x] Release notes entry updated.

brianrourkeboll avatar Apr 26 '25 19:04 brianrourkeboll

:heavy_exclamation_mark: Release notes required

@brianrourkeboll,

[!CAUTION] No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/10.0.100.md No release notes found or release notes format is not correct

github-actions[bot] avatar Apr 26 '25 19:04 github-actions[bot]

(getting a fresh build to see what the test failure was about, the previous results have expired)

T-Gro avatar May 19 '25 13:05 T-Gro

(getting a fresh build to see what the test failure was about, the previous results have expired)

Sorry, I should have left a comment. Those test failures were legit. The reverse-processing trick that was meant to fix https://github.com/dotnet/fsharp/issues/16226 breaks other scenarios.

I think the real solution would require keeping some kind of open → shadow → reopen tracking to detect potential intervening shadowings that make a subsequent reopen not actually redundant.

Some of the fixes in this PR do not require that, though, so I could keep those and leave the open → shadow → reopen tracking for another PR.

brianrourkeboll avatar May 19 '25 13:05 brianrourkeboll

(getting a fresh build to see what the test failure was about, the previous results have expired)

Sorry, I should have left a comment. Those test failures were legit. The reverse-processing trick that was meant to fix #16226 breaks other scenarios.

I think the real solution would require keeping some kind of open → shadow → reopen tracking to detect potential intervening shadowings that make a subsequent reopen not actually redundant.

Some of the fixes in this PR do not require that, though, so I could keep those and leave the open → shadow → reopen tracking for another PR.

Got it, we would exchange a fixed bug for opening another one when it comes to shadowings. That's not good indeed.

The analyzer could maybe detect that after a suggested removal, the resolution from names to symbols has changed - but that would increase CPU intensity of the analysis.

T-Gro avatar May 20 '25 10:05 T-Gro