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

IDE response to raw string literals #58846

Conversation

allisonchou
Copy link
Contributor

@allisonchou allisonchou commented Jan 13, 2022

Fixed the following bugs:

  • Spaces added between curly brackets in raw interpolated strings (e.g. { x } should be {x})
  • Missing completion in raw interpolated strings
  • Incorrect behavior in string split handler

Added tests for:

  • Completion
  • Quick info
  • String split command handler
  • Formatting (basic case)
  • Syntactic classifier (basic case)

TO-DO:

  • Testing multi-braced raw interpolated strings, e.g. $$"""{{x}}""". This is difficult with the current test infrastructure since $$ is often considered the cursor position.
  • Fix buggy syntactic classifier for multi-braced interpolated strings
  • More formatting tests
  • Handle string split command handler edge case

Relatest to test plan #55306


return position == token.Span.End &&
(token.Span.Length == startLength || (token.Span.Length > startLength && !token.ToString().EndsWith(string.Concat(tokenStart))));
}
Copy link
Member

Choose a reason for hiding this comment

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

hrmmmmmmm something seems fishy here. we likely want to talk about what's going on here and if it's necessary. i'm guessing we can answer this question without having to stringigy the token. note: the existing code seems uber suspect to me as well :)

@CyrusNajmabadi
Copy link
Member

overall seems fine. tests plz :)

Comment on lines 136 to +142
if (token.IsKind(SyntaxKind.InterpolatedStringStartToken) ||
token.IsKind(SyntaxKind.InterpolatedStringEndToken) ||
token.IsKind(SyntaxKind.InterpolatedStringTextToken))
token.IsKind(SyntaxKind.InterpolatedStringTextToken) ||
token.IsKind(SyntaxKind.InterpolatedSingleLineRawStringStartToken) ||
token.IsKind(SyntaxKind.InterpolatedSingleLineRawStringEndToken) ||
token.IsKind(SyntaxKind.InterpolatedMultiLineRawStringStartToken) ||
token.IsKind(SyntaxKind.InterpolatedMultiLineRawStringEndToken))
Copy link
Member

@Youssef1313 Youssef1313 Jan 14, 2022

Choose a reason for hiding this comment

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

I think we'd need another conditional branch for MultiLineRawStringLiteral and SingleLineRawStringLiteral here as well and coordinate the F1 keyword to be used with dotnet/docs team.

if (token.IsKind(SyntaxKind.MultiLineRawStringLiteral) || token.IsKind(SyntaxKind.SingleLineRawStringLiteral))
{
    text = "RawStringLiteral_CSharpKeyword"; // or """_CSharpKeyword
    return true;
}

Comment on lines 136 to +142
if (token.IsKind(SyntaxKind.InterpolatedStringStartToken) ||
token.IsKind(SyntaxKind.InterpolatedStringEndToken) ||
token.IsKind(SyntaxKind.InterpolatedStringTextToken))
token.IsKind(SyntaxKind.InterpolatedStringTextToken) ||
token.IsKind(SyntaxKind.InterpolatedSingleLineRawStringStartToken) ||
token.IsKind(SyntaxKind.InterpolatedSingleLineRawStringEndToken) ||
token.IsKind(SyntaxKind.InterpolatedMultiLineRawStringStartToken) ||
token.IsKind(SyntaxKind.InterpolatedMultiLineRawStringEndToken))
Copy link
Member

Choose a reason for hiding this comment

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

Consider refactoring this long list of syntax kinds into an extension in https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/CSharpExtensions.cs

@@ -143,7 +143,8 @@ public static IEnumerable<SyntaxKind> GetPreprocessorKeywordKinds()

public static bool IsPunctuation(SyntaxKind kind)
{
return kind >= SyntaxKind.TildeToken && kind <= SyntaxKind.QuestionQuestionEqualsToken;
return kind >= SyntaxKind.TildeToken && kind <= SyntaxKind.QuestionQuestionEqualsToken ||
kind is SyntaxKind.RawInterpolationOpenToken || kind is SyntaxKind.RawInterpolationCloseToken;
Copy link
Member

Choose a reason for hiding this comment

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

note: tehse tokens kinds are gone now. sorry!

}

[WpfFact, Trait(Traits.Feature, Traits.Features.SplitStringLiteral)]
public void TestMissingInRawStringLiteralInterpolation_MultiBrace()
Copy link
Member

Choose a reason for hiding this comment

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

test name seems wrong...

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review January 25, 2022 21:58
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner January 25, 2022 21:58
@allisonchou allisonchou changed the title [draft] IDE response to raw string literals IDE response to raw string literals Jan 25, 2022
Copy link
Member

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

It looks like the intender is applying one too many spaces for interpolated raw strings.

Comment on lines +90 to +94
// case 1: $"""$$
// """
// case 2: $"""
// text$$
// """
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// case 1: $"""$$
// """
// case 2: $"""
// text$$
// """
// case 1: $"""$$
// """
// case 2: $"""
// text$$
// """

Copy link
Member

Choose a reason for hiding this comment

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

the indenter isn't setting where the delimiter goes here. Instead, it's stating where the caret should go when enter is typed above teh delimeter. In that case, the caret is indented the same amount as the final string.

@CyrusNajmabadi
Copy link
Member

Merging in. Note that we still need tests around indentation and splitting of raw strings. I will add those myself in a followup.

@CyrusNajmabadi CyrusNajmabadi dismissed sharwell’s stale review January 26, 2022 16:57

The comment isn't stating that we will indent the end quote that much, just that we'll align the caret with the indentation of the end quote.

@CyrusNajmabadi CyrusNajmabadi merged commit ea2e59d into dotnet:features/RawStringLiterals Jan 26, 2022
@allisonchou allisonchou deleted the VerifyRawStringLiteral branch January 26, 2022 20:51
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.

4 participants