Fix null encoding in new document in move type action#74623
Fix null encoding in new document in move type action#74623CyrusNajmabadi merged 9 commits intodotnet:mainfrom
Conversation
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
seems reasonable. @tmat @jasonmalinowski any concerns?
|
@tmat ccan you ptal? |
src/Workspaces/Core/Portable/Workspace/Solution/DocumentState.cs
Outdated
Show resolved
Hide resolved
|
@tmat could you please review this? |
|
@tmat another reminder to review this |
|
|
|
@tmat ptal at the changed tests and review |
|
@tmat ready for review |
1 similar comment
|
@tmat ready for review |
|
Set to auto-merge. If we don't want this, please speak up @tmat |
Head branch was pushed to by a user without write access
|
@jasonmalinowski ptal. |
| encoding = null; | ||
| } | ||
| // use the encoding that we get from the new root | ||
| var encoding = newRoot.SyntaxTree.Encoding; |
There was a problem hiding this comment.
@jasonmalinowski I don't have context on the previous logic. Should it ask be removed (like the current pr does it), it should we just fall back to getting the encoding from the tree if the other checks fail?
There was a problem hiding this comment.
Note the previous comment from tmat: #74623 (comment)
There was a problem hiding this comment.
Ok. Seems reasonable. Merging in. If you have any concerns @jason let us know
Fixes dotnet/sdk#46780 This issue was introduced by #74623, which changed `Document.WithSyntaxRoot` to always use the encoding specified by the new syntax root. APIs like SyntaxRewriter do not preserve the input encoding during node rewrites, so `Formatter.Format` would overwrite the document syntax root with a node that did not specify any encoding. The fix _allows_ `WithSyntaxRoot` to specify a new encoding, but for cases where the encoding of the new syntax root is not known, we fall back to the original behavior of preserving the encoding from the previous root and/or document text.
Closes #72984
Summary
We default to receiving the encoding set in the new node we are requested to create the new document with in
DocumentState.A new test was added to test how hot reload reacts by applying the code fix that contained the null encoding bug, and applying actual code changes after performing the move type. It seems that the bug was not being triggered if the code fix had no changes to apply, i.e. the code remained the exact same, and all that changed was the location of the moved type.