Skip to content

Commit

Permalink
#switch #on PR post-merge adjustments:
Browse files Browse the repository at this point in the history 
- 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
- Manual updates for #on, and added version history entry
  • Loading branch information
@ddekany
ddekany committed Aug 28, 2024
1 parent 7a429fe commit 7d9f5e5
Show file tree
Hide file tree
Showing 6 changed files with 449 additions and 129 deletions.
73 changes: 33 additions & 40 deletions freemarker-core/src/main/java/freemarker/core/SwitchBlock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
final class SwitchBlock extends TemplateElement {

private Case defaultCase;
private boolean usesOnDirective;
private final Expression searched;
private int firstCaseOrOnIndex;

Expand All @@ -46,50 +47,47 @@ 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;
}
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;

Expand All @@ -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;
Expand Down
84 changes: 49 additions & 35 deletions freemarker-core/src/main/javacc/freemarker/core/FTL.jj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
{
(
Expand All @@ -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;
}
Expand Down
22 changes: 11 additions & 11 deletions freemarker-core/src/test/java/freemarker/core/BreakAndContinuePlacementTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,22 @@ public void testValidPlacements() throws IOException, TemplateException {
assertOutput("<#list 1..2 as x><#switch x><#on 1>one<#continue></#switch>;</#list>", "one;");
assertOutput("<#forEach x in 1..2>${x}<#break></#forEach>", "1");
assertOutput("<#forEach x in 1..2>${x}<#continue></#forEach>", "12");
assertOutput("<#switch 1><#case 1>1<#break></#switch>", "1");
assertOutput("<#switch 1><#default>1<#break></#switch>", "1");
assertOutput("<#switch 1><#case 1>1<#break>unreachable</#switch>.", "1.");
assertOutput("<#switch 1><#default>1<#break>unreachable</#switch>.", "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></#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
Expand Down
Loading

0 comments on commit 7d9f5e5

Please sign in to comment.