Comments (11)
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 :)
from typescript.el.
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...
from typescript.el.
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.
from typescript.el.
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?
from typescript.el.
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.
from typescript.el.
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?
from typescript.el.
@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)
. :(
from typescript.el.
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.
from typescript.el.
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:
-
Treat strings the same as comments. We just need to test whether
(nth 8 parse)
is set and act on 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. 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.
from typescript.el.
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?
from typescript.el.
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.
from typescript.el.
Related Issues (20)
- Consider migrating CI to something not Travis HOT 5
- Symbol’s value as variable is void: compilation-error-regexp-alist-alist HOT 4
- Indentation hangs in `typescript--backward-to-parameter-list` when previous function has unbalanced parens
- typescript-mode hangs if I enter a newline after a dot character.
- File mode specification error: (void-function -compose) HOT 3
- typescript new keyword 'override' highlight HOT 2
- M-j in /** foo */ comment blocks is broken. Reasonbly easy to fix HOT 3
- Bump Version Tag HOT 8
- const, let formatting with multiple lines HOT 1
- Missing debugger keyword HOT 1
- Freeze on indenting dot after comma HOT 1
- `typescript-mode` is failing to download HOT 4
- Incorrect highlighting of keywords on latest commit HOT 5
- Adding mechanisms in place not to collide with in-tree typescript-mode HOT 7
- typescript 4.9 - satisfies operator HOT 1
- No option to achieve desired formatting HOT 1
- typescript--backward-to-parameter-list can cause multi-second delays in large files HOT 4
- Diagnostics don't show until file is changed HOT 1
- Thanks HOT 1
- matching closing tags in jsx in typescript-ts-mode HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from typescript.el.