luke-gru / riml

Riml is a subset of VimL with some nice added features. It compiles to plain VimL.
MIT License
224 stars 6 forks source link

Namespaces or global scope #4

Closed dsawardekar closed 10 years ago

dsawardekar commented 11 years ago

The viml code generated by riml is quite readable. But there are a few issues with polluting the global function scope.

For instance,

class Foo
end

produces,

function! g:FooConstructor()
endfunction

And the methods of Foo become FooConstructor_method_name. These are also on the global scope. I have a couple of ideas to avoid conflicts with methods defined in other plugins.

First change the scope of functions to be scriptlocal instead, ie:- s:FooConstructor. This needs to a command line option, because it is sometimes beneficial to have the global scoped constructors as well. Eg:- http://github.com/dsawardekar/speckle uses global constructors to dynamically load specs.

My second suggestion is to additionally add a namespace keyword.

namespace 'plugin.utils'

This could compile down to,

function! g:PluginUtilsFooConstructor
endfunction

The plugin is the top level namespace and dots are replaced to use CamelCase. When the compiler finds another namespace declaration that becomes the current namespace, ie:- compiling classes with that prefix instead. Thus avoid any nesting blocks.

This would mitigate any clashes with viml generated code between plugins.

dsawardekar commented 11 years ago

Thinking about this some more, I figured that a basic --ns Foo switch on the command line that prefixes all methods generated by riml would get 99% of the job done. There is no need for fully qualified namespaces.

All the plugin has to do is to pick a unique prefix that is not in use. Classes and methods would get the Foo prefix and so on.

I tried this out a little bit. Currently passing in a hard coded prefix in riml.rb to Compiler.new. And in DefNodeVisitor prefixing the namespace to the declaration.

Unfortunately while renaming the classes and it's method the calls to those rewritten function names does not reflect in call g:Foo() invocations. This is the code it generates. Note that constructor and main method call is incorrect.

" included: 'shape.riml'
function! g:FooShapeConstructor()
  let shapeObj = {}
  let shapeObj.is_shape = 1
  let shapeObj.to_s = function('g:Shape_to_s')
  return shapeObj
endfunction
function! g:FooShape_to_s() dict
  return 'Shape'
endfunction
" included: 'rectangle.riml'
function! g:FooRectangleConstructor(width, height)
  let rectangleObj = {}
  let shapeObj = g:ShapeConstructor()
  call extend(rectangleObj, shapeObj)
  let rectangleObj.width = a:width
  let rectangleObj.height = a:height
  let rectangleObj.to_s = function('g:Rectangle_to_s')
  return rectangleObj
endfunction
function! g:FooRectangle_to_s() dict
  return "Rectangle(" . self.width . "x" . self.height . ")"
endfunction
function! s:Foomain()
  let rectangle = g:RectangleConstructor(10, 10)
  echomsg rectangle.to_s()
endfunction
call s:main()

Can you have a look at this branch? What am I missing?. Thanks.

https://github.com/dsawardekar/riml/compare/compiler-namespace-prefix

luke-gru commented 11 years ago

Hey,

Will look into this tomorrow. I've also created a branch that I'll finish this weekend that defaults class scopes to 's:', but you can add 'g:' if you want. ex:

class g:Shape
...
end

As for namespaces I think it's a good idea and a necessary one. I'll take a deeper dive into it this weekend, and let you know what's missing in your branch.

Thanks!

dsawardekar commented 11 years ago

Thanks @luke-gru. One small request, Can you make the default scope configurable with a --scope switch?

The reason I am asking for this is Speckle uses the default g: scope to figure out what tests to run. It does a :function /SpecConstructor$/ search to find all specs that were sourced into vim. If they were to default to scriptlocal, :s, then :function wouldn't pick up the spec classes, as each spec is sourced from different files.

With an option like --scope g:, I can override the behaviour to be use 'g:Foo' style classes when run during tests. But for a release use --scope s: to make sure conflicts are avoided with other plugins.

dsawardekar commented 11 years ago

@luke-gru I recently discovered that Vim's functions aren't sandboxed as I had earlier thought. Just prefixed with an <SID>. So Speckle :function /SpecConstructor/ search works even if the script sourced was in a different file from the one making the :function call. The g: and s: scopes in viml are purely conventional.

I now think the --scope flag might be overkill. Just switching to s: as the default scope for classes works fine.

luke-gru commented 11 years ago

Hey @dsawardekar, yeah Vim has funny scoping rules, but I've been working on some new features in a branch called class_scopes that I think is ready to be merged. If you want to try it out that would be super helpful.

Basically, it does use the default s: scope for classes now, but there were certain problems that arose from that that I had to work around. For instance, if you have a script-local class, and then want to inherit from that class but make the inherited class global, there were all sorts of issues with <SID>s and whatnot that I had to work around when trying to get that class to work outside of that file. It seems to work fine now, so if you could check it out that would be great.

I'll update the README in that branch right now to reflect the changes that have been made.

EDIT: updated the README :smile:

dsawardekar commented 11 years ago

Checking this out now.

@luke-gru How are you making a release. I couldn't find any package tasks. I imported gem_tasks into the Rakefile to test out in this branch here. It adds rake tasks build, release and install.

dsawardekar commented 11 years ago

@luke-gru I tested out class-scopes on Speckle. I needed to make some changes to use the new g: scope explicitly where I am doing some meta programming with eval, for every thing else things look good. :+1:

I noticed in the generated output you are prefixing methods with an <SID>. I think this should be fine, just need to tweak the meta stuff I'm doing to use the SID function.

Here's the before and after output.

luke-gru commented 10 years ago

Going to close this for now, feel free to reopen or add an issue if you find you still need/want namespaces for classes.

dsawardekar commented 10 years ago

I haven't needed namespaces since the switch to s:. Probably for the best. :)