This is an automated email from the ASF dual-hosted git repository.
ddekany pushed a commit to branch 2.3-gae
in repository https://gitbox.apache.org/repos/asf/freemarker.git
The following commit(s) were added to refs/heads/2.3-gae by this push:
new ade44352 #switch #on PR post-merge adjustments: - Reorganized how
#case/#on/#default is parsed in JavaCC, mostly to achieve better error messages
(also I guess it's also easier to follow). - Reorganized SwitchBlock to branch
out for the #switch+#case and the #switch+#on logic earlier (less checks on
runtime, also maybe easier to follow). - Adjusted error message wording - Added
much more test cases - Added some of this to the Manual, but much more will
done there
ade44352 is described below
commit ade44352d64f3cd81a2271edb366df90ec126ce5
Author: ddekany <[email protected]>
AuthorDate: Sun Aug 18 18:11:02 2024 +0200
#switch #on PR post-merge adjustments:
- Reorganized how #case/#on/#default is parsed in JavaCC, mostly to achieve
better error messages (also I guess it's also easier to follow).
- Reorganized SwitchBlock to branch out for the #switch+#case and the
#switch+#on logic earlier (less checks on runtime, also maybe easier to follow).
- Adjusted error message wording
- Added much more test cases
- Added some of this to the Manual, but much more will done there
---
.../src/main/java/freemarker/core/SwitchBlock.java | 78 +++++-----
.../src/main/javacc/freemarker/core/FTL.jj | 84 +++++-----
.../core/BreakAndContinuePlacementTest.java | 18 +--
.../src/test/java/freemarker/core/SwitchTest.java | 170 +++++++++++++++++++++
.../test/templatesuite/templates/switch.ftl | 2 +-
freemarker-manual/src/main/docgen/en_US/book.xml | 68 +++++++--
6 files changed, 322 insertions(+), 98 deletions(-)
diff --git a/freemarker-core/src/main/java/freemarker/core/SwitchBlock.java
b/freemarker-core/src/main/java/freemarker/core/SwitchBlock.java
index 5b6131c0..bdcad4d0 100644
--- a/freemarker-core/src/main/java/freemarker/core/SwitchBlock.java
+++ b/freemarker-core/src/main/java/freemarker/core/SwitchBlock.java
@@ -29,6 +29,7 @@ import freemarker.template.TemplateException;
final class SwitchBlock extends TemplateElement {
private Case defaultCase;
+ private boolean usesOnDirective;
private final Expression searched;
private int firstCaseOrOnIndex;
@@ -46,50 +47,52 @@ final class SwitchBlock extends TemplateElement {
firstCaseOrOnIndex = ignoredCnt; // Note that normally
postParseCleanup will overwrite this
}
- /**
- * @param cas a Case element.
- */
void addCase(Case cas) {
if (cas.condition == null) {
defaultCase = cas;
+ } else if (usesOnDirective) {
+ throw new BugException("#on after #case"); //!!T
}
addChild(cas);
}
- /**
- * @param on an On element.
- */
void addOn(On on) {
addChild(on);
+ usesOnDirective = true;
+ if (defaultCase != null) {
+ throw new BugException("#on after #default"); //!!T
+ }
}
@Override
- TemplateElement[] accept(Environment env)
- throws TemplateException, IOException {
- boolean processedCaseOrOn = false;
- boolean usingOn = false;
+ TemplateElement[] accept(Environment env) throws TemplateException,
IOException {
int ln = getChildCount();
- try {
- for (int i = firstCaseOrOnIndex; i < ln; i++) {
+ if (usesOnDirective) {
+ processOnDirectives: for (int i = firstCaseOrOnIndex; i < ln; i++)
{
TemplateElement tel = getChild(i);
- if (tel instanceof On) {
- usingOn = true;
+ // "default" is always the last; the parser ensures this
+ if (tel == defaultCase) {
+ env.visit(defaultCase);
+ break;
+ }
- for (Expression condition : ((On) tel).conditions) {
- boolean processOn = EvalUtil.compare(
- searched,
- EvalUtil.CMP_OP_EQUALS, "on==", condition,
condition, env);
- if (processOn) {
- env.visit(tel);
- processedCaseOrOn = true;
- break;
- }
- }
- if (processedCaseOrOn) {
- break;
+ for (Expression condition : ((On) tel).conditions) {
+ boolean processOn = EvalUtil.compare(
+ searched,
+ EvalUtil.CMP_OP_EQUALS, "on==", condition,
condition, env);
+ if (processOn) {
+ env.visit(tel);
+ break processOnDirectives;
}
- } else { // Case
+ }
+ }
+ } else { // case-s
+ try {
+ boolean processedCaseOrOn = false;
+ for (int i = firstCaseOrOnIndex; i < ln; i++) {
+ TemplateElement tel = getChild(i);
+
Expression condition = ((Case) tel).condition;
boolean processCase = false;
@@ -107,19 +110,14 @@ final class SwitchBlock extends TemplateElement {
processedCaseOrOn = true;
}
}
- }
-
- // If we didn't process any nestedElements, and we have a default,
- // process it.
- if (!processedCaseOrOn && defaultCase != null) {
- env.visit(defaultCase);
- }
- } catch (BreakOrContinueException br) {
- // This catches both break and continue,
- // hence continue is incorrectly treated as a break inside a case.
- // Unless using On, do backwards compatible behavior.
- if (usingOn) {
- throw br; // On supports neither break nor continue.
+ // If we didn't process any nestedElements, and we have a
default,
+ // process it.
+ if (!processedCaseOrOn && defaultCase != null) {
+ env.visit(defaultCase);
+ }
+ } catch (BreakOrContinueException br) {
+ // This catches both break and continue, hence continue is
incorrectly treated as a break inside a case.
+ // ("on", does have this bug.)
}
}
return null;
diff --git a/freemarker-core/src/main/javacc/freemarker/core/FTL.jj
b/freemarker-core/src/main/javacc/freemarker/core/FTL.jj
index 87256917..48bb1d6d 100644
--- a/freemarker-core/src/main/javacc/freemarker/core/FTL.jj
+++ b/freemarker-core/src/main/javacc/freemarker/core/FTL.jj
@@ -3883,11 +3883,13 @@ SwitchBlock Switch() :
{
SwitchBlock switchBlock;
MixedContent ignoredSectionBeforeFirstCase = null;
- Case caseIns;
- On onIns;
+ Case caseOrDefault;
+ On on;
+ boolean hadCase = false;
+ boolean hadDefault = false;
+ boolean hadOn = false;
Expression switchExp;
Token start, end;
- boolean defaultFound = false;
}
{
(
@@ -3897,60 +3899,72 @@ SwitchBlock Switch() :
[ ignoredSectionBeforeFirstCase = WhitespaceAndComments() ]
)
{
- breakableDirectiveNesting++;
+ breakableDirectiveNesting++; // Note that this will be undone when we
find an "on" directive call.
switchBlock = new SwitchBlock(switchExp,
ignoredSectionBeforeFirstCase);
}
+
[
(
- (
- caseIns = Case()
- {
- if (caseIns.condition == null) {
- if (defaultFound) {
+ caseOrDefault = Case()
+ {
+ if (caseOrDefault.condition == null) {
+ if (hadDefault) {
+ throw new ParseException(
+ "You already had a #default in the #switch
block",
+ caseOrDefault);
+ }
+ hadDefault = true;
+ } else {
+ if (hadOn) {
+ if (caseOrDefault.condition != null) {
throw new ParseException(
- "You can only have one default case in a switch
statement", template, start);
+ "You can't use both #on, and #case in a
#switch block, and you already had an #on.",
+ caseOrDefault);
}
- defaultFound = true;
}
- switchBlock.addCase(caseIns);
+ hadCase = true;
}
- )+
+
+ switchBlock.addCase(caseOrDefault);
+ }
|
(
{
- // A Switch with Case supports break, but not one with On.
- // Do it this way to ensure backwards compatibility.
- breakableDirectiveNesting--;
+ if (!hadOn) {
+ // A #switch with #case handles #break specially, but
not #switch with #on.
+ // Also, this must be done before calling On(), as
that consumes the nested #break already.
+ breakableDirectiveNesting--;
+ }
}
- (
- onIns = On()
- {
- switchBlock.addOn(onIns);
+ on = On()
+ {
+ if (hadCase) {
+ throw new ParseException(
+ "You can't use both #on, and #case in a
#switch block, and you already had a #case.",
+ on);
}
- )+
- [
- caseIns = Case()
- {
- // When using on, you can have a default, but not a
normal case
- if (caseIns.condition != null) {
- throw new ParseException(
- "You cannot mix \"case\" and \"on\" in a switch
statement", template, start);
- }
- switchBlock.addCase(caseIns);
+ if (hadDefault) {
+ throw new ParseException(
+ "You can't use #on after #default in a #switch
block; #default must come last.",
+ on);
}
- ]
+ hadOn = true;
- {
- breakableDirectiveNesting++;
+ switchBlock.addOn(on);
}
)
- )
+ )+
[<STATIC_TEXT_WS>]
]
+
end = <END_SWITCH>
{
- breakableDirectiveNesting--;
+ // If we had #on, then this was already decreased there
+ if (!hadOn) {
+ breakableDirectiveNesting--;
+ }
+
switchBlock.setLocation(template, start, end);
return switchBlock;
}
diff --git
a/freemarker-core/src/test/java/freemarker/core/BreakAndContinuePlacementTest.java
b/freemarker-core/src/test/java/freemarker/core/BreakAndContinuePlacementTest.java
index 0246688f..769400cc 100644
---
a/freemarker-core/src/test/java/freemarker/core/BreakAndContinuePlacementTest.java
+++
b/freemarker-core/src/test/java/freemarker/core/BreakAndContinuePlacementTest.java
@@ -56,16 +56,16 @@ public class BreakAndContinuePlacementTest extends
TemplateTest {
@Test
public void testInvalidPlacements() throws IOException, TemplateException {
- assertErrorContains("<#break>", BREAK_NESTING_ERROR_MESSAGE_PART);
- assertErrorContains("<#continue>",
CONTINUE_NESTING_ERROR_MESSAGE_PART);
- assertErrorContains("<#switch 1><#case 1>1<#continue></#switch>",
CONTINUE_NESTING_ERROR_MESSAGE_PART);
- assertErrorContains("<#switch 1><#on 1>1<#continue></#switch>",
CONTINUE_NESTING_ERROR_MESSAGE_PART);
- assertErrorContains("<#switch 1><#on 1>1<#break></#switch>",
BREAK_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#break>", ParseException.class,
BREAK_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#continue>", ParseException.class,
CONTINUE_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#switch 1><#case 1>1<#continue></#switch>",
ParseException.class, CONTINUE_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#switch 1><#on 1>1<#continue></#switch>",
ParseException.class, CONTINUE_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#switch 1><#on 1>1<#break></#switch>",
ParseException.class, BREAK_NESTING_ERROR_MESSAGE_PART);
assertErrorContains("<#switch 1><#on 1>1<#default><#break></#switch>",
BREAK_NESTING_ERROR_MESSAGE_PART);
- assertErrorContains("<#list 1..2 as x>${x}</#list><#break>",
BREAK_NESTING_ERROR_MESSAGE_PART);
- assertErrorContains("<#if false><#break></#if>",
BREAK_NESTING_ERROR_MESSAGE_PART);
- assertErrorContains("<#list xs><#break></#list>",
BREAK_NESTING_ERROR_MESSAGE_PART);
- assertErrorContains("<#list 1..2 as x>${x}<#else><#break></#list>",
BREAK_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#list 1..2 as x>${x}</#list><#break>",
ParseException.class, BREAK_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#if false><#break></#if>", ParseException.class,
BREAK_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#list xs><#break></#list>",
ParseException.class, BREAK_NESTING_ERROR_MESSAGE_PART);
+ assertErrorContains("<#list 1..2 as x>${x}<#else><#break></#list>",
ParseException.class, BREAK_NESTING_ERROR_MESSAGE_PART);
}
@Test
diff --git a/freemarker-core/src/test/java/freemarker/core/SwitchTest.java
b/freemarker-core/src/test/java/freemarker/core/SwitchTest.java
new file mode 100644
index 00000000..adf72388
--- /dev/null
+++ b/freemarker-core/src/test/java/freemarker/core/SwitchTest.java
@@ -0,0 +1,170 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package freemarker.core;
+
+import java.io.IOException;
+
+import org.junit.Test;
+
+import freemarker.template.TemplateException;
+import freemarker.test.TemplateTest;
+
+/**
+ * There are more (older) test in "switch-builtin.ftl", but we prefer creating
new ones in Java.
+ */
+public class SwitchTest extends TemplateTest {
+
+ @Test
+ public void testCaseBasics() throws TemplateException, IOException {
+ testCaseBasics(true);
+ testCaseBasics(false);
+ }
+
+ private void testCaseBasics(boolean hasDefault) throws TemplateException,
IOException {
+ for (int i = 1 ; i <= 6; i++) {
+ assertOutput(
+ "[<#switch " + i + ">\n"
+ + "<#case 3>Case 3<#break>"
+ + "<#case 1>Case 1<#break>"
+ + "<#case 4><#case 5>Case 4 or 5<#break>"
+ + "<#case 2>Case 2<#break>"
+ + (hasDefault ? "<#default>D" : "")
+ + "</#switch>]",
+ i < 6
+ ? "[Case " + (i < 4 ? i : "4 or 5") + "]"
+ : (hasDefault ? "[D]" : "[]"));
+ }
+ }
+
+ /** Tolerated for backward compatibility */
+ @Test
+ public void testCasesWithOddlyPlacedDefault() throws TemplateException,
IOException {
+ assertOutput("<#list 1..3 as i><#switch i><#case 1>1<#default>D<#case
3>3</#switch>;</#list>", "1D3;D;3;");
+ }
+
+ @Test
+ public void testDefaultOnly() throws TemplateException, IOException {
+ assertOutput("<#switch 1><#default>D</#switch>", "D");
+ assertOutput("<#list 1..2 as i><#switch
1><#default>D<#break>unreachable</#switch></#list>", "DD");
+ }
+
+ @Test
+ public void testCaseWhitespace() throws TemplateException, IOException {
+ assertOutput(
+ ""
+ + "<#list 1..3 as i>\n"
+ + "[\n" // <#----> is to avoid unrelated old
white-space stripping bug
+ + " <#switch i>\n"
+ + " <#case 1>C1\n"
+ + " <#case 2>C2<#break>\n"
+ + " <#default>D\n"
+ + " </#switch>\n"
+ + "]\n"
+ + "</#list>",
+ "[\nC1\n C2]\n[\nC2]\n[\nD\n]\n");
+ }
+
+ @Test
+ public void testOn() throws TemplateException, IOException {
+ testOnBasics(true);
+ testOnBasics(false);
+ }
+
+ private void testOnBasics(boolean hasDefault) throws TemplateException,
IOException {
+ for (int i = 1 ; i <= 6; i++) {
+ assertOutput(
+ "[<#switch " + i + ">\n"
+ + "<#on 3>On 3"
+ + "<#on 1>On 1"
+ + "<#on 4, 5>On 4 or 5"
+ + "<#on 2>On 2"
+ + (hasDefault ? "<#default>D" : "")
+ + "</#switch>]",
+ i < 6
+ ? "[On " + (i < 4 ? i : "4 or 5") + "]"
+ : (hasDefault ? "[D]" : "[]"));
+ }
+ }
+
+ @Test
+ public void testOnParsingErrors() throws TemplateException, IOException {
+ assertErrorContains(
+ "<#switch x><#on 1>On 1<#default>D<#on 2>On 2</#switch>",
+ ParseException.class,
+ "#on after #default");
+ assertErrorContains(
+ "<#switch x><#on 1>On 1<#case 2>On 2</#switch>",
+ ParseException.class,
+ "can't use both #on, and #case", "already had an #on");
+ assertErrorContains(
+ "<#switch x><#case 1>On 1<#on 2>On 2</#switch>",
+ ParseException.class,
+ "can't use both #on, and #case", "already had a #case");
+ assertErrorContains(
+ "<#switch x><#on 1>On 1<#default>D<#case 2>On 2</#switch>",
+ ParseException.class,
+ "can't use both #on, and #case", "already had an #on");
+ assertErrorContains(
+ "<#switch x><#on 1>On 1<#default>D1<#default>D2</#switch>",
+ ParseException.class,
+ "already had a #default");
+ assertErrorContains(
+ "<#switch x><#case 1>On 1<#default>D1<#default>D2</#switch>",
+ ParseException.class,
+ "already had a #default");
+ assertErrorContains(
+ "<#switch x><#on 1>On 1<#default>D<#on 2>On 2</#switch>",
+ ParseException.class,
+ "#on after #default");
+ assertErrorContains(
+ "<#switch x><#default>D<#on 2>On 2</#switch>",
+ ParseException.class,
+ "#on after #default");
+ }
+
+ @Test
+ public void testOnWhitespace() throws TemplateException, IOException {
+ assertOutput(
+ ""
+ + "<#list 1..3 as i>\n"
+ + "[\n" // <#----> is to avoid unrelated old
white-space stripping bug
+ + " <#switch i>\n"
+ + " <#on 1>C1\n"
+ + " <#on 2>C2\n"
+ + " <#default>D\n"
+ + " </#switch>\n"
+ + "]\n"
+ + "</#list>",
+ "[\nC1\n ]\n[\nC2\n ]\n[\nD\n]\n");
+ assertOutput(
+ ""
+ + "<#list 1..3 as i>\n"
+ + "[\n" // <#----> is to avoid unrelated old
white-space stripping bug
+ + " <#switch i>\n"
+ + " <#on 1>C1<#t>\n"
+ + " <#on 2>C2<#t>\n"
+ + " <#default>D<#t>\n"
+ + " </#switch>\n"
+ + "]\n"
+ + "</#list>",
+ "[\nC1]\n[\nC2]\n[\nD]\n");
+ }
+
+}
diff --git
a/freemarker-jython25/src/test/resources/freemarker/test/templatesuite/templates/switch.ftl
b/freemarker-jython25/src/test/resources/freemarker/test/templatesuite/templates/switch.ftl
index e9bd3775..f51f44a5 100644
---
a/freemarker-jython25/src/test/resources/freemarker/test/templatesuite/templates/switch.ftl
+++
b/freemarker-jython25/src/test/resources/freemarker/test/templatesuite/templates/switch.ftl
@@ -136,7 +136,7 @@
</#list>
<#-- two #default-s are parsing error -->
-<@assertFails message="can only have one default"><@"<#switch 1><#case
1><#default><#default></#switch>"?interpret /></@>
+<@assertFails message="already had a #default"><@"<#switch 1><#case
1><#default><#default></#switch>"?interpret /></@>
</body>
</html>
diff --git a/freemarker-manual/src/main/docgen/en_US/book.xml
b/freemarker-manual/src/main/docgen/en_US/book.xml
index 3c80856c..12a032f0 100644
--- a/freemarker-manual/src/main/docgen/en_US/book.xml
+++ b/freemarker-manual/src/main/docgen/en_US/book.xml
@@ -25210,13 +25210,32 @@ or
<section>
<title>Synopsis</title>
- <programlisting role="metaTemplate">
-<literal><#switch <replaceable>value</replaceable>>
- <#case <replaceable>refValue1</replaceable>>
+ <para>Recommended form (<literal>on</literal>-based), but this is
+ <emphasis role="bold">only supported since FreeMarker
+ 2.3.34</emphasis>:</para>
+
+ <programlisting role="metaTemplate"><literal><#switch
<replaceable>value</replaceable>>
+ <#on <replaceable>refValue1</replaceable>>
+ <replaceable>... (Handles both refValue1)</replaceable>
+ <#on <replaceable>refValue2, refValue3</replaceable>>
+ <replaceable>... (Handles both refValue2 and refValue3)</replaceable>)
+ <replaceable>...</replaceable>
+ <#case <replaceable>refValueN</replaceable>>
<replaceable>...</replaceable>
<#break>
- <#case <replaceable>refValue2</replaceable>>
+ <#default>
<replaceable>...</replaceable>
+</#switch></literal></programlisting>
+
+ <para>Deprecated legacy form (<literal>case</literal>-based):</para>
+
+ <programlisting role="metaTemplate"><literal><#switch
<replaceable>value</replaceable>>
+ <#case <replaceable>refValue1</replaceable>>
+ <replaceable>... (Handles both refValue1)</replaceable>
+ <#break>
+ <#case <replaceable>refValue2</replaceable>>
+ <#case <replaceable>refValue3</replaceable>>
+ <replaceable>... (Handles both refValue2 and refValue3)</replaceable>
<#break>
<replaceable>...</replaceable>
<#case <replaceable>refValueN</replaceable>>
@@ -25224,9 +25243,7 @@ or
<#break>
<#default>
<replaceable>...</replaceable>
-</#switch>
-</literal>
-</programlisting>
+</#switch></literal></programlisting>
<para>Where:</para>
@@ -25238,17 +25255,29 @@ or
</listitem>
</itemizedlist>
- <para>The <literal>break</literal>-s and <literal>default</literal>
- are optional.</para>
+ <para><literal>default</literal> is optional.
+ <literal>break</literal> is used with <literal>case</literal> to
+ prevent fall-through into the next <literal>case</literal>. When
+ <literal>on</literal> is used, <literal>break</literal> is not
+ allowed, except if the <literal>switch</literal> is inside something
+ that supports <literal>break</literal>.</para>
</section>
<section>
<title>Description</title>
- <para>The usage of this directive is not recommended, as it's
- error-prone because of the fall-through behavior. Use <link
- linkend="ref.directive.elseif"><literal>elseif</literal></link>-s
- instead unless you want to exploit the fall-through behavior.</para>
+ <note>
+ <para>Using this directive with <literal>case</literal> is not
+ recommended, as it's error-prone because of the fall-through
+ behavior. Starting from FreeMarker 2.3.34, use
+ <literal>on</literal> instead of <literal>case</literal>. In
+ earlier versions use <link
+ linkend="ref.directive.elseif"><literal>elseif</literal></link>-s
+ instead. </para>
+ </note>
+
+ <para>[TODO: Continue updating this for <literal>on</literal>
+ instead of <literal>case</literal>]</para>
<para>Switch is used to choose a fragment of template depending on
the value of an expression:</para>
@@ -30349,6 +30378,19 @@ TemplateModel x = env.getVariable("x"); // get
variable x</programlisting>
<title>Changes on the FTL side</title>
<itemizedlist>
+ <listitem>
+ <para>Added <literal>#on</literal> directive that's now
+ recommended instead of <literal>#case</literal> inside
+ <literal>#switch</literal>. It has no fall-through, so you need
+ not, and must not use <literal>#break</literal> like you did
+ with <literal>#case</literal>. Also, <literal>#on</literal> can
+ have a list of matching values, so you can use <literal><#on
+ 1, 2, 3>Was 1-3</literal> instead of <literal><#case
+ 1><#case 2><#case 3>Was
+ 1-3<#break></literal> See more <link
+ linkend="ref.directive.switch">here...</link></para>
+ </listitem>
+
<listitem>
<para>Added new built-ins that allow handling empty or blank
strings like the same way as if they were missing values (Java
