Bug: key-spacing with align: "value" produces incorrect alignment

[fasttime]

Environment

Node version: v19.0.0
npm version: v8.19.2
Local ESLint version: v8.26.0
Global ESLint version: Not found
Operating System: darwin 22.1.0

What parser are you using?

Default (Espree)

What did you do?

/* eslint "key-spacing": ["error", { align: "value" }] */
var foo =
{
  code: 1,
  message:
  "some value on the next line",
};

DEMO

What did you expect to happen?

No error.

What actually happened?

Got error

4:9 Missing space before value for key 'code'. (key-spacing)

Autofix adds three spaces after code: to align the value 1 one space after message:, which doesn't seem correct since there is no other value to align with.

If the order of the properties is swapped, as expected, no error is reported.

/* eslint "key-spacing": ["error", { align: "value" }] */
var foo =
{
  message:
  "some value on the next line",
  code: 1
};

DEMO

Participation

• I am willing to submit a pull request for this issue.

Additional comments

No response

[mdjermanovic]

This looks like a bug to me.

/* eslint "key-spacing": ["error", { align: "value" }] */
var foo =
{
  code: 1,
  message:
  "some value on the next line",
};

If the order of the properties is swapped, as expected, no error is reported.

/* eslint "key-spacing": ["error", { align: "value" }] */
var foo =
{
  message:
  "some value on the next line",
  code: 1
};

The different behavior seems to be because in the first case continuesPropertyGroup considers the two properties to be in the same group, while in the second case they are in separate groups.

eslint/lib/rules/key-spacing.js

Lines 343 to 349 in 74a5af4

	function continuesPropertyGroup(lastMember, candidate) {
	const groupEndLine = lastMember.loc.start.line,
	candidateStartLine = candidate.loc.start.line;
	if (candidateStartLine - groupEndLine <= 1) {
	return true;
	}

Perhaps we could change this logic a bit to fix the bug - if value is not on the same line as key, then property doesn't continue the group?

[fasttime]

Thanks @mdjermanovic, I did some debugging and I think your analysis is correct. I can work on a PR to fix this.
It seems that there are currently no unit tests covering the case of a property where the value starts on a different line than the key, so I guess I will add some.

[snitin315]

@fasttime Thanks for taking this up. Yes, you will have to add corresponding test cases in your PR.

[fasttime]

Perhaps we could change this logic a bit to fix the bug - if value is not on the same line as key, then property doesn't continue the group?

This should be fine with align: "value". I'm not sure about align: "colon". So, for instance, this code is currently considered ok by the rule:

/* eslint "key-spacing": ["error", { align: "colon" }] */
({
  command  : "npm",
  arguments:
  [
    "test"
  ]
});

But if we change the logic such that a value on a different line doesn't continue the group, then the rule would report a problem, and the corrected output would be:

/* eslint "key-spacing": ["error", { align: "colon" }] */
({
  command: "npm",
  arguments:
  [
    "test"
  ]
});

Any thoughts about this @mdjermanovic?

[mdjermanovic]

But if we change the logic such that a value on a different line doesn't continue the group, then the rule would report a problem, and the corrected output would be:

/* eslint "key-spacing": ["error", { align: "colon" }] */
({
  command: "npm",
  arguments:
  [
    "test"
  ]
});

I think that's fine. Per the documentation (emphasize mine):

"colon" enforces horizontal alignment of both colons and values in object literals.

Since the value starts on another line, values wouldn't be aligned so this looks similar to the problem with align: "value". I think it's correct to always consider that such property does not belong to the preceding group.