Fix #4544: postfix conditional on first line of implicit object #4554

helixbass · 2017-05-24T19:40:14Z

Fixes #4544. Also includes some little rewriter.coffee cleanups

The reason for the behavior @couchand describes in #4544 is that postfix ifs are currently treated as implicit-object-literal-closing if they're on the first line of the object literal so that things like x = a: b if c work correctly. But it's assuming that it's a single-line object literal, so if the expected compilation described by @couchand is desired (which I think it is, ie you should be able to swap two lines of an object literal without anything weird happening) then this fixes it by sniffing ahead to see if we appear to be on the first line of a multi-line object literal

Added the examples given by @couchand as well as a few more cases to test/objects.coffee

@GeoffreyBooth I figured you'd want this targeted against 2?

@GeoffreyBooth @lydell let me know if I should justify the correctness of any/all of the little cleanup refactorings

GeoffreyBooth · 2017-05-24T19:59:24Z

2 is great, thanks. This looks quite good. Yes thanks for the cleanups, rewriter.coffee is a beast and can use all the cleanup it can get 😄

Does this fix any of the other lexer or related open issues? Thanks for tackling this!

lydell · 2017-05-24T20:08:44Z

Wow! Just glancing over the diff I have no idea what's going on, but it all looks sensible. Good job! (I guess 😉 )

GeoffreyBooth · 2017-05-24T23:00:32Z

Yes, @lydell echoes my thoughts exactly ;)

Just merge this?

helixbass · 2017-05-25T01:53:59Z

@GeoffreyBooth yup figured this was a good excuse to jump into the belly of the beast and understand how addImplicitBracesAndParens() works. Took a brief look through open issues, this also closes #2051. Thanks for organizing and tagging them "Lexer:", I'll start going through those more closely as the lexer is what I'm most familiar with at this point. On a somewhat related note, I'd like to start following all activity in the Coffeescript repo (and perhaps coffeescript6/discuss), is Github notification emails the best way to get notified of stuff that happens? I've found the Slack Github integration feed easy to browse/scroll but looks like you need to own the repos you want to follow

@lydell @GeoffreyBooth I'll go through now and leave specific comments in case you do want to vet anything more closely but I appreciate your trust that I'm not doing anything nonsensical

GeoffreyBooth · 2017-05-25T02:39:04Z

@helixbass thanks! Yes, that was the idea behind the tagging. You might also want to skim through the “Invalid JS”-tagged ones, they might be related to the lexer/rewriter stuff.

I don’t know how to get better notifications. I watch both repos but I only get emails when I’m mentioned or I’ve already participated in a thread. If you find a better way please let me know. On a related note, the plan is to merge discuss into this repo when 2.0.0 is released.

I’ve never worked in the rewriter, so you know it far better than I do. Reading your code there’s nothing sloppy and nothing obviously wrong, so if you say it fixes the bug and doesn’t break any tests, I’m kind of taking your word for it 😄

helixbass · 2017-05-25T02:19:00Z

src/lexer.coffee

@@ -559,7 +560,9 @@ exports.Lexer = class Lexer
          else if tok[0] is '('
            tok[0] = 'PARAM_START'
            return this
-          else return this
+          else
+            paramEndToken[0] = 'CALL_END'


clean up after ourselves here rather than force the rewriter to (non-obviously) adjust a mismatched CALL_START/PARAM_END pair into CALL_START/CALL_END (in closeOpenCalls()). This is for cases like f() -> where here in tagParameters() we start trying to back-label the parens as PARAM_START/PARAM_END but realize we're not actually looking at params but rather an immediately preceding call

helixbass · 2017-05-25T02:34:02Z

src/rewriter.coffee

      if token[0] in EXPRESSION_START
        levels += 1
      else if token[0] in EXPRESSION_END
        levels -= 1
+      if levels < 0


this is combining a cleanup of the existing logic with the addition of a new option

Cleanup: you can see that the if not token in the line removed above could never be true, so you can move that line down here (simplifying the condition to just if levels < 0) and not have to do the awkward i - 1 call at the beginning of the next iteration

New option: then just additionally check for the new returnOnNegativeLevel option param, which allows you to override detectEnd()'s default behavior which is to also perform the specified action if levels < 0 (ie we hit an EXPRESSION_END we didn't see the start of)

helixbass · 2017-05-25T02:42:28Z

src/rewriter.coffee

  closeOpenCalls: ->
    condition = (token, i) ->
-      token[0] in [')', 'CALL_END'] or
-      token[0] is 'OUTDENT' and @tag(i - 1) is ')'
+      token[0] in [')', 'CALL_END']


this "mis-nested outdent" case seems to predate the behavior of eg pair() in the lexer, which prevents mis-nested outdents from making it this far, so we can safely get rid of it. Also, as mentioned in the above lexer.coffee comment, this method (closeOpenCalls()) also now doesn't have to clean up mismatched CALL_START/PARAM_END pairs, which is nice b/c the way it was cleaning them up was totally non-obvious (relied on the detectEnd() behavior described above of calling the action when levels < 0, in this case when it hit the incorrectly labeled PARAM_END)

helixbass · 2017-05-25T02:45:17Z

src/rewriter.coffee

@@ -159,31 +159,36 @@ exports.Rewriter = class Rewriter
      # class declaration or if-conditionals)
      inImplicitControl = -> inImplicit() and stackTop()?[0] is 'CONTROL'

-      startImplicitCall = (j) ->
-        idx = j ? i
+      startImplicitCall = (idx) ->


can safely remove all this j?-related logic here and in startImplicitObject() b/c all calls of these methods will clearly pass a numeric first argument

