diff --git a/freemarker-core/src/main/java/freemarker/core/SwitchBlock.java b/freemarker-core/src/main/java/freemarker/core/SwitchBlock.java index 5b6131c00..6c5641b86 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 @@ final class SwitchBlock extends TemplateElement { private Case defaultCase; + private boolean usesOnDirective; private final Expression searched; private int firstCaseOrOnIndex; @@ -46,9 +47,6 @@ 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; @@ -56,40 +54,40 @@ void addCase(Case cas) { addChild(cas); } - /** - * @param on an On element. - */ void addOn(On on) { addChild(on); + usesOnDirective = true; } @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 +105,14 @@ TemplateElement[] accept(Environment env) 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", doesn't 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 87256917c..48bb1d6d8 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); } ) - ) + )+ [] ] + end = { - 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 0246688fd..aad6ede4d 100644 --- a/freemarker-core/src/test/java/freemarker/core/BreakAndContinuePlacementTest.java +++ b/freemarker-core/src/test/java/freemarker/core/BreakAndContinuePlacementTest.java @@ -50,22 +50,22 @@ public void testValidPlacements() throws IOException, TemplateException { assertOutput("<#list 1..2 as x><#switch x><#on 1>one<#continue>;", "one;"); assertOutput("<#forEach x in 1..2>${x}<#break>", "1"); assertOutput("<#forEach x in 1..2>${x}<#continue>", "12"); - assertOutput("<#switch 1><#case 1>1<#break>", "1"); - assertOutput("<#switch 1><#default>1<#break>", "1"); + assertOutput("<#switch 1><#case 1>1<#break>unreachable.", "1."); + assertOutput("<#switch 1><#default>1<#break>unreachable.", "1."); } @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>", CONTINUE_NESTING_ERROR_MESSAGE_PART); - assertErrorContains("<#switch 1><#on 1>1<#continue>", CONTINUE_NESTING_ERROR_MESSAGE_PART); - assertErrorContains("<#switch 1><#on 1>1<#break>", 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>", ParseException.class, CONTINUE_NESTING_ERROR_MESSAGE_PART); + assertErrorContains("<#switch 1><#on 1>1<#continue>", ParseException.class, CONTINUE_NESTING_ERROR_MESSAGE_PART); + assertErrorContains("<#switch 1><#on 1>1<#break>", ParseException.class, BREAK_NESTING_ERROR_MESSAGE_PART); assertErrorContains("<#switch 1><#on 1>1<#default><#break>", BREAK_NESTING_ERROR_MESSAGE_PART); - assertErrorContains("<#list 1..2 as x>${x}<#break>", BREAK_NESTING_ERROR_MESSAGE_PART); - assertErrorContains("<#if false><#break>", BREAK_NESTING_ERROR_MESSAGE_PART); - assertErrorContains("<#list xs><#break>", BREAK_NESTING_ERROR_MESSAGE_PART); - assertErrorContains("<#list 1..2 as x>${x}<#else><#break>", BREAK_NESTING_ERROR_MESSAGE_PART); + assertErrorContains("<#list 1..2 as x>${x}<#break>", ParseException.class, BREAK_NESTING_ERROR_MESSAGE_PART); + assertErrorContains("<#if false><#break>", ParseException.class, BREAK_NESTING_ERROR_MESSAGE_PART); + assertErrorContains("<#list xs><#break>", ParseException.class, BREAK_NESTING_ERROR_MESSAGE_PART); + assertErrorContains("<#list 1..2 as x>${x}<#else><#break>", 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 000000000..c4a337875 --- /dev/null +++ b/freemarker-core/src/test/java/freemarker/core/SwitchTest.java @@ -0,0 +1,185 @@ +/* + * 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" : "") + + "]", + 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;", "1D3;D;3;"); + } + + @Test + public void testDefaultOnly() throws TemplateException, IOException { + assertOutput("<#switch 1><#default>D", "D"); + assertOutput("<#list 1..2 as i><#switch 1><#default>D<#break>unreachable", "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" + + " \n" + + "]\n" + + "", + "[\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" : "") + + "]", + 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", + ParseException.class, + "#on after #default"); + assertErrorContains( + "<#switch x><#on 1>On 1<#case 2>On 2", + ParseException.class, + "can't use both #on, and #case", "already had an #on"); + assertErrorContains( + "<#switch x><#case 1>On 1<#on 2>On 2", + 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", + ParseException.class, + "can't use both #on, and #case", "already had an #on"); + assertErrorContains( + "<#switch x><#on 1>On 1<#default>D1<#default>D2", + ParseException.class, + "already had a #default"); + assertErrorContains( + "<#switch x><#case 1>On 1<#default>D1<#default>D2", + ParseException.class, + "already had a #default"); + assertErrorContains( + "<#switch x><#on 1>On 1<#default>D<#on 2>On 2", + ParseException.class, + "#on after #default"); + assertErrorContains( + "<#switch x><#default>D<#on 2>On 2", + 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" + + " \n" + + "]\n" + + "", + "[\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" + + " \n" + + "]\n" + + "", + "[\nC1]\n[\nC2]\n[\nD]\n"); + assertOutput( + "" + + "<#list 1..3 as i>\n" + + "[\n" // <#----> is to avoid unrelated old white-space stripping bug + + " <#switch i>\n" + + " <#on 1>\n" + + " C1\n" + + " <#on 2>\n" + + " C2\n" + + " <#default>\n" + + " D\n" + + " \n" + + "]\n" + + "", + "[\n C1\n]\n[\n C2\n]\n[\n D\n]\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 e9bd37758..f51f44a59 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 @@ <#-- two #default-s are parsing error --> -<@assertFails message="can only have one default"><@"<#switch 1><#case 1><#default><#default>"?interpret /> +<@assertFails message="already had a #default"><@"<#switch 1><#case 1><#default><#default>"?interpret /> diff --git a/freemarker-manual/src/main/docgen/en_US/book.xml b/freemarker-manual/src/main/docgen/en_US/book.xml index 46936674e..f999f39d3 100644 --- a/freemarker-manual/src/main/docgen/en_US/book.xml +++ b/freemarker-manual/src/main/docgen/en_US/book.xml @@ -21199,6 +21199,10 @@ with_args_last: nt + + on + + outputformat @@ -25181,12 +25185,14 @@ or
- switch, case, default, break + switch, on, case, default, break + + @@ -25199,6 +25205,10 @@ or case directive + + on directive + + default directive @@ -25210,13 +25220,32 @@ or
Synopsis - -<#switch value> - <#case refValue1> + Recommended form (on-based), but this is + only supported since FreeMarker + 2.3.34: + + <#switch value> + <#on refValue1> + ... (Handles refValue1) + <#on refValue2, refValue3> + ... (Handles both refValue2 and refValue3)) + ... + <#case refValueN> ... <#break> - <#case refValue2> + <#default> ... +</#switch> + + Deprecated legacy form (case-based): + + <#switch value> + <#case refValue1> + ... (Handles refValue1) + <#break> + <#case refValue2> + <#case refValue3> + ... (Handles both refValue2 and refValue3) <#break> ... <#case refValueN> @@ -25224,9 +25253,7 @@ or <#break> <#default> ... -</#switch> - - +</#switch> Where: @@ -25238,54 +25265,140 @@ or - The break-s and default - are optional. + Additional rules: + + + + A switch block either have + on-s, or case-s, but not + both. + + + + default: Optional, can occur at most + once. Must be after the on-s. There's no + ordering restriction when used with + case-s. + + + + break: + + + + If the switch uses + case then break + immediately exits from the closes enclosing + switch. This is used to prevent + fall-through into the next case, or into + the following (adjacent) default. (To be + precise, break behaves as described if + the switch doesn't contain + on, and thus also if all it contains is a + default.) + + + + If the switch uses + on then break is not + supported by switch directly, but + naturally can be used when the switch is + inside something else that supports break + (like inside a list). + + + + With case, + continue does the same as + break. This is an old bug that works for + backward compatibility, but don't utilize it. With + on, this is fixed. + + + +
Description - The usage of this directive is not recommended, as it's - error-prone because of the fall-through behavior. Use elseif-s - instead unless you want to exploit the fall-through behavior. + + Using this directive with case is not + recommended, as it's error-prone because of the fall-through + behavior. Starting from FreeMarker 2.3.34, use + on instead of case. In + earlier versions use elseif-s + instead. + Switch is used to choose a fragment of template depending on - the value of an expression: + the value of an expression, and is a shorthand instead using if-elseif-else: + + <#switch animal.size> + <#on "small"> + Processed if animal.size was "small" + <#on "medium"> + Processed if animal.size was "medium" + <#on "large", "extra large"> + Processed if animal.size was "large" or "extra large" + <#default> + Processed if animal.size is neither of the above +</#switch> + + Before FreeMarker 2.3.24, on wasn't + supported, only case (note the usage of + break, and the intentional omission of it after + <#case "large">): <#switch animal.size> <#case "small"> - This will be processed if it is small + Processed if animal.size was "small" <#break> <#case "medium"> - This will be processed if it is medium + Processed if animal.size was "medium" <#break> <#case "large"> - This will be processed if it is large + <#case "extra large"> + Processed if animal.size was "large" or "extra large" <#break> <#default> - This will be processed if it is neither + Processed if animal.size is neither of the above </#switch> - Inside the switch must be one or more - <#case value>, - and after all such case tags optionally one - <#default>. When FM reaches the - switch directive, it chooses a - case directive where + Above examples are basically equivalent with this: + + <#assign value = animal.size> +<#if value == "small"> + Processed if animal.size was "small" +<#elseif value == "medium"> + Processed if animal.size was "medium" +<#elseif value == "large" || value == "extra large"> + Processed if animal.size was "large" or "extra large" +<#else> + Processed if animal.size is neither of the above +</#if> + + That is, when the switch directive is + processed, it chooses an on or + case directive where a refValue equals with - value and continues + value, and continues the processing of the template there. If there is no - case directive with appropriate value then it - continues processing at the default directive if - that exists, otherwise it continues the processing after the end-tag - of switch. And now comes the confusing thing: - when it has chosen a case directive, it will - continue the processing there, and will go ahead until it reaches a - break directive. That is, it will not - automatically leave the switch directive when it - reaches another case directive or the - <#default> tag. Example: + on or case directive with + appropriate refValue, + then it continues processing at the default + directive, if that exists, otherwise it continues the processing + after the end-tag of switch. + + Be careful with case's fall-through + behavior! It means that after processing have jumped on the matching + case, it will not leave the + switch directive when it reaches another + case or default, only when it + reaches a break. Example: <#switch x> <#case 1> @@ -25296,12 +25409,14 @@ or d </#switch> - If x is 1, then it will print 1 2 d; if - x is 2 then it will print 2 d; if - x is 3 then it will print d. This is the - mentioned fall-through behavior. The break tag - instructs FM to immediately skip past the switch - end-tag. + If x is 1, then it will + print 1 2 d (actually with more white-space + between); if x is 2 then it + will print 2 d; if x is + 3 then it will print d. This + is usually unintended, or if it is intended then probably not + obvious for the reader, and that's why on is + recommended over case.
@@ -30349,6 +30464,19 @@ TemplateModel x = env.getVariable("x"); // get variable x Changes on the FTL side + + Added #on directive that's now + recommended instead of #case inside + #switch. It has no fall-through, so you need + not, and must not use #break like you did + with #case. Also, #on can + have a list of matching values, so you can use <#on + 1, 2, 3>Was 1-3 instead of <#case + 1><#case 2><#case 3>Was + 1-3<#break> See more here... + + Added new built-ins that allow handling empty or blank strings like the same way as if they were missing values (Java