Skip to content

Misc. unused opens analyzer fixes #18510

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Apr 26, 2025

Copy link
Contributor

github-actions bot commented Apr 26, 2025

❗ 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

@brianrourkeboll brianrourkeboll force-pushed the unused-opens-analyzer-fixes branch from 1263077 to 23d3cbc Compare April 27, 2025 14:28
@T-Gro
Copy link
Member

T-Gro commented May 19, 2025

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

@brianrourkeboll
Copy link
Contributor Author

(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.

@T-Gro
Copy link
Member

T-Gro commented May 20, 2025

(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.

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