WIP - Editor - Rename/Findall bug fixes #19252
Draft
+997
−788
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes issues #14969 and #19173 where active pattern cases were not being found in signature files by Find All References. Root cause: When a value is published via MakeAndPublishVal, only Item.Value was registered with CallNameResolutionSink. For active patterns in implementation files, the individual cases are registered during TcLetBinding. However, signature files don't go through that path, so active pattern cases were missing. Fix: In MakeAndPublishVal, when processing a signature file (inSig=true), also check if the value is an active pattern using TryGetActivePatternInfo. If so, register each case with Item.ActivePatternResult at its range. Changes: - src/Compiler/Checking/Expressions/CheckExpressions.fs: Add active pattern case registration for signature files - tests/FSharp.Compiler.ComponentTests/FSharpChecker/FindReferences.fs: Update tests to expect correct behavior and add new test for case distinction in signature files
When renaming a property with explicit get/set accessors, the rename operation now correctly preserves the get and set keywords instead of replacing them with the new property name. Changes: - Added Tokenizer.tryFixupSpan to detect and filter out property accessor keywords (get/set) from rename operations - Updated InlineRenameService.FindRenameLocationsAsync to use tryFixupSpan and skip locations where it returns ValueNone - Added test documenting compiler service behavior for property references - Added VS layer test for property rename with get/set accessors The fix works by detecting when the span text after fixup is exactly 'get' or 'set' and excluding such spans from the rename locations.
The previous fix only applied tryFixupSpan filtering to InlineRenameService. This commit extends the fix to FindUsagesService.onSymbolFound so that 'Find All References' also filters out property accessor keywords (get/set). Changes: - FindUsagesService.onSymbolFound: Use Tokenizer.tryFixupSpan instead of fixupSpan to filter out property accessor keywords from references - Update test comment to accurately describe where filtering occurs
- Add 2 tests in SingleLineInterfaceSyntax module to verify Find All References works correctly for single-line interface syntax - Test interface member references (definition, implementation, usage) - Test interface type references (definition, implementation, cast) - All 42 FindReferences tests pass Note: The VS crash mentioned in #15399 was identified as a VS-side bug that also affects C# when renaming the last symbol in a file (not an F# issue).
When using #line directives (common in generated code like parser/lexer files), Find All References now correctly returns ranges at the remapped file/line positions from the directive, matching C# behavior. The fix applies range.ApplyLineDirectives() when returning ranges from ItemKeyStore.FindAll, converting original file positions to the positions specified by #line directives. Added test to verify references are found at correct remapped locations.
Fix SynPat.Or pattern binding classification (#5546): - In disjunctive 'or' patterns like '| x | x', the second occurrence of 'x' was incorrectly marked as ItemOccurrence.Binding. It should be marked as ItemOccurrence.Use since only the left-most path introduces the binding. - Changed CheckPatterns.fs to emit ItemOccurrence.Use for non-left-most paths. Filter synthetic event handler symbols (#4136): - Events with [<CLIEvent>] generate synthetic 'handler' values that were incorrectly appearing in GetAllUsesOfAllSymbolsInFile results. - Added filtering in IncrementalBuild.fs and TransparentCompiler.fs to skip items with synthetic ranges when building the ItemKeyStore. Tests: - Added test for SynPat.Or pattern to verify second binding is classified as Use - Added test for event handler to verify synthetic 'handler' symbols are filtered - All 45 FindReferences tests pass
- Add isFSharpSourceFile function to Pervasive.fs to check for F# file extensions - Filter Project.FindFSharpReferencesAsync to only process F# source files - Fixes crash when F# projects contain non-F# files (e.g., .cshtml) Issue #15721 (disposable type rename timing) was investigated and determined to be a VS IDE layer caching issue, not a compiler service bug.
…ers (#15290, #16621) ## Record copy-and-update (#15290) When a record copy-and-update expression like `{ m with V = "m" }` is processed, the record type is now registered as a reference. This ensures "Find All References" on the record type includes copy-and-update usages. Implementation: Added CallNameResolutionSink call in CheckExpressions.fs after BuildFieldMap determines the record type for copy-and-update expressions. ## DU case testers (#16621) When accessing a union case tester property like `.IsB` on a union type, a reference to the underlying union case (`B`) is now registered. This ensures "Find All References" on a union case includes usages via its tester. Implementation: Modified CallNameResolutionSink, CallMethodGroupNameResolutionSink, and CallNameResolutionSinkReplacing in NameResolution.fs to detect union case testers and register the corresponding union case reference with a slightly shifted range to avoid duplicate filtering. ## Tests Added 4 new tests in FindReferences.fs: - Record copy-and-update detection (2 tests) - DU case tester detection (2 tests) All 49 FindReferences tests pass.
Additional constructors can now be found via Find All References. When a constructor usage is resolved (e.g., MyClass()), we now also register it as Item.Value(vref) in addition to Item.CtorGroup. This allows symbol lookup from constructor definitions to find usages. Changes: - src/Compiler/Checking/Expressions/CheckExpressions.fs: Register Item.Value for F# constructor usages in ForNewConstructors - tests: Added tests for additional constructor references - tests: Updated symbol count baseline (79→80)
When searching for references to symbols from external DLLs (e.g., System.String), the code previously searched ALL projects in the solution. This commit optimizes the search by filtering to only projects that actually reference the specific DLL. Changes: - Added getProjectsReferencingAssembly helper function in SymbolHelpers.fs - Updated findSymbolUses to use assembly file path for filtering when scope is None - Added 2 new tests in ExternalDllOptimization module to verify the optimization The optimization checks the assembly file path of external symbols and matches it against project metadata references, significantly reducing unnecessary searches in large solutions with many projects that don't reference the specific DLL.
For C# extension methods, ItemKeyStore was using ApparentEnclosingType (the type being extended, e.g., Array) instead of DeclaringTyconRef (the static class declaring the extension, e.g., Enumerable). This caused inconsistent keys between usages of the same extension method, preventing Find All References from finding all uses. The fix uses IsILExtensionMethod to detect C# extension methods and writes the DeclaringTyconRef to the key instead of ApparentEnclosingType. Added 2 tests in CSharpExtensionMethods module: - Find references for same overload finds all usages - Extension method has correct symbol information
Added issue references (#NNNNN) to source code for traceability: - #19173, #14969: Active patterns in signature files (CheckExpressions.fs) - #18270: Property rename get/set keyword filtering (Tokenizer.fs) - #17221: Operator handling in fixupSpan (Tokenizer.fs) - #16394: Non-F# file filtering (Pervasive.fs) - #16621: Union case tester references (NameResolution.fs) - #10227: DLL optimization for Find All References (SymbolHelpers.fs) #15399 already has documentation in test file (FindReferences.fs). All 55 FindReferences tests pass.
- Add #15290 comment to CheckExpressions.fs for record copy-and-update fix - Add #14057 to Tokenizer.fs comment (alongside #17221 for operator fixes) - Update docs/TASKLIST.md to reflect 14 fixed issues All 14 fixed issues now have searchable source code comments: #19173, #14969, #18270, #17221, #14057, #16394, #9928, #5546, #4136, #16993, #15290, #16621, #14902, #10227 Open issues (4): - #15399: VS layer interface rename crash (test coverage added) - #15721: VS layer disposable timing issue - #5545: Not investigated (2018 SAFE bookstore bug, low impact) - #4760: Feature request (rename in strings, deferred)
…ixed The 2018 issue #5545 reported that DU types like Wishlist/Msg and Database/DatabaseType were not always found by Find All References. Investigation shows the issue has been fixed by other changes in this PR. Added 2 tests in SAFEBookstoreSymbols module to verify: - DU type definitions are found - Type annotations in parameters are found - Qualified usages (TypeName.Case) are found All 57 FindReferences tests pass (55 existing + 2 new).
Extract the union case tester registration logic that was duplicated 3 times in CallNameResolutionSink, CallMethodGroupNameResolutionSink, and CallNameResolutionSinkReplacing into a single private helper function. This reduces ~62 lines of duplicate code to ~39 lines of DRY code.
Updates all Find All References and Rename code paths to use tryFixupSpan instead of fixupSpan. The tryFixupSpan function filters out property accessor keywords (get/set) which should not be included in rename operations. Changes: - InlineRenameService.fs: Use tryFixupSpan in GetReferenceEditSpan and trigger span initialization - FindUsagesService.fs: Use tryFixupSpan in rangeToDocumentSpans - RenameParamToMatchSignature.fs: Use tryFixupSpan to filter keywords - ClassificationService.fs: Added comment documenting intentional use of fixupSpan for syntax coloring - Tokenizer.fs: Enhanced tryFixupSpan documentation with usage pattern Usage pattern: - tryFixupSpan: For Find All References and Rename (filters get/set) - fixupSpan: Only for semantic classification/syntax coloring
- Add 6 helper functions: findRefsInSource, testFindRefsInSource, expectRanges, expectLines, expectLinesInclude, expectMinRefs - Refactor 12+ tests to use new helpers - Reduce file from 1417 to 1168 lines (17.5% reduction, 249 lines saved) - Remove duplicate sortBy/map/Array.ofSeq assertion patterns - All 57 FindReferences tests pass
Documents 15 fixed issues: - #19173, #14969: Active patterns in signature files - #17221, #14057: Operators with . and operator-to-operator rename - #18270: Property get/set keyword rename - #16394: cshtml file crash - #9928: #line directive remapping - #5546, #5545: SynPat.Or and DU types in modules - #4136: Synthetic event handlers filtering - #16993: C# extension methods - #16621: DU case tester properties - #15290: Record copy-and-update expressions - #14902: Constructor references - #10227: DLL optimization
Updated test baselines in ProjectAnalysisTests and EditorTests to include the new constructor reference symbols. The #14902 fix registers F# constructor usages as Item.Value references, which adds additional symbol entries to GetAllUsesOfAllSymbols results.
Previously, registerUnionCaseTesterIfApplicable was called from 3 generic sink functions (CallNameResolutionSink, CallMethodGroupNameResolutionSink, CallNameResolutionSinkReplacing), pattern-matching Item.Property on EVERY name resolution (variables, methods, types, everything). Now: - Renamed to RegisterUnionCaseTesterForProperty (public, takes PropInfo list) - Called directly from 2 property-specific handlers in CheckExpressions.fs: 1. TcItemThen for static properties 2. TcLookupItemThen for instance properties - Removed from 3 generic sink functions Benefits: - Zero overhead for non-property name resolutions - Logic colocated with property-specific handling - Clearer architectural layering (property logic in property handlers) - No redundant pattern matching on every sink call This addresses the architectural inefficiency identified in code review where union case tester logic was scattered across generic sinks instead of being targeted at property-specific code paths.
Contributor
❗ 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
|
Fixed invalid F# syntax where active patterns were incorrectly used inside ValueSome(...) pattern matches. Active patterns must be applied directly to the matched value, not inside discriminated union case patterns. Files fixed: - RoslynHelpers.fs: TryFSharpRangeToTextSpanForEditor - FindUsagesService.fs: reportReferenceIfNotDeclaration - InlineRenameService.fs: GetRenameInfoAsync Also ran dotnet fantomas for formatting compliance.
RoslynHelpers.fs is compiled before Tokenizer.fs in the project file, so the helper function that uses Tokenizer.FixedSpan active pattern cannot be defined in RoslynHelpers. Moved TryFSharpRangeToTextSpanForEditor to Tokenizer.fs where it can reference the FixedSpan active pattern directly without qualification. Updated callers to use Tokenizer.TryFSharpRangeToTextSpanForEditor.
- SymbolHelpers.fs: Add missing cases for CurrentDocument and SignatureAndImplementation - FindUsagesService.fs: Change unused 'textSpan' binding to wildcard '_'
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.