Skip to content

Reject _ as a rename target for a binding that is used - #20599

Open
xperiandri wants to merge 11 commits into
dotnet:mainfrom
xperiandri:fix/rename-underscore-with-uses
Open

Reject _ as a rename target for a binding that is used#20599
xperiandri wants to merge 11 commits into
dotnet:mainfrom
xperiandri:fix/rename-underscore-with-uses

Conversation

@xperiandri

@xperiandri xperiandri commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Fixes #20597

Stacked on #20598

_ is not a referenceable name in F# — in expression position _.M() is the shorthand lambda — so renaming a self identifier (or any local) that its body uses produced a file that no longer compiles. Rename now accepts _ only when nothing but the declaration would be rewritten.

Rename also refuses outright when the symbol's declaration sits in the file being renamed but is missing from the locations to rewrite, rather than rewriting the uses and leaving the declaration behind.

🤖 Generated with Claude Code

@xperiandri
xperiandri requested a review from a team as a code owner September 20, 2026 20:45
@github-actions

github-actions Bot commented Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

✅ Release notes checked

✅ Found changes and release notes in following paths:

Warning

No PR link found in some release notes, please consider adding it.

Change path Release notes path Description
`src/Compiler` docs/release-notes/.FSharp.Compiler.Service/11.0.200.md No current pull request URL (#20599) found, please consider adding it
`vsintegration/src` docs/release-notes/.VisualStudio/18.vNext.md

@github-actions github-actions Bot added the ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager label Sep 20, 2026
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions github-actions Bot added ⚠️ Affects-Build-Infra Tooling check: PR touches build infrastructure ⚠️ Affects-Restore Tooling check: PR touches NuGet packages or feeds labels Sep 20, 2026
T-Gro
T-Gro reviewed Sep 21, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🕵️ AI review — verify independently.

Comment thread vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs Resolved
Comment thread vsintegration/src/FSharp.Editor/InlineRename/InlineRenameService.fs Resolved
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Sep 21, 2026
@T-Gro
T-Gro self-requested a review September 21, 2026 09:31
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

github-actions Bot commented Sep 29, 2026

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Build-Infra, Affects-Design-Time, Affects-Restore
Affects-Build-Infra: Changes project build configuration.
Affects-Design-Time: Changes IDE project behavior.
Affects-Restore: Changes package references.

Generated by PR Tooling Safety Check · gpt56 2.5M · ◷

T-Gro
T-Gro reviewed Oct 1, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🕵️ Re-reviewed all recent changes as of 595873d; LGTM

@xperiandri
xperiandri force-pushed the fix/rename-underscore-with-uses branch from 595873d to 142b96c Compare October 2, 2026 16:03
T-Gro
T-Gro reviewed Oct 7, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🕵️ Re-reviewed all recent changes as of 142b96c; LGTM

@xperiandri
xperiandri force-pushed the fix/rename-underscore-with-uses branch from 142b96c to e1c69ea Compare October 8, 2026 01:11
T-Gro
T-Gro reviewed Oct 9, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🕵️ Re-reviewed all recent changes as of e1c69ea; LGTM

@xperiandri
xperiandri force-pushed the fix/rename-underscore-with-uses branch from e1c69ea to 9ee4e3c Compare October 10, 2026 22:28
T-Gro
T-Gro reviewed Oct 12, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🕵️ Re-reviewed all recent changes as of a9587eb; LGTM

T-Gro
T-Gro requested changes Oct 14, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff is still way too big - apply the NoBloat instructions and CodeCompaction skill.

Especially the massive code comments would benefit not just with drastic reduction, but ideally with well named functions/arguments/bindings/types making the intention clear.

If a well named piece of code just tells what the comment says - delete the comment.

  • Rename the test helper to renameUnderCaret and delete its XML walkthrough.
  • Delete the 9 non-license implementation/test comment lines and use predicates such as isUnescapedWildcardRenameOfUsedSymbol, hasDeclarationCoveringTriggerSpan, and isUserDeclaredThisVal.
  • Merge the single-case Fact into the rename theory and make the declaration/use caret cases named theory rows.
  • Compact both release-note entries to observable Rename behavior. Remove auto-property desugaring, synthetic-range, and _.M() mechanism prose.

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Oct 14, 2026
@xperiandri
xperiandri requested a review from T-Gro October 29, 2026 19:51
@xperiandri

xperiandri commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Compacted as requested in d015c35. It includes the compaction of #20598 (32b7ec0), on top of current main (fbc9be3aa8).

  • All six comment lines of this PR are gone and the PR adds none (the license header of the new test file is the only comment line). Their content moved into names: isUnescapedWildcardRenameOfUsedSymbol for the bare _ check, and hasDeclarationAmongUses for the guard that refuses to start a rename that would leave the declaration behind. I did not use hasDeclarationCoveringTriggerSpan: the guard never looks at the trigger span, and it has to hold for a rename started at a use.
  • The test helper is renameUnderCaret and its walkthrough is deleted.
  • The single-case Fact is a row of the self identifier theory, together with the two __ cases. The declaration/use caret cases of both theories are labelled rows ([used, renamed to _], [__, caret on a use], [caret on the declaration], ...).
  • The test file is a root module and calls FindRenameLocationsAsync(false, false, ...) like InlineRenameTests.
  • The Visual Studio release note states the behaviour only: Rename no longer accepts _ for a used binding and no longer starts when it would leave the declaration unrenamed.

The diff is +122/-12 (it was +164/-8). InlineRenameServiceTests (8 rows) and the existing InlineRenameTests pass locally, 47 of 47 (net472, Arcade Debug build with 0 warnings).

xperiandri added a commit to xperiandri/fsharp that referenced this pull request Oct 29, 2026
@xperiandri @claude
Match the release notes to the PR heads
c1b9d19 
The compacted notes of dotnet#20598, dotnet#20599 and dotnet#20637 replace the lines they rewrote; the union merge had kept both.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
T-Gro
T-Gro reviewed Nov 2, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 🕵️ Re-reviewed all recent changes as of d015c35; LGTM

xperiandri and others added 11 commits November 5, 2026 18:53
@xperiandri
Report a hand-written __ self identifier to the name resolution sink
2326dc5 
`MakeAndPublishVal` skipped the sink for any `MemberThisVal` literally named
`__`. The target was the self identifier that the auto-property desugaring
synthesizes, which sits on the *property name's* range and would therefore
shadow the property's own symbol -- a range collision, not a name problem.

Filtering by name also hid every `member __.M()` a user writes. Declarations go
through `MakeAndPublishVal` and were suppressed, while uses in the body resolve
normally and were not, so FCS reported the uses of `__` without its declaration.
Editor rename then rewrote the body and left `member __.` behind, silently
producing code that no longer compiles.

Mark the synthesized ident's range synthetic instead, as the adjacent backing
field already does. `TcResultsSinkImpl.allowedRange` drops synthetic ranges for
every sink notification, so the auto-property self identifier stays invisible
and the name check becomes dead code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xperiandri @claude
Add release note
3e551d9 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri
Name why the base val is not reported and compact the self identifier…
938632e 
… tests

The two comment lines about the range of `base` become the name `shadowsCtorParams`,
which also makes the `MemberInfo` match redundant: only `MakeAndPublishBaseVal` builds a base val and it has none.
The tests reuse `chooseMemberOrFunctionOrValue`, `getRangeCoords` and `shouldEqual`,
and the release note states the observable fix only.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri
Compare the self identifier ranges with collection asserts
ed5b6ed 
The ranges are an array now, and the tests use Assert.Equal and Assert.Empty
instead of FSharp.Test.Assert.shouldEqual.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri @claude
Refuse _ as a rename target for a binding that has uses
28f23a1 
`_` is not a referenceable name in F#: in expression position `_.M()` is the
shorthand lambda, not a reference. Renaming a self identifier (or any local)
that the body actually uses therefore produced a file that no longer compiles.

Accept `_` only when the rename rewrites nothing but the declaration.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xperiandri @claude
Refuse a rename whose set of locations omits the declaration
21df079 
FCS reports both the declaration and the uses of a healthy symbol, so a
declaration that sits in the file being renamed yet is absent from that
file's uses means the rename would rewrite the uses and leave the
declaration behind -- the failure mode `member __.M()` just exhibited.

Refuse to start the rename instead of corrupting the file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xperiandri @claude
Add inline rename tests for self identifiers
bae8e36 
Nothing covered InlineRenameService before.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xperiandri @claude
Point the __ use-site caret at the use
16e4fd1 
`__.TestMethod()` also matched the declaration one line above, whose own
self identifier has no uses, so the test placed the caret there and found
one location instead of two.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@xperiandri @claude
Accept an escaped _ and rename active pattern cases
d31ebc9 
Two review findings on the rename guards.

The `_` rule read the replacement after normalization, which strips the
backticks of an escaped ``_``. That name is referenceable, so it must stay
valid; the rule now looks at the text as typed.

The declaration guard compared the symbol's declaration with the ranges of its
uses for equality. Seen from a use, the declaration of an active pattern case is
the whole `(|A|B|)`, while the definition use is the case itself, so equality
never held and renaming `Even` was refused. The guard now asks whether the
declaration contains one of the uses; on a file with 166 symbol uses this blocks
none, where equality blocked the five active pattern cases.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@xperiandri @claude
Name the rename guards instead of commenting them and merge the renam…
738caba 
…e service tests

`isUnescapedWildcardRenameOfUsedSymbol` and `hasDeclarationAmongUses` carry what the six comment lines said.
The self identifier tests become one theory with labelled rows, the active pattern theory gets labelled rows,
and the test file is a root module that calls `FindRenameLocationsAsync(false, false, ...)` like `InlineRenameTests`.
The release note states the observable behaviour only.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri @claude
Add release note
cba9f41 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@xperiandri
xperiandri force-pushed the fix/rename-underscore-with-uses branch from d015c35 to cba9f41 Compare November 5, 2026 18:17

This branch has not been deployed

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

Reviewers

@T-Gro T-Gro T-Gro requested changes
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

⚠️ Affects-Build-Infra Tooling check: PR touches build infrastructure ⚠️ Affects-Design-Time Tooling check: PR touches type providers or dependency manager ⚠️ Affects-Restore Tooling check: PR touches NuGet packages or feeds AI-reviewed PR reviewed by AI review council

Projects

F# Compiler and Tooling
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rename accepts _ as a new name for a value that is used

3 participants

@xperiandri @T-Gro