fix: use correct positions for ranges

[adalinesimonian]

While testing range support using Stylelint, I noticed a couple of problems:

• Ranges were using the node's start position for both start and end by default
• There was no way to force PostCSS to use ranges if endIndex or word weren't provided

This patch fixes the first by using the node's end position as the default for end. The second problem is addressed by adding a new option, inferRange, which forces PostCSS to infer ranges when a word or endIndex aren't provided. I thought to do it this way so that existing code elsewhere won't get ranges if it only provides a node unless it opts into it since I'm not sure if ranges would always be desired. However, if you disagree, I could be convinced otherwise to make ranges default regardless.

[ai]

Can you explain “There was no way to force PostCSS to use ranges”? For instance, by showing the error when developer need to use inferRange.

[adalinesimonian]

Can you explain “There was no way to force PostCSS to use ranges”? For instance, by showing the error when developer need to use inferRange.

Sure thing; for example:

result.warn('message', { node, index, endIndex })
result.warn('message', { node, word })

would use ranges. But if you wanted to infer a range for the whole node without providing an index, you wouldn't be able to:

result.warn('message', { node })

would return a single position and endLine and endColumn would be undefined. But with the inferRange option, now you can:

result.warn('message', { node, inferRange: true })

[ai]

What downsides do you see in enabling inferRange by default for warnings/errors with node?

I like the idea of changing node behaviour without adding new option. If we had no end in the 8.2 we do not change behaviour.

[adalinesimonian]

I was thinking of the case:

result.warn(message, { node, index })

With current behaviour this would warn for a specific index. But if the range behaviour was default, it would warn for the range from the index to the end of the node. I wasn't sure if this would be desired in some instances.

[ai]

But if the range behaviour was default, it would warn for the range from the index to the end of the node. I wasn't sure if this would be desired in some instances.

Yeap, in the case of index and node, it will be strange to do it.

Can we remove inferRange and use end detection only for the case node && !index?

[adalinesimonian]

I think that's a good idea. How should we handle the case when the user wants to have a range and is only providing a start index since they want the range to extend to the end of the node?

[ai]

Do we generate end for errors and warnings where word was passed? It will be nice.

How should we handle the case when the user wants to have a range and is only providing a start index since they want the range to extend to the end of the node?

Hm. Can you show an example of such warning?

[adalinesimonian]

Do we generate end for errors and warnings where word was passed? It will be nice.

Yes, it currently does so. I also updated the word warning test in the original PR: b1583d9#diff-f4ad3511d3af2ff0b7ae47b50c500dd3609bc42a6b74ed01ff3b3befec547a21R72-R78

How should we handle the case when the user wants to have a range and is only providing a start index since they want the range to extend to the end of the node?

Hm. Can you show an example of such warning?

For example, if we wanted to generate a warning for:

a {
  color: red      ;
  /*        ^^^^^^^ */
}

or:

a { /* starting at brace */
  color: red;
} /* ending at end */

If cases like these can be handled without needing an extra check, maybe I just don't know about it?

[adalinesimonian]

One thing I was just thinking about: the end position for a warning is exclusive, as per the LSP specification. So if a warning is emitted for a single position, shouldn't it also get an end line and end column position? i.e. if the warning is for only the single character at line 1 col 2 the end position would be line 1 col 3.

[ai]

For example, if we wanted to generate a warning for:

What do you think about explicit API like?

decl.warn('Spaces at the end', { index, end: decl.source.end })

So if a warning is emitted for a single position, shouldn't it also get an end line and end column position? i.e. if the warning is for only the single character at line 1 col 2 the end position would be line 1 col 3.

Yeap, I like the idea. More consistency.

[adalinesimonian]

What do you think about explicit API like?

decl.warn('Spaces at the end', { index, end: decl.source.end })

I really like this! For consistency, maybe we could also accept the start as a position? Otherwise, it might be strange only to accept an index for start but a position or index for end.

So if a warning is emitted for a single position, shouldn't it also get an end line and end column position? i.e. if the warning is for only the single character at line 1 col 2 the end position would be line 1 col 3.

Yeap, I like the idea. More consistency.

I'll make the appropriate change shortly.

[ai]

For consistency, maybe we could also accept the start as a position?

Sure!

[adalinesimonian]

Here's the API with the latest commit:

result.warn(message, { index }) // -> one character warning at index
result.warn(message, { endIndex }) // -> warning starts at node start, ends at endIndex
result.warn(message, { index, endIndex }) // -> warning starts at index, ends at endIndex
result.warn(message, { start }) // -> warning starts at start, ends at node end
result.warn(message, { end }) // -> warning starts at node start, ends at end
result.warn(message, { start, end }) // -> warning starts at start, ends at end
result.warn(message, { word }) // -> warning starts at word location, ends at word index + length

Using the node's end is a little different, however, since source.end is inclusive, not exclusive like warning ranges are expected to be. So instead of:

result.warn(message, { index, end: decl.source.end })

you'd use

result.warn(message, { index, end: { line: decl.source.end.line, column: decl.source.end.column + 1 } })

The alternative was to do this automatically in warn, but then end would be expected to be inclusive even though all other ending parameters for warnings are expected to be exclusive. So another way to do this that's less ugly would be:

result.warn(message, { start: decl.positionBy({ index: 0 }) })

[ai]

Sorry, can I ask you to rebase branch and update tests to uvu?

[adalinesimonian]

Updated in 8e2366b and aed9a9b. Merged by accident instead of rebased, hope that's not an issue.

[ai]

Great. I am planing to release this release today or tomorrow.

Do you need some time to test it or do we ready to go?

[adalinesimonian]

It seems to be working well enough in stylelint/stylelint#5725!