language-definitions AutoHotkey: Modified block comment matching by nnnik · Pull Request #1703 · PrismJS/prism

nnnik · 2019-01-13T11:02:28Z

The block comment matching has been fixed.
I also made the behavior as close as possible to, how AutoHotkey actually parses block comments as of 1.1.30.01.
The parser looks for a line starting with /* (excluding whitespace).
Then starting from the next line it will look for */ to end the comment.
The closing tag seems to be entirely optional and the entire remaining file will become a comment block if no ending tag is found.
Any code directly after a starting sequence is ignored until the parser encounters a new line.

Modified comment matching

Block comments now need to start on their own line

RunDevelopment

Thank you for contributing!

However before we can merge this, we will have to make a few changes.

The comment pattern has two parts:

(^[^";\n]*("[^"\n]*?"[^"\n]*?)*)(?:;.*$) to match single-line comments and
^\s*\/\*[^\n]*(?:\n(?:[^*]|\*[^\/])*(?:\*\/)?)? to match multi-line comments.

You only changed the second part but we might as well take this chance to improve the first one as well, so let's start with that:

The single-line comment pattern can be simplified to ;.*. The $ at the end is redundant (see .) and the lookbehind group (^[^";\n]*("[^"\n]*?"[^"\n]*?)*) is to prevent matching ;s inside strings.
I removed the lookbehind group because Prism has a better way to deal with that: greedy matching. So making string greedy will have the same effect and makes the pattern simpler.

Now to multi-line comments:

First of all, there are 3 possible end of line sequences: \n, \r, and \r\n. So the pattern doesn't support \r line ends.

Second, the requirement for */ to close the comment is to be at the start of a line (plus spaces). This means that the following is a comment:

/*
 * I don't end here */ but there
 */

Knowing that, the pattern should be (^\s*)\/\*[\s\S]*?^\s*\*\/.
(Leading and trailing spaces should not be part of any token, so I use the lookbehind group to remove them.)

This doesn't yet include comments which aren't closed by */ and end at the end of file, so let's add them:
(^\s*)\/\*(?:[\s\S]*?^\s*\*\|[\s\S]*)/.

Please verify that this works and please extend the comment test. I haven't thoroughly tested it.

Also, are non-space characters allowed on the same line after the closing */? E.g.

/*
 */ where do I end?
 */

nnnik · 2019-01-13T14:17:03Z

However before we can merge this, we will have to make a few changes.

The comment pattern has two parts:

(^[^";\n]("[^"\n]?"[^"\n]?))(?:;.$) to match single-line comments and
^\s/*[^\n](?:\n(?:[^*]|*[^\/])(?:*/)?)? to match multi-line comments.
You only changed the second part but we might as well take this chance to improve the first one as well, so let's start with that:

The single-line comment pattern can be simplified to ;.. The $ at the end is redundant (see .) and the lookbehind group (^[^";\n]("[^"\n]?"[^"\n]?)*) is to prevent matching ;s inside strings.
I removed the lookbehind group because Prism has a better way to deal with that: greedy matching. So making string greedy will have the same effect and makes the pattern simpler.

I dont know about the single line comments for now. I haven't looked into them extensively.
Especially since I haven't checked how extensive the string detection is - especially with the 2 syntaxes around and continuation sections etc. - so I'm going to leave them as it stands and might work on them at another time.

Now to multi-line comments:

First of all, there are 3 possible end of line sequences: \n, \r, and \r\n. So the pattern doesn't support \r line ends.

I kind of assumed that the parser would handle the newlines accordingly. AutoHotkey can deal with all types of Unicode newlines - sadly thats not a feature javascript can handle apparently.
For now I guess it would be fine if we only cover all \r and \n options.

Second, the requirement for */ to close the comment is to be at the start of a line (plus spaces). This means that the following is a comment:

Thats incorrect - the latest AHK Version use exactly the behavior I implemented.
I didn't read the source code though - I only ran several tests with a few AHK scripts.
Namely:

/**/
Msgbox this is still a comment
And will go on without closing.

Msgbox /*
Msgbox This is not a comment */

/* This is all part of the
Comment */Msgbox this is code

Msgbox the following is recognized as a comment:
/*

So this would be correct:

/*
 * I do end here */ Msgbox not there

(Leading and trailing spaces should not be part of any token, so I use the lookbehind group to remove them.)

Oh I will fix that

I will also add more unit tests to verify the new features.

Split comment-blocks from single-line comments Removed leading whitespace from match Added unit tests for new features

nnnik · 2019-01-13T15:20:50Z

I added multiple regexes to the comment matching in order to split of multi-line comments from single-line comments.
I have one issue with the unit tests though. For multiline testing I added a few new tests which involve new-line characters. Locally those were encoded as \r\n and I had to match that in the string. If somebody changes those line-endings locally (for reasons I cannot understand) the unit tests will actually fail.
Which way would you suggest around this? Or is it fine and I'm just worrying for nothing?

nnnik · 2019-01-13T16:09:43Z

So I went ahead and looked into single line comments and it seems that the only requirements that AHK has regarding single line comments are:

; must follow a whitespace character or new-line
continuation sections disregard ; for comments unless set in their options

Which actually means that strings will be torn apart by ; if it follows a whitespace
See:

FileDelete,  test.txt
FileAppend, "This is a comment: ; inside the comment"
, test.txt
FileAppend, "`nThis is not a comment:; still just text"`n
, test.txt
;FileAppend,  % "`n This is a comment and would cause an error if it wasn't commented out (commentception?) ; inside the comment"
;, test.txt
FileAppend, 
(
;This is not a comment
Its just text and more text
/*
No comments here
*/
), test.txt

Will result in the creation of test.txt with the content:

"This is a comment:"
This is not a comment:; still just text"
;This is not a comment
Its just text and more text
/*
No comments here
*/

It will be hard to detect whether "" is currently inside an expression or not so highlighting it all the time doesn't seem to be wrong. Since ; interupts it not highlighting it then seems to be the safest option.
E.g.:

Msgbox This is text
Msgbox "This is still text" this too "and also this"
Msgbox % "This is text" thisIsAVariable
SoundBeep "13", thisIsAVariable ;since both parameters can be expressions

Single line comments can be reduced to:

/(^|\s);.*/m

using this logic.

nnnik · 2019-01-13T16:41:41Z

I greatly simplified single-line comment matching and made the behavior equivalent to autohotkeys own behavior. Strings don't take precedence over comments in my browser but I will probably modify the regex for strings in order to cover that case.
Sadly greedy cannot be used since continuation sections exist.

…ized as functions

nnnik · 2019-01-15T11:04:17Z

This one is a minor fix that fixes if( and while(

RunDevelopment

Sorry for the delay.

RunDevelopment · 2019-01-15T13:50:16Z

components/prism-autohotkey.js

-	},
+	'comment': [
+		{
+			pattern: /(^|\s);.*/m,


You don't need the m flag.
\s will also match \n and \r. Do keep ^ though.

RunDevelopment · 2019-01-15T13:56:37Z

components/prism-autohotkey.js

+			lookbehind: true
+		},
+		{
+			pattern: /(^\s*)\/\*[^\n\r]*(?:(\n|\r|\r\n)(?:[^*]|\*[^\/])*(?:\*\/)?)?/m,


How about this:

/(^\s*)\/\*.*$(?:[\s\S]*?\*\/|[\s\S]*)/m

Like this, we can just let JS handle all different kinds of line breaks.

RunDevelopment · 2019-01-15T13:57:13Z

components/prism-autohotkey.js

+			lookbehind: true
+		},
+		{
+			pattern: /(^\s*)\/\*[^\n\r]*(?:(\n|\r|\r\n)(?:[^*]|\*[^\/])*(?:\*\/)?)?/m,


This has to be greedy because of:

/* ; what now? */

joedf · 2019-04-22T03:10:00Z

Has this been integrated into v1.16?

RunDevelopment · 2019-04-22T07:19:59Z

There are some comments that haven't been addressed yet which is why this PR is still open.
The soonest Prism version these changes will be available in will most likely be v1.17.0.

joedf · 2019-04-23T02:07:29Z

There are some comments that haven't been addressed yet which is why this PR is still open.
The soonest Prism version these changes will be available in will most likely be v1.17.0.

Okay cool, thanks! 👍

RunDevelopment · 2019-07-17T17:45:12Z

@nnnik Is this still being worked on or shall I take over?

mAAdhaTTah · 2019-07-21T17:49:42Z

@RunDevelopment Gonna drop this from 1.17.0, if we want to pub today.

RunDevelopment · 2019-07-21T17:59:11Z

@mAAdhaTTah Agreed.

joedf · 2020-05-25T00:22:12Z

What's wrong with this PR? anything missing?
We are currently using a customized version of prismjs v1.15 in the meantime...
(over at https://www.autohotkey.com/boards/)

nnnik added 2 commits January 13, 2019 11:37

language-definitions AutoHotkey:

26688ef

Modified comment matching

language-definitions AutoHotkey: Modified Block Comment matching

1923e6c

Block comments now need to start on their own line

nnnik mentioned this pull request Jan 13, 2019

AutoHotkey comment blocks don't close properly #1700

Closed

RunDevelopment requested changes Jan 13, 2019

View reviewed changes

RunDevelopment added enhancement language-definitions needs changes labels Jan 13, 2019

Implemented discussed changes

3a63337

Split comment-blocks from single-line comments Removed leading whitespace from match Added unit tests for new features

Added correct single-line comment behavior

9c3ac16

Reordered function and punctuation to fix if( and while( being recogn…

098a2c7

…ized as functions

RunDevelopment requested changes Jan 15, 2019

View reviewed changes

RunDevelopment added this to the 1.17.0 milestone Jul 17, 2019

mAAdhaTTah removed this from the 1.17.0 milestone Jul 21, 2019

RunDevelopment mentioned this pull request May 30, 2020

Autohotkey: Improved comments and other improvements #2412

Merged

RunDevelopment closed this in #2412 Jun 7, 2020

Uh oh!

Conversation

nnnik commented Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RunDevelopment left a comment

Choose a reason for hiding this comment

Uh oh!

nnnik commented Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nnnik commented Jan 13, 2019

Uh oh!

nnnik commented Jan 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nnnik commented Jan 13, 2019

Uh oh!

nnnik commented Jan 15, 2019

Uh oh!

RunDevelopment left a comment

Choose a reason for hiding this comment

Uh oh!

RunDevelopment Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

RunDevelopment Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

RunDevelopment Jan 15, 2019

Choose a reason for hiding this comment

Uh oh!

joedf commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RunDevelopment commented Apr 22, 2019

Uh oh!

joedf commented Apr 23, 2019

Uh oh!

RunDevelopment commented Jul 17, 2019

Uh oh!

mAAdhaTTah commented Jul 21, 2019

Uh oh!

RunDevelopment commented Jul 21, 2019

Uh oh!

joedf commented May 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nnnik commented Jan 13, 2019 •

edited

Loading

nnnik commented Jan 13, 2019 •

edited

Loading

nnnik commented Jan 13, 2019 •

edited

Loading

joedf commented Apr 22, 2019 •

edited

Loading