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

U+0085 not recognised as a newline character in CSharpCodeGenerator.cs (#1740) #1853

Merged
merged 6 commits into from
Jan 23, 2020
Merged

U+0085 not recognised as a newline character in CSharpCodeGenerator.cs (#1740) #1853

merged 6 commits into from
Jan 23, 2020

Conversation

hamarb123
Copy link
Contributor

@hamarb123 hamarb123 commented Jan 17, 2020

U+0085 is not recognised as a newline character in CSharpCodeGenerator.cs, this fixes that.

This an issue is because it is recognised as a newline character in C# (see here), which cannot be in literal strings.

Specifically:

new_line_character
    : '<Carriage return character (U+000D)>'
    | '<Line feed character (U+000A)>'
    | '<Next line character (U+0085)>'
    | '<Line separator character (U+2028)>'
    | '<Paragraph separator character (U+2029)>'
    ;

and

regular_string_literal
    : '"' regular_string_literal_character* '"'
    ;

regular_string_literal_character
    : single_regular_string_literal_character
    | simple_escape_sequence
    | hexadecimal_escape_sequence
    | unicode_escape_sequence
    ;

single_regular_string_literal_character
    : '<Any character except " (U+0022), \\ (U+005C), and new_line_character>'
    ;

If the CSharpCodeGenerator class was used to make code with a string in it that has U+0085 in it, it would generate invalid C# code.

I have also added a test which ensures that all of the newline characters (other than \n and \r) are generated properly.

Fix for #1740

U+0085 not recognised as a newline character in CSharpCodeGenerator.cs

Fix $1740
U+0085 not recognised as a newline character in CSharpCodeGenerator.cs
@dnfclas
Copy link

dnfclas commented Jan 17, 2020

CLA assistant check
All CLA requirements met.

U+0085 not recognised as a newline character in CSharpCodeGenerator.cs
This change fixes a previous test that incorrectly causes the character to be forced out of being in unicode format.
@hamarb123 hamarb123 changed the title U+0085 not recognised as a newline character in CSharpCodeGenerator.cs (#1740) [WIP] U+0085 not recognised as a newline character in CSharpCodeGenerator.cs (#1740) Jan 17, 2020
@hamarb123
Copy link
Contributor Author

It seems like the test is failing on 2 platforms: Windows_NT netfx x86 Release, and Windows_NT x64.
I imagine the reasoning behind the netfx failure is because it must still have the old code, but can anyone tell me what the other one is, its error messages don't make sense to me.
Also, what should I do to fix the netfx one, should I just revert the tests for the netfx platform, or is there something else that I should do?

@hamarb123
Copy link
Contributor Author

This change will be disabled for .NET Framework, and Microsoft can add it if they wish, see #1913

@danmoseley danmoseley requested a review from cston January 22, 2020 20:46
@hamarb123 hamarb123 changed the title [WIP] U+0085 not recognised as a newline character in CSharpCodeGenerator.cs (#1740) U+0085 not recognised as a newline character in CSharpCodeGenerator.cs (#1740) Jan 23, 2020
@hamarb123
Copy link
Contributor Author

hamarb123 commented Jan 23, 2020

As all of the checks have now passed, this is now ready for review.
Edit: also note, the numbers in 'Merge pull request #number' aren't accurate AFAIK, I'm pretty sure the 2nd one wasn't merging PR #2, it was just the second one merged.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@stephentoub stephentoub merged commit 4c79f98 into dotnet:master Jan 23, 2020
@cston
Copy link
Member

cston commented Jan 23, 2020

Thanks @hamarb123!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants