Grab bag of UTF8 string support in IDE features by davidwengier · Pull Request #60599 · dotnet/roslyn

davidwengier · 2022-04-06T04:32:25Z

Fixes most of #60582
Relates to #58848

Also fixed a couple of raw string things that were missed. If the review is too annoying, each commit is a separate IDE feature.

Youssef1313 · 2022-04-06T07:17:40Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/SyntaxTreeExtensions.cs

+                             SyntaxKind.SingleLineRawStringLiteralToken,
+                             SyntaxKind.MultiLineRawStringLiteralToken,


I see these already existed before your change. But I'm confused with it when reading this comment:

roslyn/src/Analyzers/CSharp/CodeFixes/ConvertNamespace/ConvertNamespaceTransform.cs

Lines 126 to 129 in c8ebc86

// if this line is inside a string-literal or interpolated-text-content, then we definitely do not want to

// touch what is inside there. Note: this will not apply to raw-string literals, which can potentially be

// dedented safely depending on the position of their close terminator.

if (tree.IsEntirelyWithinStringLiteral(textLine.Span.Start, cancellationToken))

Yes, interesting disconnect there. The comment isn't wrong, in that raw strings can be potentially dedented, but definitely outside the scope of this PR.

Youssef1313 · 2022-04-06T07:18:34Z

src/VisualStudio/CSharp/Impl/LanguageService/CSharpHelpContextService.cs

+            if (token.IsKind(SyntaxKind.UTF8StringLiteralToken) ||
+                token.IsKind(SyntaxKind.UTF8SingleLineRawStringLiteralToken) ||
+                token.IsKind(SyntaxKind.UTF8MultiLineRawStringLiteralToken))
+            {
+                text = Keyword("UTF8StringLiteral");
+                return true;
+            }
+
+            if (token.IsKind(SyntaxKind.SingleLineRawStringLiteralToken) ||
+                token.IsKind(SyntaxKind.MultiLineRawStringLiteralToken))
+            {
+                text = Keyword("RawStringLiteral");
+                return true;
+            }


@BillWagner FYI to use those f1 keywords when documenting the features

Currently F1ing on a UTF8 string goes to a "No help content found" page, so its not a big deal IMO if these keywords are specified before docs pages exist/are updated.

JoeRobich · 2022-04-06T19:34:05Z

Retargeting to main so we can remove main-vs-deps branch

src/EditorFeatures/CSharp/BraceMatching/StringLiteralBraceMatcher.cs

src/Features/CSharp/Portable/SplitStringLiteral/SimpleStringSplitter.cs

davidwengier · 2022-04-07T03:32:56Z

/azp run roslyn-integration-corehost

azure-pipelines · 2022-04-07T03:33:06Z

Azure Pipelines successfully started running 1 pipeline(s).

davidwengier · 2022-04-12T00:23:08Z

ping @dotnet/roslyn-ide for review

ryzngard

