Implement typed PathPart parsing

[darichey]

Summary & Motivation

A quick attempt at fixing #95.

Backwards-incompatible changes

PathWithInterpol is no longer a Wrapper node.

Further context

This might not be exactly the direction we want to go, but it seemed the most straightforward. We might consider combining StrPart and PathPart considering their overlap (InterpolationPart maybe?).

[darichey]

It's also worth mentioning that, assuming a parse with no errors, the first part is always going to be a valid path which we could parse into a Value::Path. I think it would be nice to expose that, but that would require some bigger refactoring.

[fogti]

It might be a good idea to expose/return an impl Iterator instead of always allocating a vector, given that this internally already just wraps an iterator in a relatively simple way.

[darichey]

I would agree, but I wanted to be consistent with Str::parts. I'd be happy to change them both.

[darichey]

It's also worth mentioning that, assuming a parse with no errors, the first part is always going to be a valid path which we could parse into a Value::Path. I think it would be nice to expose that, but that would require some bigger refactoring.

61c0789 (#96) implements this with some breaking changes. Again, not sure if this is definitely the right way to go, but just trying things out. I'll wait for some feedback before doing more! :)

[fogti]

I would agree, but I wanted to be consistent with Str::parts. I'd be happy to change them both.

If you're able to do that, please do it (lately, I was working on extracting some changes from @oberblastmeister's more-typed-stuff PR, which involved reducing unnecessary memory allocations, and it would be nice to "keep this spirit", because it might also improve downstream performance, and reduce peak memory usage)

[fogti]

this cast should mean that the assert directly above it is unnecessary, right?

[darichey]

Reworked based on @oberblastmeister Path changes in 9df0b11. I noticed that commit makes ast::Path ambiguous (it could either be the token Path or the node Path). Should I try to fix that here?

I also combined StrPart and PathPart into InterpolPart and return an impl Iterator instead of Vec.

@zseri I'm going to hold off on changing Str::parts in this PR since I believe @Ma27 is making changes there rn.

[oberblastmeister]

@darichey You can fix that here. What name should we use?

[darichey]

Sorry, I've been away. I see the rename was done in #134. Is there anything else that needs to be done here?

[oberblastmeister]

@darichey I think InterpolPart should be generic over T. We should have a new node called StrContent or something and Str::parts should return InterpolPart<StrContent>. I think the existing Str::parts methods should be refactored to take an iterator over InterpolPart<StrContent> and return a vector of InterpolPart<String>, or maybe a tuple.

[darichey]

Sorry, I don't understand. Is this what you mean?

node! { #[from(NODE_STRING_CONTENT)] struct StrContent; }

impl StrContent {
    tg! { content, TOKEN_STRING_CONTENT }
}

enum InterpolPart<T> {
  Literal(T)
  Interpolation(super::Interpol)
}

impl ast::Str {
  pub fn parts(&self) -> impl Iterator<Item = InterpolPart<StrContent>> { /* ... */ }
}

// name and location tbd
pub fn parse_parts(impl Iterator<Item = InterpolPart<StrContent>>) -> impl Iterator<Item = InterpolPart<String>> { /* ... */ }

It's not clear to me what is gained here since we have to examine the contents of the inner TOKEN_STRING_CONTENT in ast::Str::parts anyways.

Also, how should we apply the same ideas to paths?

[fogti]

Why do we need the NODE_STRING_CONTENT indirection, when it just contains a token, or is this necessary because of some rowan limitation?

[oberblastmeister]

ast::Path::Parts should return impl Iterator<Item = InterpolPart<PathContent>>

[oberblastmeister]

@darichey @zseri StrContent should probably be a token

[oberblastmeister]

Sorry, I don't understand. Is this what you mean?

Yes. The idea of returning InterpolPart<StrContent> and then converting it to InterpolPart<String> is that the former abstracts the manual casting we have currently in ast::Str::Parts. In addition, we should have an api for accessing the nodes without further parsing just like we have for other nodes.

[darichey]

Got it, I tried implementing that.

It might be a good idea to expose/return an impl Iterator instead of always allocating a vector, given that this internally already just wraps an iterator in a relatively simple way.

@zseri going way back to this, I just realized that I don't think this is possible for string parts, because we need to iterate over all the children first to calculate the common indent. Do you agree?

[fogti]

please omit the .collect(), what the user intends with the iterator shouldn't be a business of the library, if avoidable. (although I understand if we keep it that way to stay symmetric to ast::Str, even tho I don't think that's important, because their function is also different)

[oberblastmeister]

@darichey parts_parsed should be removed, it doesn't add any value unlike the Str variant

[oberblastmeister]

I think normalized_parts is a better name than parts_parsed

[fogti]

@darichey

I just realized that I don't think this is possible for string parts, because we need to iterate over all the children first to calculate the common indent.

yes, correct