[Bug Fix] Refactoring of Mark and Span, and bug fix

[Paalon]

• Split Mark and Span to each source files.
• Add methods for iteration and indexing to Span.
• Use those methods:
  • Replace token.span.start_mark to firstmark(token).
  • Replace token.span.end_mark to lastmark(token).
• Bug fix:
  • token.start_mark → firstmark(token)
  • token.end_mark → lastmark(token)

[kescobo]

I've been merging all of the PRs that are small / obviously non-breaking. These big refactoring ones are going to take me a lot longer to get to. @GunnarFarneback if you can take a look, that would be awesome.

Otherwise, just keep pinging me every couple of weeks if I don't get to it. @Paalon if you can also add notes about whether things are internal / part of the API, breaking or not, etc, that would be very helpful. As far as I can tell, this one is just cleaning up internal stuff?

[GunnarFarneback]

I don't see so much value in splitting Mark and Span to own files when they look like integral parts of the Token structs, but on the whole I'm neutral about that change. firstmark and lastmark look fine and I agree about the bug fix.

The part I don't understand is the iteration protocol for Span. It's an uncommon thing to implement for this kind of struct. Do you have specific plans for how to make use of it?

[Paalon]

I defined them only for first and last methods. However the implementation of iteration and indexing is a mimic of Julia base's Pair.

[Paalon]

@kescobo Currently, I only send non-breaking pull requests. But some new feature implementation (especially, replaceable resolvers #172 and constructors #173, they are non-breaking) are needed for further development.

[Paalon]

I try to add more comments in #172 and #173 for easier reviewing.

[GunnarFarneback]

Well, Pair is a kind of mini collection. I don't think that's a very useful model for Span. It seems questionable to set up all the iteration machinery just to be able to write first(span) and last(span) instead of span.start_mark and span.end_mark, especially when they are only used to define firstmark and lastmark.

[kescobo]

Is there any reason we can't just define first and last directly as accessors?

[GunnarFarneback]

We could but it wouldn't match the first and last docstrings to define them explicitly for a non-iterable.

[kescobo]

Fair point. And if this is part of internal machinery anyway, I don't think we should extend a base function in that way. Let's just use the properties explicitly.

[Paalon]

Julia Base exports first(::Any) here

https://github.com/JuliaLang/julia/blob/319d8900dbb2566eef9f7e594e7a1e875dfe819a/base/abstractarray.jl#L454C1-L475C4

"""
    first(coll)

Get the first element of an iterable collection. Return the start point of an
[`AbstractRange`](@ref) even if it is empty.

See also: [`only`](@ref), [`firstindex`](@ref), [`last`](@ref).
"""
function first(itr)
    x = iterate(itr)
    x === nothing && throw(ArgumentError("collection must be non-empty"))
    x[1]
end

So I think any method which equip first should be iterable. Otherwise, we'll lose consistency with Julia Base. The reason I override these methods is

x = iterate(itr)
x === nothing && throw(ArgumentError("collection must be non-empty"))

is not required for Span.

[Paalon]

I think we should avoid more than 2 dots to access a property (e.g. token.span.start_mark).

[Paalon]

If we don't implement first and last for Span,

firstmark(span::Span) = span.start_mark
lastmark(span::Span) = span.end_mark

firstmark(token::Token) = firstmark(token.span)
lastmark(token::Token) = lastmark(token.span)

will be fine.

[GunnarFarneback]

I think you're overestimating the badness of nested property accesses. I do agree that it's nice not to have token.span.start_mark spread out all over the code base, but it's quite clear what it does, so I really don't see that it would be problematic to have the definition

firstmark(token::Token) = token.span.start_mark

If you absolutely want to avoid a two-level property access you could also do

function firstmark(token::Token)
    span = token.span
    span.start_mark
end

but to me it doesn't really improve anything.

Adding a firstmark(::Span) method (or the iteration protocol) just increases the overhead in figuring out how the code works, compared to straightforward property accesses.

[Paalon]

I removed iteration and indexing methods for Span. Now only includes

• Split Mark to its file.
• Split Span to its file.
• Define firstmark(::Token) and lastmark(::Token).
• Use firstmark(::Token) and lastmark(::Token).
  • token.span.start_mark → firstmark(token)
  • token.span.end_mark → lastmark(token)
• Bug fix by
  • token.start_mark → firstmark(token)
  • token.end_mark → lastmark(token)