LGTM. Can you add tests to MoveToFile as well to make sure they get handled correctly? (I didn't see any in source prior to this change)

src/EditorFeatures/CSharp/TextStructureNavigation/CSharpTextStructureNavigatorProvider.cs

src/EditorFeatures/CSharpTest/SplitStringLiteral/SplitStringLiteralCommandHandlerTests.cs

davidwengier · 2022-04-13T06:25:44Z

Can you add tests to MoveToFile as well to make sure they get handled correctly?

Will do, but I'm curious, did you have something specific in mind for this? It would surprise me if MoveToFile was sensitive to new syntax nodes that aren't new member declarations.

davidwengier · 2022-04-13T07:12:35Z

Actually.. I'm struggling to work out where a test for MoveToFile would make sense. Just to be clear do you mean MoveTypeTests? There don't seem to be tests for other string literals or even any other specific syntax nodes, so I'm not sure what you're looking to validate.

…res/required-members * upstream/main: (66 commits) Fix #55183: Add SymbolVisitor<TArgument, TResult> (#56530) Simplifier options (#60174) Remove duplicated asset Do not try to refcount solution syncing when communicating with OOP Delay symbol-search index updating until solution is fully loaded. add more miscellaneous tests for checked operators (#60727) Support checked operators in explicit interface implementation (#60715) Avoid formatting diagnostics with raw strings (#60655) Make heading levels for warning waves documentation consistent (#60721) Clean up IDiagnosticService extension methods Remove #nullable enable Add integration test to flag MEF composition breaks Generate static abstract interface members correctly (#60618) Merge release/dev17.2 to main (#60682) Fix FAR on checked operators (#60698) Add implement interface support for checked operators and cast operators (#60719) Update csc.dll path in launch.json (#60663) Grab bag of UTF8 string support in IDE features (#60599) Allow code actions to retrieve options for any language (#60697) Fix flaky VSTypeScriptHandlerTests (#60706) ...

…o setsrequiredmembers * upstream/features/required-members: (66 commits) Fix dotnet#55183: Add SymbolVisitor<TArgument, TResult> (dotnet#56530) Simplifier options (dotnet#60174) Remove duplicated asset Do not try to refcount solution syncing when communicating with OOP Delay symbol-search index updating until solution is fully loaded. add more miscellaneous tests for checked operators (dotnet#60727) Support checked operators in explicit interface implementation (dotnet#60715) Avoid formatting diagnostics with raw strings (dotnet#60655) Make heading levels for warning waves documentation consistent (dotnet#60721) Clean up IDiagnosticService extension methods Remove #nullable enable Add integration test to flag MEF composition breaks Generate static abstract interface members correctly (dotnet#60618) Merge release/dev17.2 to main (dotnet#60682) Fix FAR on checked operators (dotnet#60698) Add implement interface support for checked operators and cast operators (dotnet#60719) Update csc.dll path in launch.json (dotnet#60663) Grab bag of UTF8 string support in IDE features (dotnet#60599) Allow code actions to retrieve options for any language (dotnet#60697) Fix flaky VSTypeScriptHandlerTests (dotnet#60706) ...

davidwengier added 5 commits April 6, 2022 14:30

String splitting for UTF8 strings

6db75e0

Brace matching for UTF8 strings

fae93f0

Structure navigation for UTF8 strings

da1965b

IsEntirelyWithinStringLiteral support for UTF8 strings

8b446aa

F1 Help support for UTF8 strings

fecea54

davidwengier requested a review from a team as a code owner April 6, 2022 04:32

ghost added the Area-IDE label Apr 6, 2022

davidwengier mentioned this pull request Apr 6, 2022

UTF8 string IDE follow up items #60582

Closed

7 tasks

Correctness

c9a1ded

Youssef1313 reviewed Apr 6, 2022

View reviewed changes

Youssef1313 mentioned this pull request Apr 6, 2022

IDE raw string literal feature list #58356

Closed

5 tasks

JoeRobich changed the base branch from main-vs-deps to main April 6, 2022 19:34

CyrusNajmabadi reviewed Apr 6, 2022

View reviewed changes

src/EditorFeatures/CSharp/BraceMatching/StringLiteralBraceMatcher.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Apr 6, 2022

View reviewed changes

src/Features/CSharp/Portable/SplitStringLiteral/SimpleStringSplitter.cs Outdated Show resolved Hide resolved

PR feedback

7c6a25d

ryzngard approved these changes Apr 13, 2022

View reviewed changes

davidwengier added 2 commits April 13, 2022 17:00

Unify kind checks

d0ba1a6

Add test

d27eec6

davidwengier enabled auto-merge (squash) April 13, 2022 07:00

davidwengier disabled auto-merge April 13, 2022 07:01

davidwengier enabled auto-merge (squash) April 13, 2022 07:48

davidwengier disabled auto-merge April 13, 2022 20:52

davidwengier merged commit a96c8ac into dotnet:main Apr 13, 2022

davidwengier deleted the UTF8 branch April 13, 2022 20:52

ghost added this to the Next milestone Apr 13, 2022

dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022

		SyntaxKind.SingleLineRawStringLiteralToken,
		SyntaxKind.MultiLineRawStringLiteralToken,

	// if this line is inside a string-literal or interpolated-text-content, then we definitely do not want to
	// touch what is inside there. Note: this will not apply to raw-string literals, which can potentially be
	// dedented safely depending on the position of their close terminator.
	if (tree.IsEntirelyWithinStringLiteral(textLine.Span.Start, cancellationToken))

Conversation

davidwengier commented Apr 6, 2022

Uh oh!

Youssef1313 Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

davidwengier Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

davidwengier Apr 6, 2022

Choose a reason for hiding this comment

Uh oh!

JoeRobich commented Apr 6, 2022

Uh oh!

Uh oh!

Uh oh!

davidwengier commented Apr 7, 2022

Uh oh!

azure-pipelines bot commented Apr 7, 2022

Uh oh!

davidwengier commented Apr 12, 2022

Uh oh!

ryzngard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davidwengier commented Apr 13, 2022

Uh oh!

davidwengier commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants