Change comment parsing (SkriptLang#6583)

* Rework to only allow single # in quoted strings

* fix save()

* fix state machine bug with exiting %s

* Update skript-aliases

* remove unreachable code

---------

Co-authored-by: Moderocky <admin@moderocky.com>

src/main/java/ch/njol/skript/config/Node.java

@@ -23,6 +23,7 @@
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import ch.njol.skript.SkriptConfig;
 import org.eclipse.jdt.annotation.Nullable;
 
 import ch.njol.skript.Skript;
@@ -114,17 +115,27 @@ public void move(final SectionNode newParent) {
 		p.remove(this);
 		newParent.add(this);
 	}
-
-	@SuppressWarnings("null")
-	private final static Pattern linePattern = Pattern.compile("^((?:[^#]|##)*)(\\s*#(?!#).*)$");
-
+
 	/**
 	 * Splits a line into value and comment.
 	 * <p>
 	 * Whitespace is preserved (whitespace in front of the comment is added to the value), and any ## in the value are replaced by a single #. The comment is returned with a
 	 * leading #, except if there is no comment in which case it will be the empty string.
+	 *
+	 * @param line the line to split
+	 * @return A pair (value, comment).
+	 */
+	public static NonNullPair<String, String> splitLine(String line) {
+		return splitLine(line, new AtomicBoolean(false));
+	}
+
+	/**
+	 * Splits a line into value and comment.
+	 * <p>
+	 * Whitespace is preserved (whitespace in front of the comment is added to the value), and any ## not in quoted strings in the value are replaced by a single #. The comment is returned with a
+	 * leading #, except if there is no comment in which case it will be the empty string.
 	 * 
-	 * @param line
+	 * @param line the line to split
 	 * @param inBlockComment Whether we are currently inside a block comment
 	 * @return A pair (value, comment).
 	 */
@@ -138,34 +149,99 @@ public static NonNullPair<String, String> splitLine(String line, AtomicBoolean i
 		} else if (inBlockComment.get()) { // we're inside a comment, all text is a comment
 			return new NonNullPair<>("", line);
 		}
-		final Matcher m = linePattern.matcher(line);
-		boolean matches = false;
-		try {
-			matches = line.contains("#") && m.matches();
-		} catch (StackOverflowError e) { // Probably a very long line
-			handleNodeStackOverflow(e, line);
+
+		// idea: find first # that is not within a string or variable name. Use state machine to determine whether a # is a comment or not.
+		int length = line.length();
+		StringBuilder finalLine = new StringBuilder(line);
+		int numRemoved = 0;
+		SplitLineState state = SplitLineState.CODE;
+		SplitLineState previousState = SplitLineState.CODE; // stores the state prior to entering %, so it can be re-set when leaving
+		// find next " or %
+		for (int i = 0; i < length; i++) {
+			char c = line.charAt(i);
+			// check for things that can be escaped by doubling
+			if (c == '%' || c == '"' || c == '#') {
+				// skip if doubled (only skip ## outside of strings)
+				if ((c != '#' || state != SplitLineState.STRING) && i + 1 < length && line.charAt(i + 1) == c) {
+					if (c == '#') { // remove duplicate #
+						finalLine.deleteCharAt(i - numRemoved);
+						numRemoved++;
+					}
+					i++;
+					continue;
+				}
+				SplitLineState tmp = state;
+				state = SplitLineState.update(c, state, previousState);
+				if (state == SplitLineState.HALT)
+					return new NonNullPair<>(finalLine.substring(0, i - numRemoved), line.substring(i));
+				// only update previous state when we go from !CODE -> CODE due to %
+				if (c == '%' && state == SplitLineState.CODE)
+					previousState = tmp;
+			}
 		}
-		if (matches)
-			return new NonNullPair<>("" + m.group(1).replace("##", "#"), "" + m.group(2));
-		return new NonNullPair<>("" + line.replace("##", "#"), "");
+		return new NonNullPair<>(finalLine.toString(), "");
 	}
 
 	/**
-	 * Splits a line into value and comment.
-	 * <p>
-	 * Whitespace is preserved (whitespace in front of the comment is added to the value), and any ## in the value are replaced by a single #. The comment is returned with a
-	 * leading #, except if there is no comment in which case it will be the empty string.
-	 *
-	 * @param line
-	 * @return A pair (value, comment).
+	 * state machine:<br>
+	 * ": CODE -> STRING,  			STRING -> CODE, 	VARIABLE -> VARIABLE<br>
+	 * %: CODE -> PREVIOUS_STATE, 	STRING -> CODE, 	VARIABLE -> CODE<br>
+	 * {: CODE -> VARIABLE, 		STRING -> STRING,	VARIABLE -> VARIABLE<br>
+	 * }: CODE -> CODE, 			STRING -> STRING, 	VARIABLE -> CODE<br>
+	 * #: CODE -> HALT, 			STRING -> STRING, 	VARIABLE -> HALT<br>
+	 * invalid characters simply return given state.<br>
 	 */
-	public static NonNullPair<String, String> splitLine(String line) {
-		return splitLine(line, new AtomicBoolean(false));
+	private enum SplitLineState {
+		HALT,
+		CODE,
+		STRING,
+		VARIABLE;
+
+		/**
+		 * Updates the state given a character input.
+		 * @param c character input. '"', '%', '{', '}', and '#' are valid.
+		 * @param state the current state of the machine
+		 * @param previousState the state of the machine when it last entered a % CODE % section
+		 * @return the new state of the machine
+		 */
+		private static SplitLineState update(char c, SplitLineState state, SplitLineState previousState) {
+			if (state == HALT)
+				return HALT;
+
+			switch (c) {
+				case '%':
+					if (state == CODE)
+						return previousState;
+					return CODE;
+				case '"':
+					switch (state) {
+						case CODE:
+							return STRING;
+						case STRING:
+							return CODE;
+						default:
+							return state;
+					}
+				case '{':
+					if (state == STRING)
+						return STRING;
+					return VARIABLE;
+				case '}':
+					if (state == STRING)
+						return STRING;
+					return CODE;
+				case '#':
+					if (state == STRING)
+						return STRING;
+					return HALT;
+			}
+			return state;
+		}
 	}
 
 	static void handleNodeStackOverflow(StackOverflowError e, String line) {
 		Node n = SkriptLogger.getNode();
-		SkriptLogger.setNode(null); // Avoid duplicating the which node error occurred in paranthesis on every error message
+		SkriptLogger.setNode(null); // Avoid duplicating the which node error occurred in parentheses on every error message
 
 		Skript.error("There was a StackOverFlowError occurred when loading a node. This maybe from your scripts, aliases or Skript configuration.");
 		Skript.error("Please make your script lines shorter! Do NOT report this to SkriptLang unless it occurs with a short script line or built-in aliases!");
@@ -214,12 +290,43 @@ protected String getIndentation() {
 	abstract String save_i();
 
 	public final String save() {
-		return getIndentation() + save_i().replace("#", "##") + comment;
+		return getIndentation() + escapeUnquotedHashtags(save_i()) + comment;
 	}
 
 	public void save(final PrintWriter w) {
 		w.println(save());
 	}
+
+	private static String escapeUnquotedHashtags(String input) {
+		int length = input.length();
+		StringBuilder output = new StringBuilder(input);
+		int numAdded = 0;
+		SplitLineState state = SplitLineState.CODE;
+		SplitLineState previousState = SplitLineState.CODE;
+		// find next " or %
+		for (int i = 0; i < length; i++) {
+			char c = input.charAt(i);
+			// check for things that can be escaped by doubling
+			if (c == '%' || c == '"' || c == '#') {
+				// escaped #s outside of strings
+				if (c == '#' && state != SplitLineState.STRING) {
+					output.insert(i + numAdded, "#");
+					numAdded++;
+					continue;
+				}
+				// skip if doubled (not #s)
+				if (i + 1 < length && input.charAt(i + 1) == c) {
+					i++;
+					continue;
+				}
+				SplitLineState tmp = state;
+				state = SplitLineState.update(c, state, previousState);
+				previousState = tmp;
+			}
+		}
+		return output.toString();
+	}
+
 
 	@Nullable
 	public SectionNode getParent() {

src/main/java/ch/njol/skript/expressions/ExprChestInventory.java

@@ -52,7 +52,7 @@
 	"set slot 4 of {_inventory} to a diamond named \"example\"",
 	"open {_inventory} to player",
 	"",
-	"open chest inventory named \"<##00ff00>hex coloured title!\" with 6 rows to player",
+	"open chest inventory named \"<#00ff00>hex coloured title!\" with 6 rows to player",
 })
 @RequiredPlugins("Paper 1.16+ (chat format)")
 @Since("2.2-dev34, 2.8.0 (chat format)")

src/main/java/ch/njol/skript/expressions/ExprLoopIteration.java

@@ -45,7 +45,7 @@
 	"",
 	"loop {top-balances::*}:",
 	"\tif loop-iteration <= 10:",
-	"\t\tbroadcast \"##%loop-iteration% %loop-index% has $%loop-value%\"",
+	"\t\tbroadcast \"#%loop-iteration% %loop-index% has $%loop-value%\"",
 })
 @Since("2.8.0")
 public class ExprLoopIteration extends SimpleExpression<Long> {

src/main/java/ch/njol/skript/expressions/ExprLoopValue.java

@@ -62,7 +62,7 @@
 	"",
 	"loop {top-balances::*}:",
 		"\tloop-iteration <= 10",
-		"\tsend \"##%loop-iteration% %loop-index% has $%loop-value%\"",
+		"\tsend \"#%loop-iteration% %loop-index% has $%loop-value%\"",
 })
 @Since("1.0, 2.8.0 (loop-counter)")
 public class ExprLoopValue extends SimpleExpression<Object> {

src/main/java/ch/njol/skript/util/Utils.java

@@ -641,7 +641,7 @@ public String run(final Matcher m) {
 		return "" + m;
 	}
 
-	private static final Pattern HEX_PATTERN = Pattern.compile("(?i)#?[0-9a-f]{6}");
+	private static final Pattern HEX_PATTERN = Pattern.compile("(?i)#{0,2}[0-9a-f]{6}");
 
 	/**
 	 * Tries to get a {@link ChatColor} from the given string.

src/main/resources/config.sk

@@ -22,7 +22,7 @@
 # This file, all scripts and other files ending in .sk are NOT .yml/YAML files, but very similar!
 # Please remember the following when editing files:
 # - To indent sections you can use spaces like in YAML, but tabs are also allowed. Just remember to stick to the one or the other for a section/trigger.
-# - '#' starts a comment like in YAML. If you don't want it to start a comment simply double it: '##' (You also have to double these in "quoted text")
+# - '#' starts a comment like in YAML. If you don't want it to start a comment simply double it: '##' (You do NOT have to double these in "quoted text")
 # - If you use special characters (§, äöü, éèàôç, ñ, etc.) you have to encode the file in UTF-8.
 #
 

src/main/resources/scripts/-examples/text formatting.sk

@@ -4,15 +4,15 @@
 # You can also use <> for colours and formats, like `<red>` for red and `<bold>` for bold
 #
 # In Minecraft 1.16, support was added for 6-digit hexadecimal colors to specify custom colors other than the 16 default color codes.
-# The tag for these colors looks like this: <##hex code> e.g. `<##123456>`
+# The tag for these colors looks like this: <#hex code> e.g. `<#123456>`
 #
 
 command /color:
     permission: skript.example.color
     trigger:
         send "&6This message is golden."
         send "<light red><bold>This message is light red and bold."
-        send "<##FF0000>This message is red."
+        send "<#FF0000>This message is red."
 
 #
 # Other formatting options are also available.

src/test/java/org/skriptlang/skript/test/tests/config/NodeTest.java

@@ -47,6 +47,7 @@ public void splitLineTest() {
 				{"#########", "", "#########"},
 				{"a##b#c##d#e", "a#b", "#c##d#e"},
 				{" a ## b # c ## d # e ", " a # b ", "# c ## d # e "},
+				{"a b \"#a  ##\" # b \"", "a b \"#a  ##\" ", "# b \""},
 		};
 		for (String[] d : data) {
 			NonNullPair<String, String> p = Node.splitLine(d[0]);

src/test/skript/tests/misc/comments.sk

@@ -0,0 +1,46 @@
+test "comments":
+	parse:
+		set {_a} to {_b} # test
+	assert last parse logs is not set with "skript should be able to handle inline comments but did not"
+
+
+	parse:
+		assert "a" is "a" with "wrong number of hashtags"
+		assert "#a" is join "#", and "a" with "wrong number of hashtags"
+		assert "##a" is join "#", "#", and "a" with "wrong number of hashtags"
+		assert "###a" is join "#", "#", "#", and "a" with "wrong number of hashtags"
+	assert last parse logs is not set with "skript should be able to handle strings any number of hashtags but did not"
+
+
+	parse:
+		assert "a%"#"%" is join "a", and "#" with "wrong number of hashtags"
+		assert "#a%"#}"%" is join "#", "a", and "#}" with "wrong number of hashtags"
+		assert "##a%"#"%" is join "#", "#", "a", and "#" with "wrong number of hashtags"
+		assert "#{##a%"#"%" is join "#{", "#", "#", "a", and "#" with "wrong number of hashtags"
+	assert last parse logs is not set with "skript should be able to handle complex strings any number of hashtags but did not"
+
+
+	parse:
+		set {_a} to "<#abcdef>test"
+		set {_b} to "<##abcdef>test"
+		assert uncoloured {_a} is "test" with "failed to parse single hashtag colour code"
+		assert uncoloured {_b} is "test" with "failed to parse double hashtag colour code"
+	assert last parse logs is not set with "skript should be able to handle hex colour codes but did not"
+
+	parse:
+		set {_a} to "###SH#JABJ#BJ#JB#K#BH#G#J##J#HJ%%KJB#JKK%%""%%""%%""%%#""##%%""#""%%##""#%""%##%"#"""%#"#!!""#""#L@$L@:@#L@K:L%@^$:"#^#:^J$%:K^J%&LK:#::&&^^^%%test
+	assert last parse logs is not set with "skript should be able to handle very messy string but did not"
+
+
+	parse:
+		set {_a##} to "test"
+		set {##_b} to "test"
+		set {##_b::%"{}"%} to "test"
+		set {##_b::%"{}#"%} to "#test"
+		assert {##_b::%"{}#"%} is join "#" and "test" with "failed state machine check"
+	assert last parse logs is not set with "skript should be able to handle hashtags in variable names but did not"
+
+
+	parse:
+		set {##_b::%"{}"#%} to "#test"
+	assert last parse logs is set with "skript should not be able to handle hashtags in an expression in a variable name but did"

src/test/skript/tests/regressions/4235-floating point errors rounding functions.sk

@@ -1,12 +1,12 @@
 test "floating point errors in rounding functions":
-	#assert ceil(100*0.07) is 7 with "ceil function doesn't adjust for floating point errors ##1"
-	#assert ceil(100*0.033 - 0.3) is 3 with "ceil function doesn't adjust for floating point errors ##2"
+	#assert ceil(100*0.07) is 7 with "ceil function doesn't adjust for floating point errors"
+	#assert ceil(100*0.033 - 0.3) is 3 with "ceil function doesn't adjust for floating point errors"
 
-	#assert rounded up 100*0.07 is 7 with "ceil expression doesn't adjust for floating point errors ##1"
-	#assert rounded up 100*0.033 - 0.3 is 3 with "ceil expression doesn't adjust for floating point errors ##2"
+	#assert rounded up 100*0.07 is 7 with "ceil expression doesn't adjust for floating point errors"
+	#assert rounded up 100*0.033 - 0.3 is 3 with "ceil expression doesn't adjust for floating point errors"
 
 	set {_sum} to 0
 	loop 100 times:
 		add 0.1 to {_sum}
-	assert floor({_sum}) is 10 with "floor function doesn't adjust for floating point errors ##1"
-	assert rounded down {_sum} is 10 with "floor expression doesn't adjust for floating point errors ##1"
+	assert floor({_sum}) is 10 with "floor function doesn't adjust for floating point errors"
+	assert rounded down {_sum} is 10 with "floor expression doesn't adjust for floating point errors"

src/test/skript/tests/regressions/4664-formatted time.sk

@@ -3,18 +3,18 @@ test "formatted time":
 	set {_now} to now
 
 	set {_date1} to {_now} formatted
-	assert {_date1} = {_now} formatted as {_default} with "default date format failed ##1"
+	assert {_date1} = {_now} formatted as {_default} with "default date format failed"
 
 	set {_date2} to {_now} formatted human-readable
-	assert {_date2} = {_now} formatted as {_default} with "default date format failed ##2"
+	assert {_date2} = {_now} formatted as {_default} with "default date format failed"
 
 	set {_date3} to {_now} formatted as "HH:mm"
-	assert length of {_date3} = 5 with "custom date format failed ##1"
+	assert length of {_date3} = 5 with "custom date format failed"
 
 	set {_cFormat} to "hh:mm"
 	set {_date4} to {_now} formatted as {_cFormat}
-	assert length of {_date4} = 5 with "custom date format failed ##2"
+	assert length of {_date4} = 5 with "custom date format failed"
 
 	set {_cFormat2} to "i"
 	set {_date5} to {_now} formatted as {_cFormat2}
-	assert {_date5} is not set with "custom date format failed ##3"
+	assert {_date5} is not set with "custom date format failed"

src/test/skript/tests/regressions/4769-fire-visualeffect.sk

@@ -8,8 +8,8 @@ test "fire visual effect comparison":
 	set block at spawn of world "world" to {_block}
 	play 5 fire at spawn of world "world"
 	assert "fire" parsed as visual effect is fire with "failed to compare visual effects"
-	assert "fire" parsed as visual effect is "fire" parsed as itemtype to fail with "failed to compare visual effects ##2"
+	assert "fire" parsed as visual effect is "fire" parsed as itemtype to fail with "failed to compare visual effects"
 	spawn a chicken at spawn of world "world":
 		assert event-entity is a chicken with "failed to compare a chicken"
-		assert event-entity is a "chicken" parsed as itemtype to fail with "failed to compare a chicken ##2"
+		assert event-entity is a "chicken" parsed as itemtype to fail with "failed to compare a chicken"
 		clear event-entity

src/test/skript/tests/regressions/535-math is done in the wrong order.sk

@@ -1,10 +1,10 @@
 test "math order":
-	assert 1 + 1 = 2 with "basic math ##1 failed"
-	assert 5 - 3 = 2 with "basic math ##2 failed"
+	assert 1 + 1 = 2 with "basic math failed"
+	assert 5 - 3 = 2 with "basic math failed"
 
-	assert 5 - 3 - 1 = 1 with "basic chained math ##1 failed"
-	assert 5-3-2 = 0 with "basic chained math ##2 failed"
-	assert 10 - 1 - 5 = 4 with "basic chained math ##3 failed"
+	assert 5 - 3 - 1 = 1 with "basic chained math failed"
+	assert 5-3-2 = 0 with "basic chained math failed"
+	assert 10 - 1 - 5 = 4 with "basic chained math failed"
 
-	assert (9 - 1) - 3 = 5 with "basic chained math with parentheses ##1 failed"
-	assert 9 - (1 - 3) = 11 with "basic chained math with parentheses ##2 failed"
+	assert (9 - 1) - 3 = 5 with "basic chained math with parentheses failed"
+	assert 9 - (1 - 3) = 11 with "basic chained math with parentheses failed"