Make 'from' parameter in TokenList.slice/find inclusive #534
Conversation
| def leading(token: Token): SeqView[Token, IndexedSeq[Token]] = | ||
| tokens.view(0, tok2idx(token)).drop(1).reverse | ||
| tokens.view(0, tok2idx(token)).reverse |
There was a problem hiding this comment.
I believe the goal here was to have the token element dropped from the result. However, this is already handled by the .view method as the end parameter is exclusive
There was a problem hiding this comment.
Woah, this was surprising :D
@ tokens.view(0, tok2idx(token)).reverse.mkString
res19: String = "(list { a object"
@ tokens.view(0, tok2idx(token)).drop(1).reverse.mkString
res20: String = "(list { a object"Yes, the objective was to drop the token.
There was a problem hiding this comment.
val tokens = "foo bar".tokenize.get
tokens: scala.meta.tokens.Tokens = Tokens(, foo, , bar, )
val barIdx = 3
tokens.view(0, barIdx).reverse.mkString("|")
res11: String = " |foo|"
tokens.view(0, barIdx).drop(1).reverse.mkString("|")
res12: String = " |foo"There was a problem hiding this comment.
I see, drop(1) was removing the BOF token, which is wrong behaviour but probably never surfaced as a problem because it's zero length. Good catch!
marcelocenerine
force-pushed
the
slice_from_inclusive
branch
from
b9856fc to
a15c9b0
Compare
There was a problem hiding this comment.
Thank you for this contribution @marcelocenerine ! Everything looks good except the slice signature change 👍
| def leading(token: Token): SeqView[Token, IndexedSeq[Token]] = | ||
| tokens.view(0, tok2idx(token)).drop(1).reverse | ||
| tokens.view(0, tok2idx(token)).reverse |
There was a problem hiding this comment.
Woah, this was surprising :D
@ tokens.view(0, tok2idx(token)).reverse.mkString
res19: String = "(list { a object"
@ tokens.view(0, tok2idx(token)).drop(1).reverse.mkString
res20: String = "(list { a object"Yes, the objective was to drop the token.
| } | ||
| builder.result() | ||
| } | ||
| def slice(from: Token, to: Token): SeqView[Token, IndexedSeq[Token]] = |
There was a problem hiding this comment.
Is this signature change necessary? I think we can keep the signature to avoid a binary breaking change.
There was a problem hiding this comment.
In fact it is not. Given that the method now returns a view, I wanted to make this fact explicit as well as have the return type consistent the leading*/trailing* methods. It wouldn't cause compilation errors but indeed it would break binary compatibility. I'm gonna change it back to Seq to avoid that.
| builder.result() | ||
| } | ||
| def slice(from: Token, to: Token): Seq[Token] = | ||
| tokens.view(tok2idx(from), tok2idx(to)) | ||
|
|
There was a problem hiding this comment.
given that the method no longer says that the returned collection is a view, I was going to change the implementation and delegate to Tokens.slice instead which should be slightly faster as it is backed by an array and does not involve any copying. But then I realized that Tokens.length is buggy, which breaks isEmpty:
"foo".tokenize.get.slice(1, 0).isEmpty
res4: Boolean = false
I'm gonna file an issue or submit a PR to Scalameta.
There was a problem hiding this comment.
iirc someone reported a similar problem with Tokens.length the other day. Length is implemented like this
def length: Int = end - startShould it be this instead?
def length: Int = math.max(0, end - start)There was a problem hiding this comment.
The root cause is in Tokens.slice: scalameta/scalameta#1207
Addresses Make
fromin TokenList.slice(from, to) inclusive #516. The original goal was to do this only forTokenList.slicebut then I realized thatfindwas also inconsistent.findis not used anywhere in the reposliceis used in 2 places:ProcedureSyntax&ExplicitResultTypes. For both of them this change won't cause any harm.Fixes a bug in
TokenList.leadingthat was causing the first element in the token list (usuallyToken.BOF) being incorrectly dropped.