From bf496851df5fd55714491fc4eac4702ea908feca Mon Sep 17 00:00:00 2001 From: Kelvin Kavisi <68240897+kekavc24@users.noreply.github.com> Date: Fri, 7 Nov 2025 12:25:05 +0000 Subject: [PATCH 1/2] Prevent `YamlEditor.remove` from affecting existing block scalars * Refactor `_removeFromBlockList` and `_removeFromBlockMap` and remove dangling line breaks which may affect how block scalars are intepreted by the `package: yaml`. * Prefer obtaining an entry in one pass from `YamlMap`. * Add tests --- pkgs/yaml_edit/lib/src/equality.dart | 14 + pkgs/yaml_edit/lib/src/list_mutations.dart | 95 +++---- pkgs/yaml_edit/lib/src/map_mutations.dart | 101 +++---- pkgs/yaml_edit/lib/src/utils.dart | 309 +++++++++++++++++++++ pkgs/yaml_edit/test/remove_test.dart | 78 ++++++ 5 files changed, 470 insertions(+), 127 deletions(-) diff --git a/pkgs/yaml_edit/lib/src/equality.dart b/pkgs/yaml_edit/lib/src/equality.dart index 0c6a9526a..cf6404481 100644 --- a/pkgs/yaml_edit/lib/src/equality.dart +++ b/pkgs/yaml_edit/lib/src/equality.dart @@ -91,6 +91,20 @@ YamlNode getKeyNode(YamlMap map, Object? key) { return map.nodes.keys.firstWhere((node) => deepEquals(node, key)) as YamlNode; } +/// Returns the entry associated with a [mapKey] and its index in the [map]. +({int index, YamlNode keyNode, YamlNode valueNode}) getYamlMapEntry( + YamlMap map, + Object? mapKey, +) { + for (final (index, MapEntry(:key, :value)) in map.nodes.entries.indexed) { + if (deepEquals(key, mapKey)) { + return (index: index, keyNode: key, valueNode: value); + } + } + + throw YamlException('$mapKey not found in map', map.span); +} + /// Returns the [YamlNode] after the [YamlNode] corresponding to the provided /// [key]. YamlNode? getNextKeyNode(YamlMap map, Object? key) { diff --git a/pkgs/yaml_edit/lib/src/list_mutations.dart b/pkgs/yaml_edit/lib/src/list_mutations.dart index 17da6dd77..acf1f36b9 100644 --- a/pkgs/yaml_edit/lib/src/list_mutations.dart +++ b/pkgs/yaml_edit/lib/src/list_mutations.dart @@ -306,72 +306,43 @@ SourceEdit _insertInFlowList( /// [index] should be non-negative and less than or equal to `list.length`. SourceEdit _removeFromBlockList( YamlEditor yamlEdit, YamlList list, YamlNode nodeToRemove, int index) { - RangeError.checkValueInInterval(index, 0, list.length - 1); - - var end = getContentSensitiveEnd(nodeToRemove); - - /// If we are removing the last element in a block list, convert it into a - /// flow empty list. - if (list.length == 1) { - final start = list.span.start.offset; - - return SourceEdit(start, end - start, '[]'); - } + final listSize = list.length; + RangeError.checkValueInInterval(index, 0, listSize - 1); final yaml = yamlEdit.toString(); final span = nodeToRemove.span; - /// Adjust the end to clear the new line after the end too. - /// - /// We do this because we suspect that our users will want the inline - /// comments to disappear too. - final nextNewLine = yaml.indexOf('\n', end); - if (nextNewLine != -1) { - end = nextNewLine + 1; - } - - /// If the value is empty - if (span.length == 0) { - var start = span.start.offset; - return SourceEdit(start, end - start, ''); - } - - /// -1 accounts for the fact that the content can start with a dash - var start = yaml.lastIndexOf('-', span.start.offset - 1); - - /// Check if there is a `-` before the node - if (start > 0) { - final lastHyphen = yaml.lastIndexOf('-', start - 1); - final lastNewLine = yaml.lastIndexOf('\n', start - 1); - if (lastHyphen > lastNewLine) { - start = lastHyphen + 2; - - /// If there is a `-` before the node, we need to check if we have - /// to update the indentation of the next node. - if (index < list.length - 1) { - /// Since [end] is currently set to the next new line after the current - /// node, check if we see a possible comment first, or a hyphen first. - /// Note that no actual content can appear here. - /// - /// We check this way because the start of a span in a block list is - /// the start of its value, and checking from the back leaves us - /// easily confused if there are comments that have dashes in them. - final nextHash = yaml.indexOf('#', end); - final nextHyphen = yaml.indexOf('-', end); - final nextNewLine = yaml.indexOf('\n', end); - - /// If [end] is on the same line as the hyphen of the next node - if ((nextHash == -1 || nextHyphen < nextHash) && - nextHyphen < nextNewLine) { - end = nextHyphen; - } - } - } else if (lastNewLine > lastHyphen) { - start = lastNewLine + 1; - } - } - - return SourceEdit(start, end - start, ''); + return removeBlockCollectionEntry( + yaml, + blockCollection: list, + isFirstEntry: index == 0, + isSingleEntry: listSize == 1, + isLastEntry: index >= listSize - 1, + nodeToRemoveOffset: ( + start: span.length == 0 + ? span.start.offset + : yaml.lastIndexOf('-', span.start.offset - 1), + end: getContentSensitiveEnd(nodeToRemove) + ), + lineEnding: getLineEnding(yaml), + nextBlockNodeInfo: () { + final nextNode = list.nodes[index + 1]; + final nextNodeSpan = nextNode.span; + final offset = nextNodeSpan.start.offset; + + final hyphenOffset = yaml.lastIndexOf( + '-', + span.length == 0 ? offset : offset - 1, + ); + + final nearestLineEnding = yaml.lastIndexOf('\n', hyphenOffset); + + return ( + nearestLineEnding: nearestLineEnding, + nextNodeColStart: hyphenOffset - (nearestLineEnding + 1), + ); + }, + ); } /// Returns a [SourceEdit] describing the change to be made on [yamlEdit] to diff --git a/pkgs/yaml_edit/lib/src/map_mutations.dart b/pkgs/yaml_edit/lib/src/map_mutations.dart index 46e8c7935..3b655c95d 100644 --- a/pkgs/yaml_edit/lib/src/map_mutations.dart +++ b/pkgs/yaml_edit/lib/src/map_mutations.dart @@ -36,13 +36,11 @@ SourceEdit updateInMap( /// removing the element at [key] when re-parsed. SourceEdit removeInMap(YamlEditor yamlEdit, YamlMap map, Object? key) { assert(containsKey(map, key)); - final keyNode = getKeyNode(map, key); - final valueNode = map.nodes[keyNode]!; if (map.style == CollectionStyle.FLOW) { - return _removeFromFlowMap(yamlEdit, map, keyNode, valueNode); + return _removeFromFlowMap(yamlEdit, map, key); } else { - return _removeFromBlockMap(yamlEdit, map, keyNode, valueNode); + return _removeFromBlockMap(yamlEdit, map, key); } } @@ -169,74 +167,47 @@ SourceEdit _replaceInFlowMap( } /// Performs the string operation on [yamlEdit] to achieve the effect of -/// removing the [keyNode] from the map, bearing in mind that this is a block +/// removing the [key] from the map, bearing in mind that this is a block /// map. -SourceEdit _removeFromBlockMap( - YamlEditor yamlEdit, YamlMap map, YamlNode keyNode, YamlNode valueNode) { - final keySpan = keyNode.span; - var end = getContentSensitiveEnd(valueNode); +SourceEdit _removeFromBlockMap(YamlEditor yamlEdit, YamlMap map, Object? key) { + final (index: entryIndex, :keyNode, :valueNode) = getYamlMapEntry(map, key); final yaml = yamlEdit.toString(); - final lineEnding = getLineEnding(yaml); - - if (map.length == 1) { - final start = map.span.start.offset; - final nextNewLine = yaml.indexOf(lineEnding, end); - if (nextNewLine != -1) { - // Remove everything up to the next newline, this strips comments that - // follows on the same line as the value we're removing. - // It also ensures we consume colon when [valueNode.value] is `null` - // because there is no value (e.g. `key: \n`). Because [valueNode.span] in - // such cases point to the colon `:`. - end = nextNewLine; - } else { - // Remove everything until the end of the document, if there is no newline - end = yaml.length; - } - return SourceEdit(start, end - start, '{}'); - } - - var start = keySpan.start.offset; - - /// Adjust the end to clear the new line after the end too. - /// - /// We do this because we suspect that our users will want the inline - /// comments to disappear too. - final nextNewLine = yaml.indexOf(lineEnding, end); - if (nextNewLine != -1) { - end = nextNewLine + lineEnding.length; - } else { - // Remove everything until the end of the document, if there is no newline - end = yaml.length; - } - - final nextNode = getNextKeyNode(map, keyNode); - - if (start > 0) { - final lastHyphen = yaml.lastIndexOf('-', start - 1); - final lastNewLine = yaml.lastIndexOf(lineEnding, start - 1); - if (lastHyphen > lastNewLine) { - start = lastHyphen + 2; - - /// If there is a `-` before the node, and the end is on the same line - /// as the next node, we need to add the necessary offset to the end to - /// make sure the next node has the correct indentation. - if (nextNode != null && - nextNode.span.start.offset - end <= nextNode.span.start.column) { - end += nextNode.span.start.column; - } - } else if (lastNewLine > lastHyphen) { - start = lastNewLine + lineEnding.length; - } - } + final mapSize = map.length; + final keySpan = keyNode.span; - return SourceEdit(start, end - start, ''); + return removeBlockCollectionEntry( + yaml, + blockCollection: map, + isFirstEntry: entryIndex == 0, + isSingleEntry: mapSize == 1, + isLastEntry: entryIndex >= mapSize - 1, + nodeToRemoveOffset: ( + start: keySpan.start.offset, + end: valueNode.span.length == 0 + ? keySpan.end.offset + 2 // Null value have no span. Skip ":". + : getContentSensitiveEnd(valueNode), + ), + lineEnding: getLineEnding(yaml), + + // Only called when the next node is present. Never before. + nextBlockNodeInfo: () { + final nextKeyNode = map.nodes.keys.elementAt(entryIndex + 1) as YamlNode; + final nextKeySpan = nextKeyNode.span.start; + + return ( + nearestLineEnding: yaml.lastIndexOf('\n', nextKeySpan.offset), + nextNodeColStart: nextKeySpan.column + ); + }, + ); } /// Performs the string operation on [yamlEdit] to achieve the effect of -/// removing the [keyNode] from the map, bearing in mind that this is a flow +/// removing the [key] from the map, bearing in mind that this is a flow /// map. -SourceEdit _removeFromFlowMap( - YamlEditor yamlEdit, YamlMap map, YamlNode keyNode, YamlNode valueNode) { +SourceEdit _removeFromFlowMap(YamlEditor yamlEdit, YamlMap map, Object? key) { + final (index: _, :keyNode, :valueNode) = getYamlMapEntry(map, key); + var start = keyNode.span.start.offset; var end = valueNode.span.end.offset; final yaml = yamlEdit.toString(); diff --git a/pkgs/yaml_edit/lib/src/utils.dart b/pkgs/yaml_edit/lib/src/utils.dart index ef85526d3..33580731c 100644 --- a/pkgs/yaml_edit/lib/src/utils.dart +++ b/pkgs/yaml_edit/lib/src/utils.dart @@ -2,10 +2,13 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'dart:math'; + import 'package:source_span/source_span.dart'; import 'package:yaml/yaml.dart'; import 'editor.dart'; +import 'source_edit.dart'; import 'wrap.dart'; /// Invoke [fn] while setting [yamlWarningCallback] to [warn], and restore @@ -277,6 +280,312 @@ String getLineEnding(String yaml) { return windowsNewlines > unixNewlines ? '\r\n' : '\n'; } +final _nonSpaceMatch = RegExp(r'[^ \t]'); + +/// Skip empty lines and returns the offset of the last possible line ending +/// only if the [offset] is a valid offset within the [yaml] string that +/// points to first line ending. +int indexOfLastLineEnding(String yaml, int offset) { + if (yaml.isEmpty || offset == -1) return yaml.length; + + final lastOffset = yaml.length - 1; + var currentOffset = min(offset, lastOffset); + + if (yaml[currentOffset] case '\r' || '\n') { + var lineEndingIndex = currentOffset; + ++currentOffset; + + scanner: + while (currentOffset <= lastOffset) { + switch (yaml[currentOffset]) { + case ' ' || '\t': + { + currentOffset = yaml.indexOf(_nonSpaceMatch, currentOffset); + + // We scanned till the end of the string and found nothing. + if (currentOffset == -1) { + lineEndingIndex = lastOffset; + break scanner; + } + } + + case '\r' || '\n': + { + lineEndingIndex = currentOffset; + ++currentOffset; + } + + default: + break scanner; + } + } + + return lineEndingIndex; + } + + return currentOffset; +} + +/// Backtracks from the [start] offset and looks for the nearest character +/// that is not a separation space (tab/space) that can be used to declare a +/// nested compact block map +/// +/// ```yaml +/// # In a block list +/// - key: value +/// next: value +/// +/// --- +/// # In an explicit key and its value +/// +/// ? key: value +/// next: value +/// : key: value +/// next: value +/// ``` +/// +/// If a line feed `\n` is encountered first then `compactCharOffset` defaults +/// to -1. Otherwise, only returns a non-negative `compactCharOffset` if +/// `?`, `-` or `:` were seen. +({int compactCharOffset, int lineEndingIndex}) indexOfCompactChar( + String yaml, + int start, +) { + var startOffset = max(0, start - 1); + + scanner: + while (true) { + switch (yaml[startOffset]) { + // This is either indent or separation space + case ' ' || '\t': + { + startOffset = yaml.lastIndexOf(_nonSpaceMatch, startOffset); + if (startOffset == -1) break scanner; + } + + case '\r' || '\n': + return (compactCharOffset: -1, lineEndingIndex: startOffset); + + /// Block sequences and explicit keys/values can be used to declare block + /// maps in a compact-inline notation. + /// + /// - a: b + /// c: d + /// + /// OR as an explicit key with its explicit value + /// + /// ? a: b + /// c: d + /// : e: f + /// g: h + /// + /// See "Example 8.19 Compact Block Mappings" at + /// https://yaml.org/spec/1.2.2/#822-block-mappings + case '-' || '?' || ':': + return (compactCharOffset: startOffset, lineEndingIndex: -1); + + default: + break scanner; + } + } + + return (compactCharOffset: -1, lineEndingIndex: -1); +} + +typedef NextBlockNodeInfo = ({int nearestLineEnding, int nextNodeColStart}); +typedef BlockNodeOffset = ({int start, int end}); + +/// Removes a block entry and its line ending from a [blockCollection] using the +/// [nodeToRemoveOffset] provided. Any trailing comments are also removed. +/// +/// If [blockCollection] is a [YamlMap], the chunk removed corresponds to the +/// key-value pair represented by the offset. If [blockCollection] is a +/// [YamlList], the chunk represents a single element within the list. +/// +/// [nextBlockNodeInfo] is only called if the [blockCollection] has at least +/// 2 entries and the entry being removed is not the last entry in the +/// collection. +SourceEdit removeBlockCollectionEntry( + String yaml, { + required YamlNode blockCollection, + required bool isFirstEntry, + required bool isSingleEntry, + required bool isLastEntry, + required BlockNodeOffset nodeToRemoveOffset, + required String lineEnding, + required NextBlockNodeInfo Function() nextBlockNodeInfo, +}) { + final isBlockList = blockCollection is YamlList; + + assert( + isBlockList || blockCollection is YamlMap, + 'Expected a block map/list', + ); + + var makeNextNodeCompact = false; + var (:start, :end) = nodeToRemoveOffset; + + if (start != 0 && !isSingleEntry) { + /// Try making it compact in case the collection's parent is a: + /// - block sequence + /// - explicit key + /// - explicit key's explicit value. + if (isFirstEntry) { + final (:compactCharOffset, :lineEndingIndex) = indexOfCompactChar( + yaml, + start, + ); + + if (compactCharOffset != -1) { + start = compactCharOffset + 2; // Skip separation space. + makeNextNodeCompact = true; + } else { + start = lineEndingIndex + 1; + } + } else { + /// If not possible, just consume this node's indent. This prevents this + /// node from interfering with the next node. + start = yaml.lastIndexOf('\n', start) + 1; + } + } + + var replacement = ''; + + // Skip empty lines to the last line break + end = indexOfLastLineEnding(yaml, yaml.indexOf('\n', end - 1)); + end = min(++end, yaml.length); // Mark it for removal + + if (isSingleEntry) { + /// Take back the last line ending even if multiple were skipped. This node + /// is sandwiched between other elements not in this map. + if (end < yaml.length - 1) { + end -= lineEnding == '\r\n' ? 2 : 1; + } + + replacement = isBlockList ? '[]' : '{}'; + } else if (!isLastEntry) { + final (:nearestLineEnding, :nextNodeColStart) = nextBlockNodeInfo(); + final trueEndOffset = end - 1; + + /// Make compact only if we are pointing to same line break from different + /// node extremes. This can only be true if we are removing the first + /// entry. + /// + /// ** For block lists ** + /// + /// [*] Before: + /// + /// - - value + /// - next + /// + /// [*] After: + /// + /// - - next + /// + /// ** For block maps ** + /// + /// [*] Before: + /// + /// - key: value + /// next: value + /// + /// [*] After: + /// + /// - next: value + if (makeNextNodeCompact) { + /// Leaving comments here has no effect since we are removing the first + /// entry of the collection. The next node will be the first. However, the + /// inline comment hugging the outermost node in the current line will be + /// consumed. + /// + /// ** For Block Lists ** + /// + /// [*] Before: + /// + /// - - value # Comment + /// # Next Comment + /// - next + /// + /// [*] After: + /// + /// - # Next Comment + /// - next + /// + /// ** For Block Maps ** + /// + /// [*] Before: + /// + /// - key: value # Comment + /// # Next Comment + /// next: value + /// + /// [*] After: + /// + /// - # Next Comment + /// next-key: value + end = nearestLineEnding == trueEndOffset ? end + nextNodeColStart : end; + } else if (nearestLineEnding != trueEndOffset) { + /// We shouldn't assume the string will always have a flow node. Edits + /// may be made to a YAML string which declared block scalars initially. + /// Better safe than sorry. + /// + /// Truncate any dangling comments which may change the structure of + /// existing nodes. We must point to the same offset after every empty + /// line has been skipped. Mimic (intelligently) what the parser does + /// with lexing. + /// + /// ** For Block Lists ** + /// + /// [*] Before: + /// + /// - >+ + /// folded keep + /// + /// - target # Comment + /// # Next Comment + /// # Another Comment + /// - next + /// + /// [*] After: + /// + /// - >+ + /// folded keep + /// + /// - next + /// + /// ** For Block Maps ** + /// + /// [*] Before: + /// + /// key: >+ + /// value + /// + /// target-key: value # Comment + /// # Next Comment + /// # Another Comment + /// + /// next: here + /// + /// [*] After: + /// + /// key: >+ + /// value + /// + /// next: here + /// + /// We preserved the string's meaning without much effort. + end = nearestLineEnding + 1; + } + } else { + end = max( + yaml.lastIndexOf('\n', blockCollection.span.end.offset) + 1, + end, + ); + } + + return SourceEdit(start, end - start, replacement); +} + extension YamlNodeExtension on YamlNode { /// Returns the [CollectionStyle] of `this` if `this` is [YamlMap] or /// [YamlList]. diff --git a/pkgs/yaml_edit/test/remove_test.dart b/pkgs/yaml_edit/test/remove_test.dart index 4742b5608..f601584ed 100644 --- a/pkgs/yaml_edit/test/remove_test.dart +++ b/pkgs/yaml_edit/test/remove_test.dart @@ -241,6 +241,25 @@ dev_dependencies: retry:'''); doc.remove(['dev_dependencies', 'retry']); }); + + test('Removes node with preceding block scalar', () { + final doc = YamlEditor(''' +key: >+ + folded with keep chomping + +target-key: value # Comment + # Comment +next: value +'''); + + doc.remove(['target-key']); + expect(doc.toString(), ''' +key: >+ + folded with keep chomping + +next: value +'''); + }); }); group('flow map', () { @@ -564,6 +583,65 @@ b: } ]); }); + + test('Removes comments that may interfere with block scalar', () { + final doc = YamlEditor(''' +- >+ + folded keep chomp + +- - - |+ # Nested + literal keep chomp + + - value # May interfere + # With block node + +- top # With + # Funky + + # Comments + + # Comment +'''); + + doc.remove([1, 1]); + + expect(doc.toString(), ''' +- >+ + folded keep chomp + +- - - |+ # Nested + literal keep chomp + +- top # With + # Funky + + # Comments + + # Comment +'''); + + doc.remove([1]); + + expect(doc.toString(), ''' +- >+ + folded keep chomp + +- top # With + # Funky + + # Comments + + # Comment +'''); + + doc.remove([1]); + + expect(doc.toString(), ''' +- >+ + folded keep chomp + +'''); + }); }); group('flow list', () { From 111d8e651ca54816bd7c9ee245ca858b053f5eaf Mon Sep 17 00:00:00 2001 From: Kelvin Kavisi <68240897+kekavc24@users.noreply.github.com> Date: Sun, 9 Nov 2025 22:44:49 +0000 Subject: [PATCH 2/2] Use correct node span from the next node --- pkgs/yaml_edit/lib/src/list_mutations.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkgs/yaml_edit/lib/src/list_mutations.dart b/pkgs/yaml_edit/lib/src/list_mutations.dart index acf1f36b9..a82e0876c 100644 --- a/pkgs/yaml_edit/lib/src/list_mutations.dart +++ b/pkgs/yaml_edit/lib/src/list_mutations.dart @@ -332,7 +332,7 @@ SourceEdit _removeFromBlockList( final hyphenOffset = yaml.lastIndexOf( '-', - span.length == 0 ? offset : offset - 1, + nextNodeSpan.length == 0 ? offset : offset - 1, ); final nearestLineEnding = yaml.lastIndexOf('\n', hyphenOffset);