Regression: origStart is not accurate in some cases

[ikatyang]

version: 1.7.0 (minimal reproducible version: 1.2.0)

const YAML = require("yaml");

const text = `
a: #b

`.replace(/\n/g, "\r\n");

const cst = YAML.parseCST(text);

cst.setOrigRanges();

console.log(
  JSON.stringify(text[cst[0].contents[1].items[2].props[0].origStart])
); // expected "#" but got "\r"

[eemeli]

Something weird is happening with the BlankLine node there; not sure if the issues are related. But I find it surprising that it's before rather than after the CollectionItem. Will investigate.

[eemeli]

Bother, the issues are indeed related, and they do get a bit complicated. There are three things going on here that are, when put together, a bit wrong:

• CollectionItem is too eager to generate BlankLine nodes, in particular for empty map values. Sometimes, this may currently lead to one blank line being expressed twice in the CST.
• Comments may exist in the CST both as independent nodes and as node props; comments after the : but before the value currently end up as props of the CollectionItem. Blank lines on the other hand are never props, and if they're after a CollectionItem comment but before its value or another comment, they have no better place to go int the CST than before the node's entry.
• When setOrigRanges() is run, it doesn't handle well non-monotonously increasing ranges.

So something's got to give here. I can fix most of the bugginess without any deeper changes, but that'll still leave some problematic cases where it's possible to end up with BlankLine nodes that have ranges that start after the start location of subsequent nodes. I'm pretty sure that this is isolated to map values, but that's because blank lines between most other prop comments & values are currently dropped.

Now, there are three possible ways to fix this, and it's not obvious to me which one to go with:

1. Stop detecting blank lines between map value comments or its comment & value.
2. Also allow blank lines to exist as CST node props, and put them there for map values.
3. For map value comments, add them to the collection as independent nodes rather than props.

I'm leaning towards the third option right now, but I'll at least sleep on this before picking a way forward.

[eemeli]

I went with the first option, at least for now. Blank lines that could appear between comments or a value and a preceding comment are in any case ignored by the AST level, and this should fix the actual bugs that were identified.

Strictly speaking option 2 might've required a major version bump, as it would add a new prop type for nodes. Option 3 might work, but there's some special handling for map value comments at the AST level that breaks if the comments are nodes rather than props, as well as issues that come up with comments in compact in-line notation cases, where the reference to the surrounding item array is more complicated. All that added complexity just isn't worth it.

[eemeli]

Fix included in patch release 1.7.1.