Suppress unused-opens/declarations false positives when file has type errors#19854
Suppress unused-opens/declarations false positives when file has type errors#19854T-Gro wants to merge 3 commits into
Conversation
… errors (#16415) These tests assert that UnusedOpens.getUnusedOpens returns no ranges when the source contains any Error-severity diagnostic. They were intended to fail on main and pass after the guard is added in ServiceAnalysis.fs. Actual results on current main (all 11 tests): passed: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11 failed: (none) All 11 tests currently pass; symbol uses appear to be recorded despite type errors in the snippets used here. The tests remain as regression coverage for the analysis behavior on files with errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… errors (#16415) When a file contains any Error-severity diagnostic, symbol-use collection in the type checker is incomplete because the checker aborts early on errored expressions. Running the unused-opens or unused-declarations analysis over that incomplete data produces false positives - opens and declarations are flagged as unused when their uses simply weren't recorded. Guard both UnusedOpens.getUnusedOpens and UnusedDeclarations.getUnusedDeclarations in ServiceAnalysis.fs to return empty when the check results contain any Error diagnostic. Warnings are still allowed - they don't impact symbol-use recording. One pre-existing test (`unused inner module open declaration in rec module`) had a snippet with a real `'open' must come first in rec` error and was relying on the unused-opens range being reported despite that error. Updated to expect no ranges under the new conservative behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ard regression (#16415) Addresses TESTS verifier coverage gaps for the Error-diagnostic guard added in ServiceAnalysis.fs: 1. Adds two tests for UnusedDeclarations.getUnusedDeclarations - one verifying the guard returns an empty seq<range> when the file has any Error diagnostic, and one verifying genuinely unused private declarations are still reported on clean files. 2. Adds a UnusedOpens test using the rec-module parse-error pattern. Without the guard the snippet produced [10, (5, 11)] (open flagged as unused because the 'open must come first in rec' parse error left symbol uses incomplete); with the guard the file's Error diagnostic suppresses the analysis and the range list is empty. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
❗ Release notes requiredCaution 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:
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
|
github-actions
Bot
added
the
AI-Tooling-Check-Bypassed
|
@T-Gro Please make this behavior configurable. It's arguably worse experience to disable the unused open feature completely when there are any errors in the file, so an FCS client should be able to keep the existing behavior. Speaking of #16415, I think the problem is not the error itself but the fact that the type checker doesn't continue analysis after it, resulting in the unused open. Disabling unused opens analyzers only partially covers the issue, the real fix is to add more recovery in the type checker. |
|
I am not sure if we can add a full recovery after any errors without risk a swamp of more diagnostics coming through. Basically I am not convinced we could guarantee that the analyzer works correctly. |
I agree that there are going to be unrecoverable errors. It's a trade-off in any case whether enabling or disabling the analyzer. I'd like to not change the behavior in Rider at this time. (I'd rather continue the recovery work 🙂) |
2 participants
Fixes #16415
Summary
When a file contains any Error-severity diagnostic, the type checker aborts early on errored expressions and symbol-use collection is incomplete. Running unused-opens or unused-declarations analysis on that incomplete data produces false positives — opens and declarations get flagged as unused when their uses simply weren't recorded.
Changes
Guard in ServiceAnalysis.fs:
Warnings are still allowed — they don't impact symbol-use recording.
Tests: