Indenting comments in template strings hangs

[AdamNiederer]

Indenting the last line of this example with tab causes an infinite loop.

`
//`
 //

[lddubeau]

I ran into an issue tangential to this one. The problem seems to be that the code of typescript--re-search-backward-inner does not take into account the possibility that a string could span multiple lines. I've tried this, and got the same behavior:

"\
//"
 //

The function tries to skip over strings and when the string is multi-line, instead of skipping, it keeps searching for the string delimiter and finds the end delimiter over and over and over and over...

[josteink]

Good analysis. Armed with that knowledge it hopefully shouldn't be too hard to fix.

I'll see if I have time to look into this some time the next few days, but I can't make any promises.

[josteink]

I see what you mean. The problem are with these three lines here:

https://github.com/ananthakumaran/typescript.el/blob/master/typescript-mode.el#L708-L710

They keep on looping forever, because the re-search-backward is bound to the current line only. This is a pretty generic function, involving way more low-level parse-things than I'm familiar or comfortable with, so I'm not entirely sure what the obvious way to exit this loop is.

My best guess would be that we should dig deeper into the parse stuff, and step back further only when we detect that the first char of the line has a syntax-property which belonging to a string-literal. Basically create a typescript--beginning-of-line-no-string and replace beginning-of-line with that instead. Does that sound correct? To me that sounds like a reasonable approach.

Or is that too complex? A simpler solution (although maybe it be overly simplistic) would be to assume that the re-search-backward is bound to the same line for performance reasons? If so, we could try to expand it. That has the hallmarks of a more hacky approach though, and I'd rather not do stuff like that unless I'm forced to.

Opinions?

[lddubeau]

typescript--re-search-backward-inner already uses (syntax-ppss) which at (nth 8 ...) has the address of the start of the string. Actually, I see just now that's what the function uses for skipping over comments. The 8th field is the start of the string or comment. I don't see why the code does not also use it for skipping over strings. If there's some gotcha, I'm not aware of it.

[josteink]

I'm not sure what other consequences it can have besides performance, but I changed this expression:

             (re-search-backward
              (concat "\\([^\\]\\|^\\)" (string str-terminator))
              (save-excursion (beginning-of-line) (point)) t))

to this:

             (re-search-backward
              (concat "\\([^\\]\\|^\\)" (string str-terminator))
              nil t))

Basically just removed the re-search-backward current-line BOUND constraint and now it seems to work fine. The test-suite runs fine too.

If nobody else can find anything wrong with that... I guess we might have a solution?

[lddubeau]

@josteink The question I raised in my previous comment remains. Why not use the information provided by (syntax-ppss)? It should already contain position of the start of the string. Why recalculate it with a search? Is there some limitation of (syntax-ppss) I'm not aware of?

---

Tangential issue I just noticed. The backtick templates allowed in ES6 and TS allow for strings to nest inside strings. Like for instance

`a${`b`}c`

I've known for a while that this confuses the highlighter. I figured it would also confuse the re-search-backward code we've been discussing. I've discovered it also confuses (syntax-ppss). :(

[josteink]

The question I raised in my previous comment remains. Why not use the information provided by (syntax-ppss)? It should already contain position of the start of the string. Why recalculate it with a search? Is there some limitation of (syntax-ppss) I'm not aware of?

Not knowing this code intimitely, and considering it is 100% undocumented at this point, I'm going to assume this may be a the result of lacking knowledge from the original developer, who may have just copy-pasted some of these bits together.

(Like I most likely would have been happy to here)

