Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for brackets in boolean expressions #165

Merged
merged 12 commits into from
Sep 21, 2018
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- Added support for resolving superclass properties for not-NSObject subclasses
- The `{% for %}` tag can now iterate over tuples, structures and classes via
their stored properties.
- You can now use brackets in boolean expressions to change operators precedence
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the entry format to match the rest? (., 2 spaces, credit yourself and ref. this PR)


### Bug Fixes

Expand Down
86 changes: 72 additions & 14 deletions Sources/IfTag.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,11 @@ func findOperator(name: String) -> Operator? {
}


enum IfToken {
indirect enum IfToken {
case infix(name: String, bindingPower: Int, op: InfixOperator.Type)
case prefix(name: String, bindingPower: Int, op: PrefixOperator.Type)
case variable(Resolvable)
case subExpression(Expression)
case end

var bindingPower: Int {
Expand All @@ -52,6 +53,8 @@ enum IfToken {
return bindingPower
case .variable(_):
return 0
case .subExpression(_):
return 0
case .end:
return 0
}
Expand All @@ -66,6 +69,8 @@ enum IfToken {
return op.init(expression: expression)
case .variable(let variable):
return VariableExpression(variable: variable)
case .subExpression(let expression):
return expression
case .end:
throw TemplateSyntaxError("'if' expression error: end")
}
Expand All @@ -80,6 +85,8 @@ enum IfToken {
throw TemplateSyntaxError("'if' expression error: prefix operator '\(name)' was called with a left hand side")
case .variable(let variable):
throw TemplateSyntaxError("'if' expression error: variable '\(variable)' was called with a left hand side")
case .subExpression(_):
throw TemplateSyntaxError("'if' expression error: sub expression was called with a left hand side")
case .end:
throw TemplateSyntaxError("'if' expression error: end")
}
Expand All @@ -100,21 +107,74 @@ final class IfExpressionParser {
let tokens: [IfToken]
var position: Int = 0

init(components: [String], tokenParser: TokenParser) throws {
self.tokens = try components.map { component in
if let op = findOperator(name: component) {
switch op {
case .infix(let name, let bindingPower, let cls):
return .infix(name: name, bindingPower: bindingPower, op: cls)
case .prefix(let name, let bindingPower, let cls):
return .prefix(name: name, bindingPower: bindingPower, op: cls)
private init(tokens: [IfToken]) {
self.tokens = tokens
}

static func parser(components: [String], tokenParser: TokenParser) throws -> IfExpressionParser {
try IfExpressionParser.validateBracketsBalance(components: components)
return try IfExpressionParser(components: components, tokenParser: tokenParser)
}

static func validateBracketsBalance(components: [String]) throws {
guard try components.reduce(0, {
var bracketsBalance = $0
if $1 == "(" { bracketsBalance += 1 }
else if $1 == ")" { bracketsBalance -= 1 }
if bracketsBalance < 0 { throw TemplateSyntaxError("unbalanced brackets") }
return bracketsBalance
}) == 0 else {
throw TemplateSyntaxError("unbalanced brackets")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we give a more explicit error, like "unbalanced brackets: missing closing bracket" here and "unbalanced brackets: too many closing brackets" above when <0?

}
}

private init(components: [String], tokenParser: TokenParser) throws {
var parsedComponents = [String]()
self.tokens = try components.enumerated().flatMap { index, component in
if component == "(" {
let (expression, parsed) = try IfExpressionParser.subExpression(
from: index + 1,
components: components,
tokenParser: tokenParser
)
parsedComponents.append(contentsOf: parsed)
return .subExpression(expression)
}
else if component == ")" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this on the same line as the }?

parsedComponents.append(component)
return nil
} else if index >= parsedComponents.count {
parsedComponents.append(component)
if let op = findOperator(name: component) {
switch op {
case .infix(let name, let bindingPower, let cls):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to look up what the cls value was in the Operator enum. Maybe ..., let operator woud be clearer?

return .infix(name: name, bindingPower: bindingPower, op: cls)
case .prefix(let name, let bindingPower, let cls):
return .prefix(name: name, bindingPower: bindingPower, op: cls)
}
}
return .variable(try tokenParser.compileFilter(component))
} else {
return nil
}

return .variable(try tokenParser.compileFilter(component))
}
}

static func subExpression(from index: Int, components: [String], tokenParser: TokenParser) throws -> (Expression, [String]) {
Copy link
Collaborator

@AliSoftware AliSoftware May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better performance and memory-wise to use an ArraySlice here for components, to prevent rebuilding a new sub-array from the full array on every recursion level? After all, that's exactly what ArraySlices are designed for 😉

That would allow you to get rid of Array(…) wrapping 2 lines below too.
Of course you'd need to replace your components.enumerated() on line 133 by zip(components, components.indices) instead, which is generally better anyway (see Soroush's article about enumerated) because not all startIndices are 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't say anything about performance, but the only thing it will really change is move suffix to caller site, because IfExpressionParser accepts Array. But it allows to remove one function parameter, so I'll make the change still.

Speaking of enumeration I don't see what zip will improve in this particular case. I find enumerated use to be more clear here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about readability but about correctness (see Soroush's article), especially if we move to use slices (because they're the type to use when you recurse on subexpression, as they avoid a ton of memory reallocation, and that's exactly what we're doing here so that's a textbook use case) and enumerated will lead to wrong indexes with slices unlike zip (as explained by Soroush)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if this type only accepts ArraySlice in constructor it makes sense 👍

var bracketsBalance = 1
let subComponents = Array(components
.suffix(from: index)
.prefix(while: {
if $0 == "(" { bracketsBalance += 1 }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of having the content of the if on the same line, and pretty sure swiftlint'll complain about this too.

else if $0 == ")" { bracketsBalance -= 1 }
return bracketsBalance != 0
}))

Copy link
Collaborator

@AliSoftware AliSoftware May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure it would be worth it, but what about a small class Balancer helper for every time we have to balance parentheses and/or brackets? Could serve here, in the code above too, and in the other PR from @djbe about indirect references…

class Balancer {
  var balance = 0
  let opening: Character
  let closing: Character
  init(pair: (Character, Character), initialBalance: Int = 0) {
    opening = pair.0
    closing = pair.1
    balance = initialBalance
  }
  // convenience constructors
  static func brackets(initial: Int = 0) -> Balancer {
    return Balancer(pair: ("[", "]"), initialBalance: initial)
  }
  // And same for parentheses

  func parse(_ char: Character) throws -> Int {
    // … parse the char, increment/decrement the balance if it matches the opening/closing chars, throws if balance < 0, return new balance
  }
}

For example here you could then create a balancer then use components.prefix(while: { balancer.parse($0) != 0)

(all that code typed from my phone so no guarantee of being correct or typo-free)

I'll let you discuss if it's worth adding it? And if so adapt the code (what I typed is just a draft, maybe you'll need to adjust it to fit all situations where it could be used) but could be nice for various places where we do that kind of logic, in this PR and others

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea but I don't think it will be useful in #215 as validation there is done during parsing, not as a separate step. We can do the same here and remove validation step.

let expressionParser = try IfExpressionParser(components: subComponents, tokenParser: tokenParser)
let expression = try expressionParser.parse()
return (expression, ["("] + subComponents + [")"])
}

var currentToken: IfToken {
if tokens.count > position {
return tokens[position]
Expand Down Expand Up @@ -154,13 +214,11 @@ final class IfExpressionParser {
}
}


func parseExpression(components: [String], tokenParser: TokenParser) throws -> Expression {
let parser = try IfExpressionParser(components: components, tokenParser: tokenParser)
let parser = try IfExpressionParser.parser(components: components, tokenParser: tokenParser)
return try parser.parse()
}


/// Represents an if condition and the associated nodes when the condition
/// evaluates
final class IfCondition {
Expand Down
29 changes: 29 additions & 0 deletions Tests/StencilTests/ExpressionSpec.swift
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,35 @@ func testExpressions() {
try expect(expression.evaluate(context: Context(dictionary: ["lhs": "a", "rhs": "bcd"]))).to.beFalse()
}
}

$0.describe("sub expression") {
$0.it("evaluates correctly") {
let context = Context(dictionary: ["one": false, "two": false, "three": true])

let expression = try! parseExpression(components: ["one", "and", "two", "or", "three"], tokenParser: parser)
let expressionWithBrackets = try! parseExpression(components: ["one", "and", "(", "two", "or", "three", ")"], tokenParser: parser)

try expect(expression.evaluate(context: context)).to.beTrue()
try expect(expressionWithBrackets.evaluate(context: context)).to.beFalse()

let notExpression = try! parseExpression(components: ["not", "one", "or", "three"], tokenParser: parser)
let notExpressionWithBrackets = try! parseExpression(components: ["not", "(", "one", "or", "three", ")"], tokenParser: parser)

try expect(notExpression.evaluate(context: context)).to.beTrue()
try expect(notExpressionWithBrackets.evaluate(context: context)).to.beFalse()
}

$0.it("fails when brackets are not balanced") {
try expect(parseExpression(components: ["(", "lhs", "and", "rhs"], tokenParser: parser)).toThrow()
try expect(parseExpression(components: [")", "lhs", "and", "rhs"], tokenParser: parser)).toThrow()
try expect(parseExpression(components: ["(", "lhs", "and", "rhs", ")", "("], tokenParser: parser)).toThrow()
try expect(parseExpression(components: ["(", "lhs", "and", "rhs", ")", ")"], tokenParser: parser)).toThrow()
try expect(parseExpression(components: ["(", "lhs", "and", "rhs", ")", ")"], tokenParser: parser)).toThrow()
try expect(parseExpression(components: ["(", "lhs", "and", ")"], tokenParser: parser)).toThrow()
try expect(parseExpression(components: ["(", "and", "rhs", ")"], tokenParser: parser)).toThrow()
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unneeded empty new line

}
}
}