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

Convert UTF-8 string literal analyzer/codefix to refactoring #61593

Closed

Conversation

davidwengier
Copy link
Contributor

Due to a language spec change, where UTF-8 strings are ReadOnlySpan<byte> rather then byte[], it no longer makes sense for there to be a code style analyzer to offer UTF-8 string literals, so this PR removes the old analyzer and code fix (added in #60647) and instead creates a refactoring that converts new byte[] { 65, 66, 67 } to "ABC"u8.ToArray(). The analyzer used to offer to convert parameter arrays too, but this is removed as it seemed a bit weird. Can always add it back later if we get feedback.

Fixes #61517

The PR looks a lot bigger than it is, most of the work was just moving code around, and simplifying.

@davidwengier davidwengier requested a review from a team as a code owner May 31, 2022 06:27
Comment on lines +43 to +44
var node = (SyntaxNode?)await context.TryGetRelevantNodeAsync<ArrayCreationExpressionSyntax>().ConfigureAwait(false) ??
await context.TryGetRelevantNodeAsync<ImplicitArrayCreationExpressionSyntax>().ConfigureAwait(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new. Is it better to not use the helpers? Or use the helpers to get ExpressionSyntax and then manually compare to these two array expression types?

return;

var model = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
if (model.Compilation.GetTypeByMetadataName(typeof(ReadOnlySpan<>).FullName!) is null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new because it wasn't relevant before the language change

Comment on lines +185 to +189
return SyntaxFactory.InvocationExpression(
SyntaxFactory.MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
SyntaxFactory.LiteralExpression(SyntaxKind.UTF8StringLiteralExpression, literal),
SyntaxFactory.IdentifierName(nameof(ReadOnlySpan<byte>.ToArray))));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new, to add the ToArray call

var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var editor = new SyntaxEditor(root, document.Project.Solution.Workspace.Services);

editor.ReplaceNode(node, CreateUTF8StringLiteralToArrayInvocation(stringValue).WithTriviaFrom(node));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't really new, but removing the parameter array stuff simplified this bit, and removed 2 or 3 helper methods, and made trivia handling much simpler

@CyrusNajmabadi
Copy link
Member

Will look later today. Did you implement 'fix all' support here? Thanks!

@davidwengier
Copy link
Contributor Author

Did you implement 'fix all' support here?

No, I assumed that was being done case-by-case basis for selected refactorings only.

@CyrusNajmabadi
Copy link
Member

No, I assumed that was being done case-by-case basis for selected refactorings only.

You are correct :) But it seems here to be useful. I could see a file with dozens of byte[]s that a user wants to bulk convert over :)

Fix-All should be fairly simple to add. Would you be willing to do it here?

@davidwengier
Copy link
Contributor Author

Superseded by #61650

@davidwengier davidwengier deleted the UTF8StringLiteralRefactoring branch June 2, 2022 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A change for natural type of a UTF-8 string literal to ReadOnlySpan<byte> breaks several IDE tests
2 participants