Skip to content

Commit

Permalink
Refactor 'if' and 'while' to do inversing logic in the transformation…
Browse files Browse the repository at this point in the history
… step. Also, improve on existential checks.
  • Loading branch information
rstacruz committed Jun 10, 2011
1 parent bea11d2 commit c4fc881
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 37 deletions.
88 changes: 52 additions & 36 deletions lib/js2coffee.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ Node = parser.Node

buildCoffee = (str) ->
scriptNode = parser.parse("#{str}\n")
transform(scriptNode)

trim build(scriptNode)

Expand All @@ -49,6 +48,13 @@ Node::left = -> @children[0]
Node::right = -> @children[1]
Node::last = -> @children[@children.length-1]

Node::clone = (hash) ->
for i of @
continue if i in ['tokenizer', 'length', 'filename']
hash[i] ?= @[i]

new Node(@tokenizer, hash)

# `inspect()`
# For debugging
Node::toHash = ->
Expand Down Expand Up @@ -103,10 +109,11 @@ Node::isA = (what...) -> Types[@type] in what
# For instance, for a `function` node, it calls `Builders.function(node)`.
# It defaults to `Builders.other` if it can't find a function for it.
build = (node, opts={}) ->
transform node

name = 'other'
name = node.typeName() if node != undefined and node.typeName

transform node
out = (Builders[name] or Builders.other).apply(node, [opts])

if node.parenthesized then paren(out) else out
Expand Down Expand Up @@ -149,6 +156,7 @@ Types = do ->

# Now extend it with a few more
dict[++last] = 'call_statement'
dict[++last] = 'existential'

dict

Expand Down Expand Up @@ -309,12 +317,10 @@ Builders =
'!==': -> re('binary_operator', @, '!=')

'==': ->
useExistential @, else: =>
re('binary_operator', @, '==') # CHANGEME: Yes, this is wrong
re('binary_operator', @, '==') # CHANGEME: Yes, this is wrong

'!=': ->
useExistential @, else: =>
re('binary_operator', @, '!=') # CHANGEME: Yes, this is wrong
re('binary_operator', @, '!=') # CHANGEME: Yes, this is wrong

'instanceof': -> re('binary_operator', @, 'instanceof')

Expand Down Expand Up @@ -496,7 +502,8 @@ Builders =
'while': ->
c = new Code

c.add inversible(@condition, "while %s", "until %s")
keyword = if @positive then "while" else "until"
c.add "#{keyword} #{build @condition}"
c.scope body(@body)
c

Expand All @@ -512,7 +519,9 @@ Builders =
'if': ->
c = new Code

c.add inversible(@condition, "if %s", "unless %s")
keyword = if @positive then "if" else "unless"

c.add "#{keyword} #{build @condition}"
c.scope body(@thenPart)

if @elsePart?
Expand Down Expand Up @@ -542,6 +551,9 @@ Builders =

c

'existential': ->
"#{build @left()}?"

'array_init': ->
if @children.length == 0
"[]"
Expand Down Expand Up @@ -683,7 +695,38 @@ Transformers =
# *CoffeeScript does not need `break` statements on `switch` blocks.*
delete ch[ch.length-1] if block.last().isA('break')

Transformers.block = Transformers.script
'block': ->
Transformers.script.apply(@)

'if': ->
Transformers.inversible.apply(@)

'while': ->
Transformers.inversible.apply(@)

'inversible': ->
# *Invert a '!='. (`if (x != y)` => `unless x is y`)*
if @condition.isA('!=')
@condition.type = Typenames['==']
@positive = false

# *Invert a '!'. (`if (!x)` => `unless x`)*
else if @condition.isA('!')
@condition = @condition.left()
@positive = false

else
@positive = true

'!=': ->
if @right().isA('null', 'void')

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra Jun 10, 2011

Contributor

You need to be careful here to make sure that the void operator isn't being applied to anything that can have side effects. With the existence of getters and setters, there's a laundry list of benign-looking expressions with side effects. Simple example:

if(a == void fn()) b;

This is currently converted to an existence check (which is okay), but it needs to also preserve the side effects:

if (fn(); a?) then b

You might want to just use that pattern for anything that's not a simple number literal, which will produce nice-looking and safe output 99.99% of the time and just safe output the other 0.01%.

This comment has been minimized.

Copy link
@rstacruz

rstacruz Jun 10, 2011

Author Member

Would this reduction of that rule be sufficient?

  • If it's a == void <anything constant>, compile it as a?.
  • Otherwise, for a == void b, use b; a?.

Something will be considered a constant if it's tree does not have any identifiers or function calls.

This comment has been minimized.

Copy link
@satyr

satyr Jun 10, 2011

if (a == void fn())
if (fn(); a?)

Should also preserve evaluation order, e.g. f() == void g() to [f()?, g()][0].

This comment has been minimized.

Copy link
@rstacruz

rstacruz Jun 10, 2011

Author Member

Just as a point of curiosity, is there an actual practical use for this 0.0001% edge case?

This comment has been minimized.

Copy link
@michaelficarra

michaelficarra Jun 10, 2011

Contributor

an actual practical use for this 0.0001% edge case

Not at all. It's very possible that the compiler would never once see code like this outside of the test cases. But I am in the "do it right" group of programmers rather than the "make it work" group. CoffeeScript development seems to agree, though we do obviously prioritize features that will be more effective. So I'd say it would be worthwhile to make this work properly (at some point), but you do have more important issues to consider right now.

@type = Typenames['!']
@children = [@clone(type: Typenames['existential'], children: [@left()])]

'==': ->
if @right().isA('null', 'void')
@type = Typenames['existential']
@children = [@left()]

# ## Unsupported Error exception

Expand Down Expand Up @@ -781,33 +824,6 @@ unshift = (str) ->
strEscape = (str) ->
JSON.stringify "#{str}"

# `inversible()`
# Returns *reverse* if the given condition is reversible, else returns *normal*.
# Used for *if/unless* and *while/until*.
#
inversible = (condition, normal, reverse) ->
existential = useExistential(condition)

if existential? and existential.substr(0,1) == '!' # Hacky, meh
reverse.replace "%s", existential.substr(1)
else if condition.typeName() == '!'
reverse.replace "%s", build(condition.left())
else
normal.replace "%s", build(condition)

useExistential = (cond, options={}) ->
left = cond.left()
right = cond.right()

if cond.isA('==', '!=')
negative = if (cond.isA('!=', '!==')) then '!' else ''

# **Caveat:** *`x == null` and `x == void` should compile to `x?`.*
if right.isA('null', 'void')
return "#{negative}#{build left}?"

options.else() if options.else?

# `p()`
# Debugging tool. Prints an object to the console.
# Not actually used, but here for convenience.
Expand Down
2 changes: 1 addition & 1 deletion test/features/existential.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ unlessChecks = ->
alert "x != null"
if x != null
alert "x !== null"
if typeof x != "undefined"
unless typeof x == "undefined"
alert "typeof x != 'undefined'"
whileAndFor = ->
while x?
Expand Down

0 comments on commit c4fc881

Please sign in to comment.