Fix Directive argument name and value ranges on non-first line

[Kyle-Ye]

Bug/issue #, if applicable:

Close #71

Summary

• Remove unnecessary optional check
• Fix and add test case for single line block parse

Dependencies

None

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran the ./bin/test script and it succeeded
• Updated documentation if necessary

[Kyle-Ye]

Some rough draft images that may help code review

[Kyle-Ye]

@swift-ci please test

[QuietMisdreavus]

Looking at the code again, it seems like there's an existing lineStartIndex property on this LineSegment type that should always be populated. Did that not work for this purpose?

[Kyle-Ye]

/// Create an argument line segment.
/// - Parameters:
///   - untrimmedText: the segment's untrimmed text from which arguments can be parsed.
///   - lineStartIndex: the index in ``text`` where the line started.
means.

As the documentation says, lineStartIndex is the index in text where the line started.

And lineStartIndex is init as untrimmedText.startIndex in BlockDirectParser.

eg.

(lldb) po untrimmedText
"hello\n @Options(scope: page"

(lldb) po untrimmedText[lineStartIndex]
"h"

(lldb) po untrimmedText[untrimmedText.index(parseIndex, offsetBy: 1 - (range?.lowerBound.column ?? 1))]
" "

[Kyle-Ye]

Looking around the usage of lineStartIndex. It seems it's currently almost not used in any place.

[Kyle-Ye]

The bug has been there for some time. Can anyone help continue to review this so that we can merge and solve it? Thanks

[d-ronnqvist]

I looked at this today and as far as I can tell the issue is that TrimmedLine and LineSegment mean different things by untrimmedText. For TrimmedLine the untrimmed text is only expecting to hold one line of text:

/// The original untrimmed text of the line.
let untrimmedText: Substring

whereas LineSegment holds a potentially larger string and points to the index where the line starts

/// The segment's untrimmed text from which arguments can be parsed.
public var untrimmedText: String
/// The index in ``untrimmedText`` where the line started.
public var lineStartIndex: String.Index

Because of this difference, passing the full untrimmed line from the line segment to the trimmed line is probably not the right thing to do.

I checked out this branch to continue investigating this issue and two things worth pointing out:

• The code that creates a LineSegment for each directive argument text doesn't compute lineStartIndex, it just passes the start index of the full text.
• The code that creates a LineSegment for each directive argument text does slice of the "untrimmed text" after the end index of the argument text. For every line of directive arguments, this creates a copy of the entire document up to and including that line.
• The TrimmedLine only trims what comes before the parseIndex.

Since the start and end of the line segment's "untrimmedText" is sliced in two different places, it raises a question of which place should be responsible for slicing the line segment's text.

I think both of these should be done in one place, so I could imagine two different solutions here:

• Change LineSegment so that untrimmedText only stores one line of text (same as TrimmedLine.untrimmedText). This means that lineStartIndex is always untrimmedText.startIndex.
• Add API to LineSegment so that the line substring can be retrieved and use that so that TrimmedLine is initialized with only one line.

Looking at the other places that create a LineSegment I felt that the first alternative was the better solution, mainly because it makes both untrimmedText properties mean that same thing.

Since I had already done most of this locally at this point I opened a PR here with the changes I made.

[Kyle-Ye]

Thanks for the follow-up. I'll read and review it later this week.

[Kyle-Ye]

@swift-ci please test

[Kyle-Ye]

Do we need a 3rd people to help review this? @d-ronnqvist Maybe you can invite one

[Kyle-Ye]

@swift-ci please test

[Kyle-Ye]

@d-ronnqvist Would you help give this a final review so that we can merge it?