Converting type to VariableDeclaration causing issue with core indent rule

[JamesHenry]

Background

In 0.3.0 we added the conversion of type declarations, such as:

type Result<T> = Success<T> | Failure

...to VariableDeclaration ESTree nodes. The final ESTree nodes have an empty declarations: [] array property.

Issue

When using typescript-eslint-parser 0.3.0 with the core rule indent enabled, a runtime TypeError is thrown.

This is because the indent rule implementation does not check for a positive length of the declarations array before attempting to evaluate its final item.

From indent.js:

//...
VariableDeclaration(node) {
  if (node.declarations[node.declarations.length - 1].loc.start.line > node.declarations[0].loc.start.line) {
    checkIndentInVariableDeclarations(node);
  }
},
//...

Solution

IMHO, this is a bug in the core rule and it should be fixed there via a simple length check, but I wanted to report it here to get @nzakas thoughts before submitting a PR to ESLint.

[platinumazure]

For what it's worth, I would support a length check in the indent rule.

[nzakas]

That seems like a bug in the parser. Why is declarations empty?

[JamesHenry]

Reflecting more on this, I do agree. The length check I am suggesting would fix the symptom of the issue, but it would come at the expense of the indent rule taking the type declaration into account.

The declarations array is empty because in #83 I wanted to implement the minimum possible conversion of the node to satisfy the VariableDeclaration "interface".

I will look into converting the type property of the TSNode into a declaration ESTree node and submit a PR to the parser.

@platinumazure Thanks a lot! Now, based on the thoughts above, do you and @nzakas feel that the runtime error in ESLint is actually a good thing to leave in as it is a signal for when a VariableDeclaration is malformed?

[platinumazure]

I didn't realize that there was an eventual goal of converting the type
declaration details into a VariableDeclarator node or equivalent- sorry
about that. I don't mind leaving indent as it is at this point. I like
defensive coding (or, maybe we could express our intention of checking the
declarations array with an assert call), but I'm okay with how it is now
that I understand the plan.

On Sep 16, 2016 2:06 PM, "James Henry" [email protected] wrote:

Reflecting more on this, I do agree. The length check I am suggesting
would fix the symptom of the issue, but it would come at the expense of the
indent rule taking the type declaration into account.

The declarations array is empty because in #83
#83 I wanted to
implement the minimum possible conversion of the node to satisfy the
VariableDeclaration "interface".

I will look into converting the type property of the TSNode into a
declaration ESTree node and submit a PR to the parser.

@platinumazure https://github.com/platinumazure Thanks a lot! Now,
based on the thoughts above, do you and @nzakas
https://github.com/nzakas feel that the runtime error in ESLint is
actually a good thing to leave in as it is a signal for when a
VariableDeclaration is malformed?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#89 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARWei-D7XwT_eYf9yGGu39zDH25M2zRks5qquiigaJpZM4J_DrQ
.

[JamesHenry]

I wouldn't say there explicitly was, the goal is just to allow TS TypeDeclarations to work with existing rules whenever possible.

PR submitted here #91

[nzakas]

I don't think the ESLint rule has an error, so I wouldn't touch that.

I'm sorry I wasn't clear and didn't look close enough at the PR for using a VariableDeclaration for type declarations. My intention was to completely implement VariableDeclaration as we would any of var, let, or const, not just the top node.