Repository: goby-lang/goby
Pull request #845 (2020-04-19 08:31:43)

Document and refactor vm package
This PR is for addressing the 2 feedback I got from #841 

> 1. the vm.New vm.New and v.ExecInstructions methods would profit from some minimal documentation, especially what all the "paths" actually mean. some of them seemed semi obvious..
> 2. same for RegisterExternalClass ExternalClass and vm.Method albeit those are relatively clear



st0012: @yannick can you give this a look?
codecov[bot]: # [Codecov](https://codecov.io/gh/goby-lang/goby/pull/845?src=pr&el=h1) Report
> Merging [#845](https://codecov.io/gh/goby-lang/goby/pull/845?src=pr&el=desc) into [master](https://codecov.io/gh/goby-lang/goby/commit/0fbde9e7fa12b133e517e692851cc79b1d5fd57c&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `55.17%`.

[![Impacted file tree graph](https://codecov.io/gh/goby-lang/goby/pull/845/graphs/tree.svg?width=650&height=150&src=pr&token=giIKD13jMt)](https://codecov.io/gh/goby-lang/goby/pull/845?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #845      +/-   ##
==========================================
+ Coverage   81.11%   81.13%   +0.01%     
==========================================
  Files          54       54              
  Lines        7412     7414       +2     
==========================================
+ Hits         6012     6015       +3     
+ Misses       1178     1177       -1     
  Partials      222      222              
```


| [Impacted Files](https://codecov.io/gh/goby-lang/goby/pull/845?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [vm/class.go](https://codecov.io/gh/goby-lang/goby/pull/845/diff?src=pr&el=tree#diff-dm0vY2xhc3MuZ28=) | `86.20% <0.00%> (ø)` | |
| [vm/vm.go](https://codecov.io/gh/goby-lang/goby/pull/845/diff?src=pr&el=tree#diff-dm0vdm0uZ28=) | `83.75% <65.00%> (+0.67%)` | :arrow_up: |
| [native/db/db.go](https://codecov.io/gh/goby-lang/goby/pull/845/diff?src=pr&el=tree#diff-bmF0aXZlL2RiL2RiLmdv) | `61.00% <100.00%> (ø)` | |
| [native/plugin/plugin.go](https://codecov.io/gh/goby-lang/goby/pull/845/diff?src=pr&el=tree#diff-bmF0aXZlL3BsdWdpbi9wbHVnaW4uZ28=) | `11.96% <100.00%> (ø)` | |
| [native/ripper/ripper.go](https://codecov.io/gh/goby-lang/goby/pull/845/diff?src=pr&el=tree#diff-bmF0aXZlL3JpcHBlci9yaXBwZXIuZ28=) | `73.18% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/goby-lang/goby/pull/845?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/goby-lang/goby/pull/845?src=pr&el=footer). Last update [0fbde9e...495719c](https://codecov.io/gh/goby-lang/goby/pull/845?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/native/db/db.go b/native/db/db.go
index 3457890..6ef598a 100644
--- a/native/db/db.go
+++ b/native/db/db.go
@@ -27,7 +27,7 @@ type (
 )
 
 func init() {
-	vm.RegisterExternalClass("db", vm.ExternalClass("DB", "db.gb",
+	vm.RegisterExternalClass("db", vm.NewExternalClassLoader("DB", "db.gb",
 		// class methods
 		map[string]vm.Method{
 			"get_connection": getConnection,
diff --git a/native/plugin/plugin.go b/native/plugin/plugin.go
index afb554b..65e7dec 100644
--- a/native/plugin/plugin.go
+++ b/native/plugin/plugin.go
@@ -42,7 +42,7 @@ var (
 )
 
 func init() {
-	vm.RegisterExternalClass("plugin", vm.ExternalClass("Plugin", "plugin.gb",
+	vm.RegisterExternalClass("plugin", vm.NewExternalClassLoader("Plugin", "plugin.gb",
 		// class methods
 		map[string]Method{
 			"new": newPlugin,
diff --git a/native/result/bindings.go b/native/result/bindings.go
index 02fb969..70ef195 100644
--- a/native/result/bindings.go
+++ b/native/result/bindings.go
@@ -13,7 +13,7 @@ import (
 
 func init() {
 	vm.RegisterExternalClass(
-		"result", vm.ExternalClass(
+		"result", vm.NewExternalClassLoader(
 			"Result",
 			"result.gb",
 			map[string]vm.Method{
diff --git a/native/ripper/ripper.go b/native/ripper/ripper.go
index 488d7e7..37d2cf4 100644
--- a/native/ripper/ripper.go
+++ b/native/ripper/ripper.go
@@ -203,7 +203,7 @@ func tokenize(receiver Object, sourceLine int, t *Thread, args []Object) Object
 
 // Internal functions ===================================================
 func init() {
-	vm.RegisterExternalClass("ripper", vm.ExternalClass("Ripper", "ripper.gb",
+	vm.RegisterExternalClass("ripper", vm.NewExternalClassLoader("Ripper", "ripper.gb",
 		// class methods
 		map[string]vm.Method{
 			"instruction": instruction,
diff --git a/vm/class.go b/vm/class.go
index 4218da4..e494e8c 100644
--- a/vm/class.go
+++ b/vm/class.go
@@ -38,9 +38,9 @@ var externalClasses = map[string][]ClassLoader{}
 var externalClassLock sync.Mutex
 
 // RegisterExternalClass will add the given class to the global registry of available classes
-func RegisterExternalClass(path string, c ...ClassLoader) {
+func RegisterExternalClass(name string, c ...ClassLoader) {
 	externalClassLock.Lock()
-	externalClasses[path] = c
+	externalClasses[name] = c
 	externalClassLock.Unlock()
 }
 
@@ -57,19 +57,19 @@ func buildMethods(m map[string]Method) []*BuiltinMethodObject {
 	return out
 }
 
-// ExternalClass helps define external go classes
-func ExternalClass(name, path string, classMethods, instanceMethods map[string]Method) ClassLoader {
+// NewExternalClassLoader helps define external go classes by generating a class loader function
+func NewExternalClassLoader(className, libPath string, classMethods, instanceMethods map[string]Method) ClassLoader {
 	return func(v *VM) error {
-		pg := v.initializeClass(name)
+		pg := v.initializeClass(className)
 		pg.setBuiltinMethods(buildMethods(classMethods), true)
 		pg.setBuiltinMethods(buildMethods(instanceMethods), false)
 		v.objectClass.setClassConstant(pg)
 
-		if path == "" {
+		if libPath == "" {
 			return nil
 		}
 
-		return v.mainThread.execGobyLib(path)
+		return v.mainThread.execGobyLib(libPath)
 	}
 }
 
diff --git a/vm/vm.go b/vm/vm.go
index 5adaaf3..f3f86b5 100644
--- a/vm/vm.go
+++ b/vm/vm.go
@@ -63,11 +63,12 @@ type VM struct {
 	fileDir string
 	// args are command line arguments
 	args []string
-	// projectRoot is goby root's absolute path, which is $GOROOT/src/github.com/goby-lang/goby
-	projectRoot string
 
-	// libPath indicates the Goby (.gb) libraries path. Defaults to `<projectRoot>/lib`, unless
-	// DefaultLibPath is specified.
+	// Goby vm uses libPath to find standard libraries written in Goby
+	// the fallback order is:
+	// 1. $GOBY_ROOT/lib
+	// 2. /usr/local/Cellar/goby/VERSION/lib - installed via homebrew
+	// 3. $GOPATH/src/github.com/goby-lang/goby/lib - development environment
 	libPath string
 
 	channelObjectMap *objectMap
@@ -99,33 +100,10 @@ func New(fileDir string, args []string) (vm *VM, e error) {
 	}
 	vm.fileDir = fileDir
 
-	gobyRoot := os.Getenv("GOBY_ROOT")
-
-	if len(gobyRoot) == 0 {
-		vm.projectRoot = fmt.Sprintf("/usr/local/Cellar/goby/%s", Version)
-
-		_, err := os.Stat(vm.projectRoot)
-
-		if err != nil {
-			gp := os.Getenv("GOPATH")
-			path, _ := filepath.Abs(gp + "/src/github.com/goby-lang/goby")
-			_, err = os.Stat(path)
-
-			if err != nil {
-				e = fmt.Errorf("You haven't set $GOBY_ROOT properly")
-				return nil, e
-			}
-
-			vm.projectRoot = path
-		}
-	} else {
-		vm.projectRoot = gobyRoot
-	}
+	err := vm.assignLibPath()
 
-	if DefaultLibPath != "" {
-		vm.libPath = DefaultLibPath
-	} else {
-		vm.libPath = filepath.Join(vm.projectRoot, "lib")
+	if err != nil {
+		return nil, err
 	}
 
 	vm.initConstants()
@@ -150,6 +128,37 @@ func (vm *VM) newThread() (t Thread) {
 	return
 }
 
+// vm.assignLibPath looks up and assigns vm.libPath
+func (vm *VM) assignLibPath() (err error) {
+	if DefaultLibPath != "" {
+		vm.libPath = DefaultLibPath
+		return
+	}
+
+	gobyRoot := os.Getenv("GOBY_ROOT")
+
+	if len(gobyRoot) == 0 {
+		// if GOBY_ROOT is not set, fallback to homebrew's path
+		gobyRoot = fmt.Sprintf("/usr/local/Cellar/goby/%s", Version)
+
+
+		// if it's not installed via homebrew, assume it's in development env and Goby's source is under GOPATH
+		if _, err := os.Stat(gobyRoot); err != nil {
+			path, _ := filepath.Abs(os.Getenv("GOPATH") + "/src/github.com/goby-lang/goby")
+
+			if _, err = os.Stat(path); err != nil {
+				return fmt.Errorf("You haven't set $GOBY_ROOT properly")
+			}
+
+			gobyRoot = path
+		}
+	}
+
+	vm.libPath = filepath.Join(gobyRoot, "lib")
+
+	return
+}
+
 // ExecInstructions accepts a sequence of bytecodes and use vm to evaluate them.
 func (vm *VM) ExecInstructions(sets []*bytecode.InstructionSet, fn string) {
 	translator := newInstructionTranslator(fn)
@@ -172,8 +181,8 @@ func (vm *VM) ExecInstructions(sets []*bytecode.InstructionSet, fn string) {
 	cf.self = vm.mainObj
 	vm.mainThread.callFrameStack.push(cf)
 
-      // here is the final destination of Goby errors at the VM level, and we don't deal with them at this point.
-      // we only decide how the user program should react to them.
+	// here is the final destination of Goby errors at the VM level, and we don't deal with them at this point.
+	// we only decide how the user program should react to them.
 	// at vm level, we don't deal with the error itself
 	// we only decide how the program should react to it
 	defer func() {
@@ -183,8 +192,8 @@ func (vm *VM) ExecInstructions(sets []*bytecode.InstructionSet, fn string) {
 		case error:
 			panic(err)
 
-		// if the error is one of the Goby's errors, such as argument error, we need to handle it depending on the mode of execution.
-		// we need to handle it depends on the type of program execution
+			// if the error is one of the Goby's errors, such as argument error, we need to handle it depending on the mode of execution.
+			// we need to handle it depends on the type of program execution
 		case *Error:
 
 			// REPLMode: We handle the error inside the igb package, so don't need to do anything here
@@ -415,4 +424,4 @@ func (vm *VM) checkArgTypes(args []Object, sourceLine int, types ...string) *Err
 	}
 
 	return nil
-}
\ No newline at end of file
+}


Test patch:
diff --git a/vm/vm_test.go b/vm/vm_test.go
index af67e9f..2a9056b 100644
--- a/vm/vm_test.go
+++ b/vm/vm_test.go
@@ -257,7 +257,7 @@ func TestAutoIncrementLocalVariable(t *testing.T) {
 func TestLoadingGobyLibraryFail(t *testing.T) {
 	vm := initTestVM()
 
-	libFileFullPath := filepath.Join(vm.projectRoot, "lib/_library_not_existing.gb")
+	libFileFullPath := filepath.Join(vm.libPath, "_library_not_existing.gb")
 	expectedErrorMessage := fmt.Sprintf("open %s: no such file or directory", libFileFullPath)
 
 	err := vm.mainThread.execGobyLib("_library_not_existing.gb")