Repository: Microsoft/thrifty (kotlin)

Issue:
Location#path should have relative path to an include-path root 
It seems that `Location#path` is always just `filename.thrift`; this is not very useful.  We'd intended for it to convey the relative position of the file, but from whose perspective?  Not that of the including file, because a file can be included more than once.

It should probably be relative to one of the paths in the include search-path (which includes the compiler's working directory, fyi).  Not sure how exactly to go about this, since we've no tests to verify this behavior and it has likely been broken for a very long time.

Pull request description:
As far as I can tell, Location has always been busted in practice - `base`, intended to be a "root" of some kind, has always been the full path to a file's parent directory, and "path" (intended to be a path relative to base) has just been a file name.

This makes Location little better than a string with line numbers in it - not all that useful.  In this PR, we fix what turns out to be the sole location (outside of tests) where Locations are created.  We do this by, when loading files from disk to parse, cycling through all of our include-roots, looking for the one with the shortest relative distance between the root and the file being loaded.

This isn't bulletproof since thrift files can include relative paths, and so potentially include files that aren't contained in any of our roots.  In that case, we'll keep the current behavior of calling the file's directory the location base.

This also isn't ideal - you could argue that, if a file under root A relatively includes a file under root B, and that included file wouldn't have otherwise been part of the compilation, that maybe A is the more useful attribution.  In the name of implementation simplicity, I disclaim that use case :)

@jparise I'd be grateful for your thoughts, if you've got the time and inclination.

Fixes #237

Patch:
diff --git a/thrifty-schema/src/main/kotlin/com/microsoft/thrifty/schema/Loader.kt b/thrifty-schema/src/main/kotlin/com/microsoft/thrifty/schema/Loader.kt
index c6b4e87c4..3b6c3fe40 100644
--- a/thrifty-schema/src/main/kotlin/com/microsoft/thrifty/schema/Loader.kt
+++ b/thrifty-schema/src/main/kotlin/com/microsoft/thrifty/schema/Loader.kt
@@ -191,8 +191,8 @@ class Loader {
                 return
             }
 
-            dir = file.parent
-            element = loadSingleFile(dir!!, file.fileName) ?: run {
+            dir = findClosestIncludeRoot(file) ?: file.parent!!
+            element = loadSingleFile(dir, dir.relativize(file)) ?: run {
                 val suffix = sourceElement?.let { "\n--> Included from ${it.location.filepath}" } ?: ""
                 throw FileNotFoundException("Failed to locate $path in $includePaths$suffix")
             }
@@ -214,6 +214,25 @@ class Loader {
         }
     }
 
+    private fun findClosestIncludeRoot(path: Path): Path? {
+        var minNameCountRoot: Path? = null
+        var minNameCount = Int.MAX_VALUE
+        for (root in includePaths) {
+            val relative = try {
+                root.relativize(path)
+            } catch (e: IllegalArgumentException) {
+                continue
+            }
+
+            if (relative.nameCount < minNameCount) {
+                minNameCountRoot = root
+                minNameCount = relative.nameCount
+            }
+        }
+
+        return minNameCountRoot
+    }
+
     private fun linkPrograms() {
         synchronized(environment) {
             for (program in loadedPrograms.values) {


Test patch:
diff --git a/thrifty-schema/src/test/kotlin/com/microsoft/thrifty/schema/LoaderTest.kt b/thrifty-schema/src/test/kotlin/com/microsoft/thrifty/schema/LoaderTest.kt
index 3b57e92c2..6f02154e9 100644
--- a/thrifty-schema/src/test/kotlin/com/microsoft/thrifty/schema/LoaderTest.kt
+++ b/thrifty-schema/src/test/kotlin/com/microsoft/thrifty/schema/LoaderTest.kt
@@ -224,7 +224,7 @@ class LoaderTest {
         """
 
         val b = """
-            include '${f1.canonicalPath}'
+            include 'nested/a.thrift'
             namespace java com.microsoft.thrifty.test.crazyIncludes
 
             struct B {
@@ -233,7 +233,7 @@ class LoaderTest {
         """
 
         val c = """
-            include '${f2.canonicalPath}'
+            include 'b.thrift'
 
             namespace java com.microsoft.thrifty.test.crazyIncludes
 
@@ -247,14 +247,15 @@ class LoaderTest {
         f3.writeText(c)
 
         val loader = Loader()
-        loader.addIncludePath(nestedDir.toPath())
-        loader.addThriftFile(f2.toPath())
-        loader.addThriftFile(f3.toPath())
+        loader.addIncludePath(tempDir.toPath())
 
         val schema = loader.load()
 
         schema.structs shouldHaveSize 2
         schema.enums shouldHaveSize 1
+
+        val enum = schema.enums.single()
+        enum.location.path shouldBe "nested/a.thrift"
     }
 
     @Test