I just tried with the following instead, like you suggested:

      (setq parse (syntax-ppss))
      (cond ((nth 3 parse)
             (goto-char (nth 8 parse)))
            ((nth 7 parse)

As far as I can see that works just as well. I say unless we can find any compelling reason to suspect otherwise, we should just move ahead assuming lack of syntax-ppss-knowledge was behind this particular construction.

---

Tangential issue I just noticed. The backtick templates allowed in ES6 and TS allow for strings to nest inside strings. ... I figured it would also confuse the re-search-backward code we've been discussing. I've discovered it also confuses (syntax-ppss). :(

Bummer. But I guess that should be digged into in another issue.

[lddubeau]

I think typescript--re-search-backward-inner could be reduced to:

(defun typescript--re-search-backward-inner (regexp &optional bound count)
  "Auxiliary function for `typescript--re-search-backward'."
  (let ((parse)
        (orig-macro-start
         (save-excursion
           (and (typescript--beginning-of-macro)
                (point)))))
    (while (> count 0)
      (re-search-backward regexp bound)
      (when (and (> (point) (point-min))
                 (save-excursion (backward-char) (looking-at "/[/*]")))
        (forward-char))
      (setq parse (syntax-ppss))
      (cond
       ;; If we are in a comment or a string, jump back to the start
       ;; of the comment or string.
       ((nth 8 parse)
        (goto-char (nth 8 parse)))
       ((and (eq (char-before) ?/) (eq (char-after) ?*))
        (re-search-backward "/\\*"))
       ((and (not (and orig-macro-start
                       (>= (point) orig-macro-start)))
             (typescript--beginning-of-macro)))
       (t
        (setq count (1- count))))))
  (point))

The test suite passes with the above. The changes are:

1. Treat strings the same as comments. We just need to test whether (nth 8 parse) is set and act on that.

2. The (nth 4 parse) condition that used to be there has been subsumed under the general case for comments. I'm not seeing why nested comments should need special handling. It is possible I'm mistaken though. I've kept the (and (eq (char-before) ?/) (eq (char-after) ?*)) test because that looks like an edge-case, but it is still possible to that it is already subsumed by the previous code.

The test suite passes with the code above. Indentation uses this code but I'm thinking whoever produces the final fix should also add unit tests for typescript--re-search-backward-inner to try to trip it. And if (nth 8 parse) is not enough to handle all cases, we should document the special handling we add to take care of special cases. Our current puzzlement at the logic is evidence that comments would be desirable.

[josteink]

We just need to test whether (nth 8 parse) is set and act on that.

Good point. I have no issues with that.

The (nth 4 parse) condition that used to be there has been subsumed under the general case for comments. I'm not seeing why nested comments should need special handling.

Agreed. And looking at the existing code, it looks like if inside nested comments, we'd just skip out to the level outside, before skipping further.

As long as the test-suite runs, I absolutely support reducing this function to its bare minimum, to avoid confusing code like this in the future.

I'm thinking whoever produces the final fix should also add unit tests for typescript--re-search-backward-inner to try to trip it.

I was thinking about writing a higher level "integration"-test for this (for this specific crash/indentation-error), but I guess that would be an useful thing to do as well.

if (nth 8 parse) is not enough to handle all cases, we should document the special handling we add to take care of special cases. Our current puzzlement at the logic is evidence that comments would be desirable.

Agreed. But so far I can't see how that is in any way insufficient.

Are you suggesting we just produce the fix, release it, and if someone comes and files a regression-bug as a result of it, then we add the special handling?

Or do you suggest we spend a little bit more time thinking before committing the fix?

[lddubeau]

I was thinking about writing a higher level "integration"-test for this (for this specific crash/indentation-error), but I guess that would be an useful thing to do as well.

I would definitely add a case to the indentation tests. Ultimately, both kinds of tests (integration and unit) are desirable, in my book.

I would go ahead with the change and adjust if it causes problems that we missed. I've been running with the code I've shown in my earlier comment for a while without any issue.

[josteink]

I've added the fix we agreed upon, alongside some reasonable basic tests for now.

@lddubeau: If you can think of more "tricky" test-cases you think we should include, feel free to add them on their own.

@AdamNiederer Sorry about taking so long getting this fixed :)