Skip to content

Add refactoring to convert normal (or verbatim) strings to raw strings.#59180

Merged
CyrusNajmabadi merged 14 commits intodotnet:mainfrom
CyrusNajmabadi:rawStringAnalyzerFixer
Feb 2, 2022
Merged

Add refactoring to convert normal (or verbatim) strings to raw strings.#59180
CyrusNajmabadi merged 14 commits intodotnet:mainfrom
CyrusNajmabadi:rawStringAnalyzerFixer

Conversation

@CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Jan 31, 2022

Looks like this:

image

Workign on the interpolated form now.

Relates to test plan #55306

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners January 31, 2022 20:32
@CyrusNajmabadi CyrusNajmabadi requested a review from a team January 31, 2022 20:32
@ghost ghost added the Area-IDE label Jan 31, 2022
@CyrusNajmabadi CyrusNajmabadi changed the base branch from main to features/RawStringLiterals January 31, 2022 20:38
// Check if we have an escaped character in the original string.
if (ch.Span.Length > 1)
{
// An escaped newline is fine to convert (to a multi-line raw string).
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a very Windows-centric assumption. What characters does IsNewLine match? Because, and maybe I missed them, but I didn't see tests for other newline characters like \n and \r on their own.

I think this needs a (may change semantics) (or some other wording) note, because this converts specifically defined newline character sequences into whatever the current machine/git repo/etc. happens to be configured to use. If a user checks out a git repo on a mac, runs this refactoring on VS Mac, this could break things.

Unless I'm wrong and the new raw strings exclusively encode \r\n into the value? If so, then I guess this is okay, but would be good to confirm that IsNewLine doesn't match anything other than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is a good point. For clarity, whatever newline sequence you wrote, we will embed into your converted string. This means from the language/compiler perspective, on that machine, the literal has the exact meaning as before (no semantics change).

Where it gets subtle though is the absolutely insane behavior of git where it may take a file and just change newlines even that are language relevant when sent to another machine.

In this case, the refactoring is in line with the lang and compiler. However, it is git that ends up causing issues here as it thinks it is sensible to touch programming lines that relate to content.

--

Note that this is a refactoring though. It is opted into the user, presumably because they actually think this is better. And it will be very clear that a single line became multi-line. So i think the user can be trusted to decide if that is acceptable for them given their git config.

Copy link
Member

Choose a reason for hiding this comment

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

For clarity, whatever newline sequence you wrote, we will embed into your converted string. This means from the language/compiler perspective, on that machine, the literal has the exact meaning as before (no semantics change).

Read the code now, and yes, I can see that. I think thats probably good, and perhaps even the saviour of it. If I was on a system that used LF, and converted a string that had CRLF, I would expect the IDE would somehow alert me to the fact that I had mixed line endings, and I could easily Ctrl+Z.

@sharwell sharwell changed the title Add fixer to convert normal (or verbatim) strings to raw strings. Add refactoring to convert normal (or verbatim) strings to raw strings. Feb 1, 2022
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner February 2, 2022 18:18
@CyrusNajmabadi CyrusNajmabadi changed the base branch from features/RawStringLiterals to main February 2, 2022 18:18
@CyrusNajmabadi CyrusNajmabadi merged commit aa97465 into dotnet:main Feb 2, 2022
@ghost ghost added this to the Next milestone Feb 2, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the rawStringAnalyzerFixer branch February 18, 2022 20:50
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.

3 participants