Detect boundaries of parse inputs

[lionel-]

Progress towards posit-dev/positron#1326.

parse_boundaries() uses the R parser to detect the line boundaries of:

• Complete inputs (zero or more)
• Incomplete inputs (optional)
• Invalid inputs (optional)

The boundaries are for lines of inputs rather than expressions. For instance, foo; bar has one input whereas foo\nbar has two inputs.

Invariants:

• Empty lines and lines containing only whitespace or comments are complete inputs.
• There is always at least one range since the empty string is a complete input.
• The ranges are sorted and non-overlapping.
• Inputs are classified in complete, incomplete, and error sections. The
  sections are sorted in this order (there cannot be complete inputs after
  an incomplete or error one, there cannot be an incomplete input after
  an error one, and error inputs are always trailing).
• There is only one incomplete and one error input in a set of inputs.

Approach:

I originally thought I'd use the parse data artifacts created by side effects to detect boundaries of complete expressions in case of incomplete and invalid inputs (see infrastructure implemented in #508). The goal was to avoid non-linear performance as the number of lines increases. I didn't end up doing that because the parse data is actually unusable for more complex cases than what I tried during my exploration.

Instead, I had to resort to parsing line by line. I start with the entire set of lines and back up one line at a time until the parse fully completes. Along the way I keep track of the error and incomplete sections of the input. In the most common cases (valid inputs, short incomplete input at the end), this should only require one or a few iterations. The boundaries of complete expressions are retrieved from the source references of the parsed expressions (using infrastructure from #482) and then transformed to complete inputs.

Supporting infrastructure:

• New CharacterVector::slice() method that returns a &[SEXP] and a corresponding CharacterVector::TryFrom<&[SEXP]> method. (Eventually List should gain those as well.) These methods make it easy to slice character vectors from the Rust side. I use this to shorten the vector of lines without having to start from a &str and reallocate CHARSXPs everytime.

• SrcFile can now be constructed from a CharacterVector with TryFrom.

• Vector::create() is no longer unsafe.

• ArkRange::TryFrom<SrcRef> method, although I didn't end up using it.

[lionel-]

TODO:

Error section should contain line number and error message

Whitespace/comment inputs should be tagged with a boolean

[DavisVaughan]

Everything else here is safe. Do you want to eventually make these unsafe again? If so I'd propose keeping it safe for consistency for now and then we move away from it (but to me this feels like it should be safe)

[lionel-]

hmm I think it's unsafe because it's taking an untyped SEXP and it doesn't return any Result.

We might argue that it's fine because it throws R errors if the type isn't right (but note that it's an implementation detail that it does so instead of crashing like other accessors like CAR()).

But I think the criterion for a safe function is that it guarantees a crash or an R error can't happen (either by dynamic checks feeding the Err path of the Result or by taking typed inputs with strong guarantees such that errors or crashes can't happen).

Does that make sense?

[DavisVaughan]

Oh that's a great idea

[DavisVaughan]

• This method does not seem to be used
• The leading comment about Return type would be clearer seems outdated because syntax error is integrated into ParseResult now I think

[lionel-]

It's no longer used, neither is ParseData, but I've left those for later in case they are needed. E.g. maybe it'll be useful to generate rowan trees from the R parser in a more efficient way than traversing R AST.

I removed the dangling comment, good catch.

[DavisVaughan]

Not used?

Also feels like it should return a typed ParseDataRange to be more self documenting

(I've noticed that the ruff team uses custom types for almost everything, and I really think it makes the code easier to read)

[lionel-]

In general I think it's a balance because too much vocabulary can also make code opaque instead of transparent.

But I'm willing to try and make more type aliases to see how this feels.

[DavisVaughan]

Cool that this does len * mem::size_of::<T>() internally automatically

[DavisVaughan]

I think this is where we'd benefit from something like TextRange
https://github.com/astral-sh/ruff/blob/bb12fe9d0c71fcb36a5000260f62dbf8411b74b4/crates/ruff_text_size/src/range.rs#L15-L19

I know we have ArkRange but that is tree-sitter specific right?

We probably will want our own typed TextRange (or similar) that we use everywhere

[DavisVaughan]

I'm just now realizing a better name for this is probably really LineRange (TextRange is for a byte range in a document)

The fact that it took me until the test section of the PR to figure this out probably does mean a named wrapper like this would be very helpful

struct LineRange {
  start: usize,
  end: usize,
}

[lionel-]

You're right documenting the contents with a type name would go a long way here.

ArkRange is Ark specific rather thant LSP- or TS-specific. It uses the row/col approach instead of byte offsets.

For byte offsets and ranges we may want to use https://github.com/rust-analyzer/text-size/

But that's storing u32 instead of usize, which makes it harder to interface with the rest of Rust, and we might not need such storage performance.

[lionel-]

These types make it harder to provide generic algorithms, e.g. for fn merge_overlapping<T>(ranges: Vec<std::ops::Range<T>>) -> Vec<std::ops::Range<T>>. Not that we really need the genericity now...

[lionel-]

ah but the ruff types are directly imported from rust-analyzer's text-size crate!

[DavisVaughan]

I dislike that you have to do a (potentially large) redundant allocation on the 1st iteration, which most of the time is all you need to do (i.e. no incomplete and no error)

[DavisVaughan]

Since you only reference &subset below, it does seem like you could add a special case for the first iteration that remapped lines_r directly as subset so you don't need that first allocation

I know that is uglier but it might be worth it

[DavisVaughan]

How about this, with a let mut first = true; outside the loop?

        // Parse within source file to get source references.
        // Avoid allocation on the first iteration, since often there are no issues.
        let srcfile = if first {
            first = false;
            harp::srcref::SrcFile::try_from(&lines_r)?
        } else {
            // Grab all code up to current line
            let subset = &lines_r.slice()[..current_line + 1];
            let subset = CharacterVector::try_from(subset)?;
            harp::srcref::SrcFile::try_from(&subset)?
        };

[lionel-]

With the proposed approach we allocate one big string that is then split by R into a vector of lines (see R implementation of srcfilecopy()).

We could avoid the unnecessary splicing though. I made it:

        // Grab all code up to current line. We don't slice the vector in the
        // first iteration as it's not needed.
        let subset = if current_line == n_lines - 1 {
            lines_r.clone()
        } else {
            CharacterVector::try_from(&lines_r.slice()[..=current_line])?
        };

To support this, CharacterVector is now clonable (it does not deep-copy since none of our wrappers make owning guarantees for the wrapped data).

[DavisVaughan]

I guess there can't be trailing whitespace between incomplete/error and eof?

Like, this adds ranges for lines from [end_last_complete, start_incomplete_or_error], but what about [end_incomplete_or_error, eof]? I guess end_incomplete_or_error == eof?

[lionel-]

I'll make the comment clearer, it's trying to say exactly what you say, the trailing whitespace that needs to be transformed to complete inputs is between the complete expressions and the rest.

[DavisVaughan]

A custom type would also let us add a new() method for something like LineRange::new(0, 1)

[lionel-]

Good point though that's arguably straying away from the standard style.