Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash on method names that break regex patterns #12

Merged
merged 1 commit into from
Feb 28, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions stags/src/main/scala/co/pjrt/stags/TagGenerator.scala
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package co.pjrt.stags

import java.io.File

import scala.meta._

import co.pjrt.stags.paths.Path

object TagGenerator {


Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with all the empty new lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the empty lines are remnants of some debug harness that I removed; but didn't catch the empty lines.

/**
* Given a Scalameta Source, generate a sequence of [[Tag]]s for it
*/
Expand Down Expand Up @@ -143,6 +143,7 @@ object TagGenerator {

val static = staticParent || isStatic(scope, mods)
def getFromPat(p: Pat) = getFromPats(scope, mods, p, staticParent, parent)

pat match {
case p: Pat.Var => Seq(patTag(scope, parent, p, static))
case Pat.Typed(p, _) => getFromPat(p)
Expand All @@ -160,6 +161,9 @@ object TagGenerator {
// DESNOTE(2017-07-14, pjrt): This is for patterns like `val 1 = x`
// For those, we should not generate tags (of course)
Seq.empty
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

case _ =>
Console.err.println(s"Error matching $pat in line ${parent.syntax}")
Seq.empty
}
}

Expand Down Expand Up @@ -332,8 +336,8 @@ private object AddressGen {
val line = tree.tokens.syntax.lines.toList(lineWithTagName).trim

val name = tagName.value
val replacement = s"\\\\zs$name"
val nameW = s"\\b$name\\b"
val replacement = s"\\\\zs$name".replaceAllLiterally("$","\\$")
val nameW = s"\\b\\Q$name\\E\\b" // wrap $name with regex quote \Q\E; and word bound \b\b
val search = line.replaceFirst(nameW, replacement)
s"/$search/"
}
Expand Down
27 changes: 27 additions & 0 deletions stags/src/test/scala/co/pjrt/stags/TagGeneratorTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,32 @@ class TagGeneratorTest extends FreeSpec with Matchers {
(abc("SomeThing"), "some", false)
)
}
"regex dangerous methods names should create tags" in {
val testFile =
"""
|package a.b.c
|object DangerMethods {
| def *(v:Int): Int = ???
| def **(v:Int): Int = ???
| def ***(v:Int): Int = ???
| def ?(v:Int):Boolean = ???
| def ^(v:Int):Boolean = ???
| def question$: Int = ???
| def x$u: Int = ???
|}
""".stripMargin

testFile ~> Seq(
(abc(), "DangerMethods", false),
(abc("DangerMethods"), "*", false),
(abc("DangerMethods"), "**", false),
(abc("DangerMethods"), "***", false),
(abc("DangerMethods"), "?", false),
(abc("DangerMethods"), "^", false),
(abc("DangerMethods"), "question$", false),
(abc("DangerMethods"), "x$u", false)
)
}

"Decl defs should create tags" in {
val testFile =
Expand All @@ -353,6 +379,7 @@ class TagGeneratorTest extends FreeSpec with Matchers {
)
}


"should ignore @annotations" in {
val testFile =
"""
Expand Down
5 changes: 3 additions & 2 deletions stags/src/test/scala/co/pjrt/stags/TagLineTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,9 @@ class TagLineTest extends FreeSpec with Matchers {
TagLine(tag, filePath)
.relativize(target)
.filePath
.toString shouldBe expectedS

.toString
.replaceAllLiterally("\\","/") //windows path sep is backwards
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that, I don't get why this was needed. Are u running this test on windows and it is failing?

Copy link
Contributor Author

@drxcc drxcc Feb 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing this on a Windows machine. In order to make the test work, I had to make the backward paths match the tests specified in a unix style path.

the replaceAllLiterally is just wedged in before the shouldBe .

.shouldBe(expectedS)
}

"should modify the filepath to be relative from the target in" - {
Expand Down