helixbass · 2017-05-25T03:03:44Z

src/rewriter.coffee

@@ -197,7 +202,7 @@ exports.Rewriter = class Rewriter
        #  1. We have seen a `CONTROL` argument on the line.
        #  2. The last token before the indent is part of the list below
        #
-        if prevTag not in ['=>', '->', '[', '(', ',', '{', 'TRY', 'ELSE', '=']
+        if prevTag not in ['=>', '->', '[', '(', ',', '{', 'ELSE', '=']


safe to remove TRY from this list b/c an implicit call could never be at the top of the stack (ie inImplicitCall() could never be true) when we've just seen TRY b/c the preceding clause would've caused a CONTROL to get pushed onto the stack (in the previous scanTokens() iteration). Effectively, this list can safely be mutually exclusive from the list above (['IF', 'TRY', ...])

helixbass · 2017-05-25T03:06:30Z

src/rewriter.coffee

@@ -236,13 +241,6 @@ exports.Rewriter = class Rewriter
      #       a: b
      #       c: d
      #
-      # and


this comment seems to reference an old syntax that no longer compiles (now the first arg has to be an implicit object in order for indentation to imply an implicit call?)

helixbass · 2017-05-25T03:10:06Z

src/rewriter.coffee

@@ -316,7 +314,8 @@ exports.Rewriter = class Rewriter
          # Close implicit objects such as:
          # return a: 1, b: 2 unless true
          else if inImplicitObject() and not @insideForDeclaration and sameLine and
-                  tag isnt 'TERMINATOR' and prevTag isnt ':'
+                  tag isnt 'TERMINATOR' and prevTag isnt ':' and
+                  not (tag is 'POST_IF' and startsLine and implicitObjectContinues(i + 1))


the actual fix. If we see a POST_IF on the first line of an implicit object, check if it looks like that implicit object is actually multi-line rather than assuming it's an inline-implicit-object-ending POST_IF

helixbass · 2017-05-25T03:16:48Z

src/rewriter.coffee


      endImplicitObject = (j) ->
        j = j ? i
        stack.pop()
        tokens.splice j, 0, generate '}', '}', token
        i += 1

+      implicitObjectContinues = (j) =>


looks ahead (from the POST_IF in question) for a following line that should be treated as part of the same implicit object (ie is at the same indentation level, is the next line (after any properly balanced intervening nested stuff), and is itself looking like an object literal)

helixbass · 2017-05-25T03:19:35Z

test/objects.coffee

@@ -439,6 +439,12 @@ test "#3216: For loop declaration as a value of an implicit object", ->
  arrayEq ob.b, test
  arrayEq ob.c, test
  arrayEq ob.d, test
+  byFirstKey =


added a couple tests since the @insideForDeclaration/#3216 stuff really only kicks in on the first line of an implicit object

lydell · 2017-05-25T08:57:20Z

Well, I’m convinced! I’d say let’s just merge it!

vendethiel · 2017-05-25T09:44:42Z

src/rewriter.coffee


      endImplicitObject = (j) ->
        j = j ? i
        stack.pop()
        tokens.splice j, 0, generate '}', '}', token
        i += 1

+      implicitObjectContinues = (j) =>
+        nextTerminatorIdx = null
+        @detectEnd j,


How costly is that lookahead here? It's called every single time we rewrite an implicit object literal, right? Is there a perf. impact?

@vendethiel nope only gets called when it sees a POST_IF on the first line of an implicit object literal

Thank you for the explanation.

helixbass added 2 commits May 24, 2017 13:21

support postfix conditional on first line of implicit object

b1c1ce5

cleanup cleanup

28b292e

helixbass mentioned this pull request May 24, 2017

Lexer: Postfix conditionals in object literals #4544

Closed

also fixes jashkenas#2051, added test

bb2009a

helixbass commented May 25, 2017

View reviewed changes

vendethiel reviewed May 25, 2017

View reviewed changes

GeoffreyBooth merged commit 63b109a into jashkenas:2 May 30, 2017

vendethiel mentioned this pull request Jun 13, 2017

Incompatible code generated with 1.12.6 using function call followed by if statement. #4568

Closed

helixbass mentioned this pull request Jun 17, 2017

Lexer: postfix for in first line of implicit object #4579

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4544: postfix conditional on first line of implicit object #4554

Fix #4544: postfix conditional on first line of implicit object #4554

helixbass commented May 24, 2017

GeoffreyBooth commented May 24, 2017

lydell commented May 24, 2017

GeoffreyBooth commented May 24, 2017

helixbass commented May 25, 2017

GeoffreyBooth commented May 25, 2017

helixbass May 25, 2017

helixbass May 25, 2017

helixbass May 25, 2017

helixbass May 25, 2017

helixbass May 25, 2017

helixbass May 25, 2017

helixbass May 25, 2017

helixbass May 25, 2017

helixbass May 25, 2017

lydell commented May 25, 2017

vendethiel May 25, 2017

helixbass May 25, 2017

vendethiel May 25, 2017

@@ @@ -236,13 +241,6 @@ exports.Rewriter = class Rewriter @@
                     #       a: b
                     #       c: d
                     #
-                    # and

Fix #4544: postfix conditional on first line of implicit object #4554

Fix #4544: postfix conditional on first line of implicit object #4554

Conversation

helixbass commented May 24, 2017

GeoffreyBooth commented May 24, 2017

lydell commented May 24, 2017

GeoffreyBooth commented May 24, 2017

helixbass commented May 25, 2017

GeoffreyBooth commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lydell commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment