PatternParser: add warning to specific edge-case and show registratio…

…n errors (#141) Co-authored-by: Mwexim <[email protected]>
Mwexim · Oct 31, 2023 · f2f6ced · f2f6ced
1 parent a2659f8
commit f2f6ced
Show file tree

Hide file tree

Showing 8 changed files with 71 additions and 30 deletions.
diff --git a/src/main/java/io/github/syst3ms/skriptparser/Parser.java b/src/main/java/io/github/syst3ms/skriptparser/Parser.java
@@ -130,9 +130,7 @@ public static void init(String[] mainPackages, String[] subPackages, String[] pr
         if (!logs.isEmpty()) {
             System.out.print(ConsoleColors.PURPLE);
             System.out.println("Registration log:");
-        }
-        printLogs(logs, time, false);
-        if (!logs.isEmpty()) {
+            printLogs(logs, time, true);
             System.out.println();
         }
     }

diff --git a/src/main/java/io/github/syst3ms/skriptparser/lang/VariableString.java b/src/main/java/io/github/syst3ms/skriptparser/lang/VariableString.java
@@ -5,8 +5,6 @@
 import io.github.syst3ms.skriptparser.parsing.ParseContext;
 import io.github.syst3ms.skriptparser.parsing.ParserState;
 import io.github.syst3ms.skriptparser.parsing.SyntaxParser;
-import io.github.syst3ms.skriptparser.registration.ExpressionInfo;
-import io.github.syst3ms.skriptparser.registration.SyntaxManager;
 import io.github.syst3ms.skriptparser.registration.tags.Tag;
 import io.github.syst3ms.skriptparser.registration.tags.TagManager;
 import io.github.syst3ms.skriptparser.types.TypeManager;
@@ -22,7 +20,6 @@
 /**
  * A string that possibly contains expressions inside it, meaning that its value may be unknown at parse time
  */
-@SuppressWarnings("ConfusingArgumentToVarargsMethod")
 public class VariableString extends TaggedExpression {
     /**
      * An array containing raw data for this {@link VariableString}.
@@ -268,9 +265,7 @@ public String defaultVariableName() {
                 sb.append(((Tag) o).toString(false));
             } else {
                 assert o instanceof Expression;
-                Optional<? extends ExpressionInfo<? extends Expression<?>, ?>> exprInfo = SyntaxManager.getExpressionExact((Expression<?>) o);
-                assert exprInfo.isPresent();
-                sb.append("<").append(exprInfo.get().getReturnType().toString()).append(">");
+                sb.append("<").append(((Expression<?>) o).getReturnType().toString()).append(">");
             }
         }
         return sb.toString();

diff --git a/src/main/java/io/github/syst3ms/skriptparser/log/SkriptLogger.java b/src/main/java/io/github/syst3ms/skriptparser/log/SkriptLogger.java
@@ -188,7 +188,16 @@ public void error(String message, ErrorType errorType, @Nullable String tip) {
      * @param message the warning message
      */
     public void warn(String message) {
-        log(message, LogType.WARNING, null, null);
+        warn(message, null);
+    }
+
+    /**
+     * Logs a warning message
+     * @param message the warning message
+     * @param tip the tip for solving the warning
+     */
+    public void warn(String message, @Nullable String tip) {
+        log(message, LogType.WARNING, null, tip);
     }
 
     /**
@@ -283,11 +292,4 @@ public boolean isDebug() {
     public String getFileName() {
         return fileName;
     }
-
-    /**
-     * @return whether or not this Logger has an error stored
-     */
-    public boolean hasError() {
-        return hasError;
-    }
 }
diff --git a/src/main/java/io/github/syst3ms/skriptparser/pattern/PatternParser.java b/src/main/java/io/github/syst3ms/skriptparser/pattern/PatternParser.java
@@ -93,7 +93,25 @@ private static Optional<? extends PatternElement> parsePattern(String pattern, S
                 if (matcher.matches() && vertParts.get().length == 1) {
                     var mark = matcher.group(1);
                     optionalGroup = parsePattern(matcher.group(3), logger)
-                            .map(el -> new ChoiceElement(el, mark))
+                            .map(el -> {
+                                if (el instanceof ChoiceGroup && mark.isEmpty())
+                                    // Patterns like '[:(first|second)]' act differently than in the original Skript.
+                                    // Developers probably want to write ':[first|second]' instead, but the two actually
+                                    // have different behavior.
+                                    logger.warn(
+                                            "The parse mark in the '"
+                                                    + s.get()
+                                                    + "' part of the pattern will not be applied to all elements of the '"
+                                                    + matcher.group(3)
+                                                    + "' choice group, because it belongs to the whole optional group: ["
+                                                    + s.get()
+                                                    + "]",
+                                            "If this is unwanted behavior, use the syntax sugar ':["
+                                                    + matcher.group(3).substring(1, matcher.group(3).length() - 1)
+                                                    + "]' to denote optional choice groups"
+                                    );
+                                return new ChoiceElement(el, mark);
+                            })
                             .map(el -> new ChoiceGroup(Collections.singletonList(el)))
                             .map(OptionalGroup::new);
                 } else {

diff --git a/src/main/java/io/github/syst3ms/skriptparser/registration/DefaultRegistration.java b/src/main/java/io/github/syst3ms/skriptparser/registration/DefaultRegistration.java
@@ -327,6 +327,6 @@ public Relation apply(Duration duration, Duration duration2) {
 
         registration.addConverter(SkriptDate.class, Time.class, da -> Optional.of(Time.of(da)));
 
-        registration.register(); // Ignoring logs here, we control the input
+        registration.register(true); // Ignoring logs here, we control the input
     }
 }
diff --git a/src/main/java/io/github/syst3ms/skriptparser/registration/PatternInfos.java b/src/main/java/io/github/syst3ms/skriptparser/registration/PatternInfos.java
@@ -51,8 +51,12 @@ public String[] getPatterns() {
      */
     public String toChoiceGroup() {
         var builder = new StringBuilder("(");
-        for (int i = 0; i < patterns.length; i++)
-            builder.append(i).append(':').append(patterns[i]).append('|');
+        for (int i = 0; i < patterns.length; i++) {
+            builder.append(i).append(':').append(patterns[i]);
+            if (i + 1 < patterns.length) {
+                builder.append("|");
+            }
+        }
         return builder.append(')').toString();
     }
 }
diff --git a/src/main/java/io/github/syst3ms/skriptparser/registration/SkriptRegistration.java b/src/main/java/io/github/syst3ms/skriptparser/registration/SkriptRegistration.java
@@ -535,16 +535,31 @@ public void addTag(Class<? extends Tag> c, int priority) {
 
     /**
      * Adds all currently registered syntaxes to Skript's usable database.
+     * @return all possible errors, warnings and other logs that occurred while parsing the patterns
      */
     public List<LogEntry> register() {
+        return register(false);
+    }
+
+    /**
+     * Adds all currently registered syntaxes to Skript's usable database.
+     * @param ignoreLogs whether to return the logs and close the logger,
+     *                   or just ignore and clear them while keeping the logger open
+     * @return all possible errors, warnings and other logs that occurred while parsing the patterns
+     */
+    public List<LogEntry> register(boolean ignoreLogs) {
         SyntaxManager.register(this);
         ContextValues.register(this);
         TypeManager.register(this);
         TagManager.register(this);
         Converters.registerConverters(this);
         Converters.createMissingConverters();
-        logger.finalizeLogs();
-        return logger.close();
+        if (ignoreLogs) {
+            logger.clearLogs();
+            return new ArrayList<>();
+        } else {
+            return logger.close();
+        }
     }
 
     public interface Registrar {
@@ -662,7 +677,11 @@ protected List<PatternElement> parsePatterns() {
             boolean computePriority = priority == -1;
             priority = computePriority ? 5 : priority;
             return patterns.stream()
-                    .map(s -> PatternParser.parsePattern(s, logger).orElse(null))
+                    .map(s -> {
+                        var result = PatternParser.parsePattern(s, logger).orElse(null);
+                        logger.finalizeLogs();
+                        return result;
+                    })
                     .filter(Objects::nonNull)
                     .peek(e -> {
                         if (computePriority)

diff --git a/src/test/java/io/github/syst3ms/skriptparser/parsing/PatternParserTest.java b/src/test/java/io/github/syst3ms/skriptparser/parsing/PatternParserTest.java
@@ -134,15 +134,20 @@ public void testParsePattern() {
         // Optional with nested group with parse marks
         assertEqualsOptional(
                 new OptionalGroup(
-                        new CompoundElement(
-                                new TextElement("optional "),
-                                new ChoiceGroup(
-                                        new ChoiceElement(new TextElement("first choice"), null),
-                                        new ChoiceElement(new TextElement("second choice"), "second")
+                        new ChoiceGroup(
+                                new ChoiceElement(
+                                        new CompoundElement(
+                                                new TextElement("optional "),
+                                                new ChoiceGroup(
+                                                        new ChoiceElement(new TextElement("first choice"), null),
+                                                        new ChoiceElement(new TextElement("second choice"), "second")
+                                                )
+                                        ),
+                                        "optional"
                                 )
                         )
                 ),
-                parsePattern("[optional (first choice|second:second choice)]", logger)
+                parsePattern("[:optional (first choice|second:second choice)]", logger)
         );
 
         // Regex group