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 null encoding in new document in move type action #74623

Merged

Conversation

Rekkonnect
Copy link
Contributor

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.

@Rekkonnect Rekkonnect requested a review from a team as a code owner July 31, 2024 20:25
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 31, 2024
@dotnet-policy-service dotnet-policy-service bot added Community The pull request was submitted by a contributor who is not a Microsoft employee. VSCode labels Jul 31, 2024
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

seems reasonable. @tmat @jasonmalinowski any concerns?

@CyrusNajmabadi
Copy link
Member

@tmat ccan you ptal?

@Rekkonnect Rekkonnect requested a review from tmat August 8, 2024 09:46
@Rekkonnect
Copy link
Contributor Author

@tmat could you please review this?

@Rekkonnect
Copy link
Contributor Author

@tmat another reminder to review this

@Rekkonnect
Copy link
Contributor Author

MSBuildWorkspacePreservesEncoding fails at Assert.NotNull(doc3text.Encoding); because of the last change where we always override the encoding of the syntax tree. This behavior must also be reflected in the test, so I'll modify that. We can also preserve the behavior of retaining the encoding if we already had an encoding and the new syntax tree has a null one.

@Rekkonnect
Copy link
Contributor Author

@tmat ptal at the changed tests and review

@Rekkonnect
Copy link
Contributor Author

@tmat ready for review

1 similar comment
@Rekkonnect
Copy link
Contributor Author

@tmat ready for review

@CyrusNajmabadi
Copy link
Member

Set to auto-merge. If we don't want this, please speak up @tmat

auto-merge was automatically disabled December 2, 2024 08:27

Head branch was pushed to by a user without write access

@CyrusNajmabadi
Copy link
Member

@jasonmalinowski ptal.

encoding = null;
}
// use the encoding that we get from the new root
var encoding = newRoot.SyntaxTree.Encoding;
Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

@Rekkonnect Rekkonnect Dec 2, 2024

Choose a reason for hiding this comment

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

Note the previous comment from tmat: #74623 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Seems reasonable. Merging in. If you have any concerns @jason let us know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hot Reload unusable with CS8055 if a file created via "Move type to X.cs" is not touched by the user
4 participants