Skip to content
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

Fix the ArgumentOutOfRangeException in inline rename #69708

Merged
merged 6 commits into from
Aug 31, 2023

Conversation

Cosifne
Copy link
Member

@Cosifne Cosifne commented Aug 24, 2023

Fix two places that might cause ArgumentOutOfRange exception in inline rename during my testing.

@Cosifne Cosifne requested review from a team as code owners August 24, 2023 22:03
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 24, 2023
@@ -55,7 +55,7 @@ public string IdentifierText
{
if (value != _session.ReplacementText)
{
_session.ApplyReplacementText(value, propagateEditImmediately: true);
Copy link
Member Author

Choose a reason for hiding this comment

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

The second place causes ArugmentOutofRangeException.
When the identifier in the text box gets changed, it would update the text selection in the editor.

This might cause an Exception because text selection might also get changed when the replacement text changes.
(Like, if you type F when rename Field123, the replacement text is F, and text selection becomes zero length)
We are still using a out-off date state https://sourceroslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/InlineRename/AbstractInlineRenameUndoManager.cs,143 to update the selection in the editor.

Copy link
Member Author

@Cosifne Cosifne Aug 24, 2023

Choose a reason for hiding this comment

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

This is triggered by the binding from the UI. And we only know replacement text gets changed
IMO we should not try to update the selection at this point. Later it would be updated by another event handler of the control

private void IdentifierTextBox_SelectionChanged(object sender, RoutedEventArgs e)

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely agree with this. It's actually kind of odd that selection was being changed by the old style. There's probably some cases I can't think of where that's required.

@@ -262,8 +262,8 @@ private bool AreAllReferenceSpansMappable()
{
// in tests `ActiveTextview` could be null so don't depend on it
return ActiveTextView == null ||
_referenceSpanToLinkedRenameSpanMap.Keys
Copy link
Member Author

Choose a reason for hiding this comment

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

The story:
This is causing #68880.
Given a code

class Bar 
{
    // Some code
    public int Field123;
}

Field123 has length. Suppose the total length of the document is 100.
Field123 would be [91, 98]. When type F after invoking the new inline rename.
It would

  1. Delete all the text of Field123.
  2. Insert F

After step 1, the document shrinks, its length becomes 92. Here _referenceSpanToLinkedRenameSpanMap.Keys stores the original text span in the document, so AreAllReferenceSpansMappable would return false, because s.End <= _subjectBuffer.CurrentSnapshot.Length is not true. (s.end is 98, the total length of the document is 92)

This doesn't cause problems in the old rename UI. Because in that case we directly edit the text in the editor. But in the new UI, all the text edit is propagated from the text check box to the editor. In any case, if this text edit is filter out, it would cause the out-off sync between the editor and inline rename check box, resulting exception.

Copy link
Member Author

@Cosifne Cosifne Aug 28, 2023

Choose a reason for hiding this comment

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

This method originates from #9208 and it feels like the old workaround is causing problems in the new UI. (I have verified it's still a problem today)
From my understanding, it means in the legacy razor editor, text in @model might not be in the view yet.
If we use the tracking span instead here. At start point, the tracking span should be equal to the span here. (key and value of the dictionary should be pointed to the same thing) So the wrong case would be filtered. Since razor document doesn't get changed in rename session, the tracking span would stay the same all the time. So we don't regress the old behavior.

In normal editor case, because tracking span grows/shrink with the text edit won't be filtered.

Copy link
Member Author

@Cosifne Cosifne Aug 30, 2023

Choose a reason for hiding this comment

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

Verified in legacy razor editor (use new inline rename UI):
LegacyRazorTest
Verified in new razor editor(use new inline rename UI):
NewRazorTest

Copy link
Member Author

Choose a reason for hiding this comment

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

Verified in new razor editor (Use old inline rename UI):
NewRazorTestOldUI

Verified in legacy razor editor (Use old inline
LegacyRazorTestOldUI
rename UI):

@Cosifne
Copy link
Member Author

Cosifne commented Aug 30, 2023

Also tag @jasonmalinowski if you interested in reviewing this

@Cosifne
Copy link
Member Author

Cosifne commented Aug 31, 2023

Alright, @jasonmalinowski I will check this in since it is blocking me from fixing another rename bug. If you have any worries we can sync in following PR.

@Cosifne Cosifne merged commit 1ef50ab into dotnet:main Aug 31, 2023
@Cosifne Cosifne deleted the dev/shech/inlineRenameFix branch August 31, 2023 17:50
@sharwell sharwell added this to the 17.8 P4 milestone Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants