Fix multiline variable declarations.

[dgillis]

See #40

[josteink]

Hey there and thanks for the patch!

I'll be happy to look at it, but first we should probably get the CI build sorted out.

Right now the CI build fails due to the following test failing: indentation-reference-document-is-reflowed-correctly.

Basically if you're updating the indentation-rules, you should:

1. update the test reference-document accordingly, where you think it should be affected.
2. verify that no other formatting-rules have been broken in the process. (make test)

Please check what's the cause of the CI-failure, and add a test-case for the indentation-rule you're implementing if not already there. If there's such a rule already, please update it to stop the old tests from crashing the build :)

[dgillis]

It is failing because assignment expressions where the right-hand side value is spread over multiple lines is being mistakenly interpreted as a new assignment on the second (and onward) lines. E.g., b ought to be aligned with a below but instead my function is doing this:

const foo9 = (a: string,
      b: string): Array<number> => {
    return [1];
}

This is a similar issue to indenting multiline destructuring assignments (see my comment on #41). In both cases, I think the rigourous way to do it is to have function that can walk back over expressions of the form <LHS> = <RHS>, (or simply <LHS>, when it is just a name declaration) until it finds the var/let/const declaration. Each subsequent newline beginning with one of the <LHS> expressions should then be aligned to the first one after the var/let/const.

Lines on the interior of multiline LHS/RHS expressions are more complex. However, typescript-mode otherwise handles them correctly. So I can imagine there is some way to utilize that functionality and augment it with an offset based on the indentation of the parent LHS/RHS expression. I do not have much familiarity with the codebase so it's hard for me to come up with a way of doing this that does not seem like a lot of reinventing the wheel.

[josteink]

Sorry about the delay. I've been quite busy.

And seeing some things being broken, I wanted to take a proper look at things to ensure I had the full picture before I gave some feedback (naturally postponing things even more...)

Well, now I've taken a brief look at least.

So like you've correctly assessed, the CI-build fails because indentation for functions with return annotations no longer behaves as they used to.

This was implemented and fixed by @lddubeau quite recently, so I don't think it will be too popular if we break that while attempting to fix something else.

Do you think it would be possible for you to implement your fix without causing the existing behaviour to break?

I do not have much familiarity with the codebase so it's hard for me to come up with a way of doing this that does not seem like a lot of reinventing the wheel.

Honestly most work I do here is as a maintainer, more than actively writing code, so I can definitely sympathize.

@lddubeau : Would you be able to give this leads to follow? Have you implemented any such functions like the ones he asks about?

[lddubeau]

It seems to me that the failing case that was quoted above is failing because the new code is looking at the test case and treating it just like a multiline declaration, which it isn't. And indeed, just looking at the code, I see the new functionality runs before any of the code I added to handle type annotations for return value. This could be assessed by stepping through the old code and through the new code while trying to manually indent the faulty line in the test file. (I use edebug for this. I do C-u C-M-x to recompile with edebug the function I want to step through, and I recompile it with C-M-x when I'm done. However, I would not be surprised if there's a nicer way to do it.)

It may be possible to keep the call to typescript--multiline-declaration-indentation in the same location it is in right now. However, typescript--multiline-declaration-indentation would have to be modified to be more restrictive as to what it consider to be a multiline declaration.

However, if I were coding this, I would move that call down into the (nth 1 parse-status) branch. As a general rule, there's very very very very very little that can easily go before the (nth 1 parse-status) branch of the cond in typescript--proper-indentation. The initial tests slice the pie in very large and mostly unambiguous pieces: am I in a string?, am I in a comment?, is this a cpp macro (really?... barf!)?, etc. The exception is the test with typescript--ctrl-statement-indentation which is more complex, but, well, it works. I'd expect the new code to operate from within the code handling (nth 1 parse-status), perhaps in a way analogous to how that branch uses same-indent-p and continue-expr-p.

---

Question unrelated to the above. Can the new code handle this?

var x = () => {
    var foo = 1,
        moo = 2;
},
    y = () => { };

The above is formatted according to tsserver.

[lddubeau]

bol to me means "beginning of line". Sometimes, this point is the beginning of line, but not always, right? If that's the case, I'd give it a different name. keyword-start maybe??

[dgillis]

Good point.

[lddubeau]

I see another regex searching for var, const, let elsewhere in typescript--font-lock-keywords-3. Can we perhaps combine the two regexes into a single defconst and refer to the new constant, so as to avoid duplication?

[lddubeau]

If is-looking-at-decl-keyword is true, then match-end should already contain the point you are looking for. Is there something I'm missing? If I'm right then the whole when block could be reduced to:

(when is-looking-at-decl-keyword
  (- (match-end) (point)))

or something of that order.

[josteink]

Any updates on this PR? Any plans on fixing the things it breaks? :)

[lddubeau]

I'm closing this due to lack of activity. I'll gladly reopen if development of the PR resumes.