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

Incorrect Syntax Highlighting For Escaped Character '\"' #6899

Closed
LouisJenkinsCS opened this issue Jul 31, 2017 · 11 comments
Closed

Incorrect Syntax Highlighting For Escaped Character '\"' #6899

LouisJenkinsCS opened this issue Jul 31, 2017 · 11 comments

Comments

@LouisJenkinsCS
Copy link
Member

LouisJenkinsCS commented Jul 31, 2017

Summary of Problem

When using escape characters in a string, it compiles fine but the syntax highlighter will treat it as the beginning/end of a string. This occurs in Github's .chpl syntax highlighting, Markdown .chpl syntax highlighting, and in my text-editor Atom language-chapel. I am not sure how it is handled in any others.

Steps to Reproduce

Source Code:

var goodEscapedString = "\"Hello World\"";
var badEscapedString = "\"Hello World";
// This won't be highlighted properly...
for i in 1 .. 10 {
   on Locales[here.id] do writeln("Bad Syntax Highlighting...");
}
var endEscapedString = "\"";
// This will be highlighted properly...
for i in 1 .. 10 {
   on Locales[here.id] do writeln("Good Syntax Highlighting...");
}

TIO (Online Compiler)

@victor-ludorum
Copy link
Contributor

Hello Sir!! I am new to open source community and want to start by cracking this issue . Please guide me !!
Thanks in advance!!

@bradcray
Copy link
Member

@LouisJenkinsCS (as creator of the issue) and @lydia-duncan (as tagger with "good first issue"): any suggested pointers?

@lydia-duncan
Copy link
Member

Our highlighting stuff is spread across a few places. @LouisJenkinsCS has already pointed out Atom, where a pull request would be useful (I believe the source lives here). I don't remember specifically which of our highlighting solutions Github and Markdown draws from, but the rest of our highlighter implementations are here and here.

@ben-albrecht is the other person that might have some insights, I think

@LouisJenkinsCS
Copy link
Member Author

Sorry, didn't receive notification until Lydia pinged me (although I didn't see Brad's either).

@lydia-duncan The sublime-text version is just HTML, is that intentional?

@victor-ludorum My suggestion is to begin here

@LouisJenkinsCS
Copy link
Member Author

Second, I noticed that in chapel-tmbundle that there is no check for escape characters, which could be the issue. I don't believe it follows the specification...

I copy-pasted the specification for string literals below.

image

@lydia-duncan
Copy link
Member

Thanks for listing tmbundle, I missed that one (I think I deleted my bookmark when we pulled it into the chapel-lang heading because I'm silly).

I'm not sure about the sublime-text set up - I mostly included it for completeness, it doesn't look like it's been actively developed for a while.

@LouisJenkinsCS
Copy link
Member Author

To verify, 'chapel-tmbundle' is the repository being used to render Chapel, as seen here (Do ctrl + f for 'Chapel'), so 100% it is there.

@LouisJenkinsCS
Copy link
Member Author

My last suggestion would be to find a tool, like this, which would make importing the TextMate bundle (tmbundle) easier to modify and test, as doing it by hand would drive anyone insane. If its as easy as it looks, it might be a 5 minute fix or 5 hours, depending on how familiar you are with context-free grammars.

@victor-ludorum
Copy link
Contributor

@LouisJenkinsCS As you have pointed the code for chapel tmbundle , I have recognised that there is no check for escape character which you have mentioned too . So , in the code
`

                  <string>punctuation.definition.string.end.chapel</string>
		</dict>
		</dict>
		<key>name</key>
		<string>string.quoted.double.chapel</string>
	      </dict>

`

i.e after line 83 we should check for escape character by adding
`

                     <key>patterns</key>
		<array>
			<dict>
				<key>include</key>
				<string>#string_escaped_char</string>
			</dict>
			<dict>
				<key>include</key>
				<string>#string_placeholder</string>
			</dict>
		</array>

`
Another thing which I should mention is in chapel tmbundle line 108

`

		<array>
			<dict>
				<key>include</key>
				<string>#string_escaped_char</string>
			</dict>
		</array>

for **#string_escaped_char** is but we have not defined it anywhere in the chapel tmbundle code . So , basically we have to add previous mentioned code and define the #string_escaped_char and #string_placeholder in the chapel tmbundle code . **#string_escaped_char** definition :-

	<key>string_escaped_char</key>
	<dict>
		<key>patterns</key>
		<array>
			<dict>
				<key>match</key>
				<string>\\(\\|[abefnprtv'"?]|[0-3]\d{0,2}|[4-7]\d?|x[a-fA-F0-9]{0,2}|u[a-fA-F0-9]{0,4}|U[a-fA-F0-9]{0,8})</string>
				<key>name</key>
				<string>constant.character.escape.c</string>
			</dict>
			<dict>
				<key>match</key>
				<string>\\.</string>
				<key>name</key>
				<string>invalid.illegal.unknown-escape.c</string>
			</dict>
		</array>

`
and for #string_placeholder defiintion should be :

`

	<key>string_placeholder</key>
	<dict>
		<key>patterns</key>
		<array>
			<dict>
				<key>match</key>
				<string>(?x)%
						(\d+\$)?                             # field (argument #)
						[#0\- +']*                           # flags
						[,;:_]?                              # separator character (AltiVec)
						((-?\d+)|\*(-?\d+\$)?)?              # minimum field width
						(\.((-?\d+)|\*(-?\d+\$)?)?)?         # precision
						(hh|h|ll|l|j|t|z|q|L|vh|vl|v|hv|hl)? # length modifier
						[diouxXDOUeEfFgGaACcSspn%]           # conversion type
					</string>
				<key>name</key>
				<string>constant.other.placeholder.c</string>
			</dict>
		</array>
	</dict>

`
Hence 2 things should be added in the chapel tmbundle code

1 . checking of escaped character code after double character checking i.e after line 83
2 . definition of string_escaped_char and string_placeholder .

please suggest whether I am in a right way or not ??

@LouisJenkinsCS
Copy link
Member Author

First, I'd recommend forking the chapel-tmbundle project, committing the changes you suggested, and then making a Pull Request where I can give a code review. I'm currently away until tonight, but doing the above would allow others to give feedback as well (don't forget to @mention)

As well on first glance, your proposed solution looks sound, you just need to try it out next.

@victor-ludorum
Copy link
Contributor

Okay sir thanks! 😄 I will do as you have suggested .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants