Further refactoring of scope/context mechanisms. Mostly related to the new FileContext class. Not quite there yet, but almost

openscad.pro

@@ -216,6 +216,7 @@ HEADERS += src/typedefs.h \
            src/function.h \
            src/grid.h \
            src/highlighter.h \
+           src/localscope.h \
            src/module.h \
            src/node.h \
            src/csgnode.h \
@@ -272,6 +273,7 @@ SOURCES += src/version_check.cc \
            src/value.cc \
            src/expr.cc \
            src/func.cc \
+           src/localscope.cc \
            src/module.cc \
            src/node.cc \
            src/context.cc \

src/MainWindow.h

@@ -29,7 +29,7 @@ public:
 	QTimer *autoReloadTimer;
 	std::string autoReloadId;
 
-	ModuleContext root_ctx;
+	ModuleContext top_ctx;
 	FileModule *root_module;      // Result of parsing
 	ModuleInstantiation root_inst;    // Top level instance
 	AbstractNode *absolute_root_node; // Result of tree evaluation

src/builtin.cc

@@ -16,12 +16,12 @@ Builtins *Builtins::instance(bool erase)
 
 void Builtins::init(const char *name, class AbstractModule *module)
 {
-	Builtins::instance()->rootmodule.scope.modules[name] = module;
+	Builtins::instance()->globalscope.modules[name] = module;
 }
 
 void Builtins::init(const char *name, class AbstractFunction *function)
 {
-	Builtins::instance()->rootmodule.scope.functions[name] = function;
+	Builtins::instance()->globalscope.functions[name] = function;
 }
 
 extern void register_builtin_functions();
@@ -80,18 +80,18 @@ std::string Builtins::isDeprecated(const std::string &name)
 
 Builtins::Builtins()
 {
-	this->rootmodule.scope.assignments.push_back(Assignment("$fn", new Expression(Value(0.0))));
-	this->rootmodule.scope.assignments.push_back(Assignment("$fs", new Expression(Value(2.0))));
-	this->rootmodule.scope.assignments.push_back(Assignment("$fa", new Expression(Value(12.0))));
-	this->rootmodule.scope.assignments.push_back(Assignment("$t", new Expression(Value(0.0))));
+	this->globalscope.assignments.push_back(Assignment("$fn", new Expression(Value(0.0))));
+	this->globalscope.assignments.push_back(Assignment("$fs", new Expression(Value(2.0))));
+	this->globalscope.assignments.push_back(Assignment("$fa", new Expression(Value(12.0))));
+	this->globalscope.assignments.push_back(Assignment("$t", new Expression(Value(0.0))));
 
 	Value::VectorType zero3;
 	zero3.push_back(Value(0.0));
 	zero3.push_back(Value(0.0));
 	zero3.push_back(Value(0.0));
 	Value zero3val(zero3);
-	this->rootmodule.scope.assignments.push_back(Assignment("$vpt", new Expression(zero3val)));
-	this->rootmodule.scope.assignments.push_back(Assignment("$vpr", new Expression(zero3val)));
+	this->globalscope.assignments.push_back(Assignment("$vpt", new Expression(zero3val)));
+	this->globalscope.assignments.push_back(Assignment("$vpr", new Expression(zero3val)));
 }
 
 Builtins::~Builtins()

src/builtin.h

@@ -4,6 +4,7 @@
 #include <string>
 #include <boost/unordered_map.hpp>
 #include "module.h"
+#include "localscope.h"
 
 class Builtins
 {
@@ -17,18 +18,13 @@ public:
 	void initialize();
 	std::string isDeprecated(const std::string &name);
 
-	const FunctionContainer &functions() { return this->builtinfunctions; }
-	const ModuleContainer &modules() { return this->builtinmodules; }
-
-	const Module &getRootModule() { return this->rootmodule; }
+	const LocalScope &getGlobalScope() { return this->globalscope; }
 
 private:
 	Builtins();
 	~Builtins();
 
-	Module rootmodule;
-	FunctionContainer builtinfunctions;
-	ModuleContainer builtinmodules;
+	LocalScope globalscope;
 
 	boost::unordered_map<std::string, std::string> deprecations;
 };

src/context.h

@@ -39,7 +39,7 @@ protected:
 	ValueMap variables;
 	ValueMap config_variables;
 
-	std::string document_path;
+	std::string document_path; // FIXME: This is a remnant only needed by dxfdim
 
 #ifdef DEBUG
 public:

src/localscope.cc

@@ -0,0 +1,67 @@
+#include "localscope.h"
+#include "modcontext.h"
+#include "module.h"
+#include "typedefs.h"
+#include "expression.h"
+#include "function.h"
+
+#include <boost/foreach.hpp>
+
+LocalScope::LocalScope()
+{
+}
+
+LocalScope::~LocalScope()
+{
+	BOOST_FOREACH (ModuleInstantiation *v, children) delete v;
+	BOOST_FOREACH (const Assignment &v, assignments) delete v.second;
+	BOOST_FOREACH (FunctionContainer::value_type &f, functions) delete f.second;
+	BOOST_FOREACH (AbstractModuleContainer::value_type &m, modules) delete m.second;
+}
+
+std::string LocalScope::dump(const std::string &indent) const
+{
+	std::stringstream dump;
+	BOOST_FOREACH(const FunctionContainer::value_type &f, this->functions) {
+		dump << f.second->dump(indent, f.first);
+	}
+	BOOST_FOREACH(const AbstractModuleContainer::value_type &m, this->modules) {
+		dump << m.second->dump(indent, m.first);
+	}
+	BOOST_FOREACH(const Assignment &ass, this->assignments) {
+		dump << indent << ass.first << " = " << *ass.second << ";\n";
+	}
+	BOOST_FOREACH(const ModuleInstantiation *inst, this->children) {
+		dump << inst->dump(indent);
+	}
+	return dump.str();
+}
+
+// FIXME: Two parameters here is a hack. Rather have separate types of scopes, or check the type of the first parameter. Note const vs. non-const
+std::vector<AbstractNode*> LocalScope::instantiateChildren(const Context *evalctx, FileContext *filectx) const
+{
+	Context *c = filectx;
+
+	if (!c) {
+		c = new Context(evalctx);
+
+		// FIXME: If we make c a ModuleContext, child() doesn't work anymore
+		// c->functions_p = &this->functions;
+		// c->modules_p = &this->modules;
+
+		BOOST_FOREACH (const Assignment &ass, this->assignments) {
+			c->set_variable(ass.first, ass.second->evaluate(c));
+		}
+	}
+
+	std::vector<AbstractNode*> childnodes;
+	BOOST_FOREACH (ModuleInstantiation *modinst, this->children) {
+		AbstractNode *node = modinst->evaluate(c);
+		if (node) childnodes.push_back(node);
+	}
+
+	if (c != filectx) delete c;
+
+	return childnodes;
+}
+

src/localscope.h

@@ -12,7 +12,7 @@ public:
 
 	size_t numElements() const { return assignments.size() + children.size(); }
 	std::string dump(const std::string &indent) const;
-	std::vector<class AbstractNode*> instantiateChildren(const class Context *evalctx) const;
+	std::vector<class AbstractNode*> instantiateChildren(const class Context *evalctx, class FileContext *filectx = NULL) const;
 
 	AssignmentList assignments;
   ModuleInstantiationList children;

src/mainwin.cc

@@ -168,7 +168,7 @@ MainWindow::MainWindow(const QString &filename)
 					this, SLOT(actionRenderCGALDone(CGAL_Nef_polyhedron *)));
 #endif
 
-	root_ctx.registerBuiltin();
+	top_ctx.registerBuiltin();
 
 	this->openglbox = NULL;
 	root_module = NULL;
@@ -506,7 +506,7 @@ MainWindow::setFileName(const QString &filename)
 {
 	if (filename.isEmpty()) {
 		this->fileName.clear();
-		this->root_ctx.setDocumentPath(currentdir);
+		this->top_ctx.setDocumentPath(currentdir);
 		setWindowTitle("OpenSCAD - New Document[*]");
 	}
 	else {
@@ -522,7 +522,7 @@ MainWindow::setFileName(const QString &filename)
 			this->fileName = fileinfo.fileName();
 		}
 		
-		this->root_ctx.setDocumentPath(fileinfo.dir().absolutePath().toLocal8Bit().constData());
+		this->top_ctx.setDocumentPath(fileinfo.dir().absolutePath().toLocal8Bit().constData());
 		QDir::setCurrent(fileinfo.dir().absolutePath());
 	}
 
@@ -644,7 +644,7 @@ bool MainWindow::compile(bool reload, bool procevents)
 		
 		AbstractNode::resetIndexCounter();
 		this->root_inst = ModuleInstantiation("group");
-		this->absolute_root_node = this->root_module->instantiate(&this->root_ctx, &this->root_inst, NULL);
+		this->absolute_root_node = this->root_module->instantiate(&top_ctx, &this->root_inst, NULL);
 		
 		if (this->absolute_root_node) {
 			// Do we have an explicit root node (! modifier)?
@@ -979,19 +979,19 @@ void MainWindow::pasteViewportRotation()
 
 void MainWindow::updateTemporalVariables()
 {
-	this->root_ctx.set_variable("$t", Value(this->e_tval->text().toDouble()));
+	this->top_ctx.set_variable("$t", Value(this->e_tval->text().toDouble()));
 	
 	Value::VectorType vpt;
 	vpt.push_back(Value(-qglview->cam.object_trans.x()));
 	vpt.push_back(Value(-qglview->cam.object_trans.y()));
 	vpt.push_back(Value(-qglview->cam.object_trans.z()));
-	this->root_ctx.set_variable("$vpt", Value(vpt));
+	this->top_ctx.set_variable("$vpt", Value(vpt));
 	
 	Value::VectorType vpr;
 	vpr.push_back(Value(fmodf(360 - qglview->cam.object_rot.x() + 90, 360)));
 	vpr.push_back(Value(fmodf(360 - qglview->cam.object_rot.y(), 360)));
 	vpr.push_back(Value(fmodf(360 - qglview->cam.object_rot.z(), 360)));
-	root_ctx.set_variable("$vpr", Value(vpr));
+	top_ctx.set_variable("$vpr", Value(vpr));
 }
 
 bool MainWindow::fileChangedOnDisk()

src/modcontext.cc

@@ -7,28 +7,18 @@
 
 #include <boost/foreach.hpp>
 
-ModuleContext::ModuleContext(const class Module *module, const Context *parent, const EvalContext *evalctx)
-	: Context(parent), functions_p(NULL), modules_p(NULL), usedlibs_p(NULL)
+ModuleContext::ModuleContext(const Context *parent, const EvalContext *evalctx)
+	: Context(parent), functions_p(NULL), modules_p(NULL), evalctx(evalctx)
 {
-	if (module) setModule(*module, evalctx);
 }
 
 ModuleContext::~ModuleContext()
 {
 }
 
-void ModuleContext::setModule(const Module &module, const EvalContext *evalctx)
+void ModuleContext::initializeModule(const class Module &module)
 {
 	this->setVariables(module.definition_arguments, evalctx);
-	this->evalctx = evalctx;
-
-  // FIXME: Hack - split out file context into separate class?
-	const FileModule *fm = dynamic_cast<const FileModule*>(&module);
-	if (fm) {
-		this->usedlibs_p = &(fm->usedlibs);
-		if (!fm->modulePath().empty()) this->document_path = fm->modulePath();
-	}
-	
 	// FIXME: Don't access module members directly
 	this->functions_p = &module.scope.functions;
 	this->modules_p = &module.scope.modules;
@@ -37,17 +27,6 @@ void ModuleContext::setModule(const Module &module, const EvalContext *evalctx)
 	}
 }
 
-/*!
-	Only used to initialize builtins for the top-level root context
-*/
-void ModuleContext::registerBuiltin()
-{
-	// FIXME: built-ins only contains variables, setModule isn't really needed for this
-  // FIXME: Where to put set_variable from setModule?
-	this->setModule(Builtins::instance()->getRootModule());
-	this->set_constant("PI",Value(M_PI));
-}
-
 class RecursionGuard
 {
 public:
@@ -61,6 +40,45 @@ private:
 	const std::string &name;
 };
 
+
+/*!
+	Only used to initialize builtins for the top-level root context
+*/
+void ModuleContext::registerBuiltin()
+{
+	const LocalScope &scope = Builtins::instance()->getGlobalScope();
+
+	// FIXME: Don't access module members directly
+	this->functions_p = &scope.functions;
+	this->modules_p = &scope.modules;
+	BOOST_FOREACH(const Assignment &ass, scope.assignments) {
+		this->set_variable(ass.first, ass.second->evaluate(this));
+	}
+
+	this->set_constant("PI",Value(M_PI));
+}
+
+const AbstractFunction *ModuleContext::findLocalFunction(const std::string &name) const
+{
+	if (this->functions_p && this->functions_p->find(name) != this->functions_p->end()) {
+		return this->functions_p->find(name)->second;
+	}
+	return NULL;
+}
+
+const AbstractModule *ModuleContext::findLocalModule(const std::string &name) const
+{
+	if (this->modules_p && this->modules_p->find(name) != this->modules_p->end()) {
+		AbstractModule *m = this->modules_p->find(name)->second;
+		std::string replacement = Builtins::instance()->isDeprecated(name);
+		if (!replacement.empty()) {
+			PRINTB("DEPRECATED: The %s() module will be removed in future releases. Use %s() instead.", name % replacement);
+		}
+		return m;
+	}
+	return NULL;
+}
+
 Value ModuleContext::evaluate_function(const std::string &name, const EvalContext *evalctx) const
 {
 	RecursionGuard g(*this, name);
@@ -69,51 +87,16 @@ Value ModuleContext::evaluate_function(const std::string &name, const EvalContex
 		return Value();
 	}
 
-	if (this->functions_p && this->functions_p->find(name) != this->functions_p->end()) {
-		return this->functions_p->find(name)->second->evaluate(this, evalctx);
-	}
-	
-	if (this->usedlibs_p) {
-		BOOST_FOREACH(const FileModule::ModuleContainer::value_type &m, *this->usedlibs_p) {
-			if (m.second->scope.functions.find(name) != m.second->scope.functions.end()) {
-				ModuleContext ctx(m.second, this->parent);
-				// FIXME: Set document path
-#if 0 && DEBUG
-				PRINTB("New lib Context for %s func:", name);
-				ctx.dump(NULL, NULL);
-#endif
-				return m.second->scope.functions[name]->evaluate(&ctx, evalctx);
-			}
-		}
-	}
+	const AbstractFunction *foundf = findLocalFunction(name);
+	if (foundf) return foundf->evaluate(this, evalctx);
+
 	return Context::evaluate_function(name, evalctx);
 }
 
 AbstractNode *ModuleContext::instantiate_module(const ModuleInstantiation &inst, const EvalContext *evalctx) const
 {
-	if (this->modules_p && this->modules_p->find(inst.name()) != this->modules_p->end()) {
-		AbstractModule *m = this->modules_p->find(inst.name())->second;
-		std::string replacement = Builtins::instance()->isDeprecated(inst.name());
-		if (!replacement.empty()) {
-			PRINTB("DEPRECATED: The %s() module will be removed in future releases. Use %s() instead.", inst.name() % replacement);
-		}
-		return m->instantiate(this, &inst, evalctx);
-	}
-
-	if (this->usedlibs_p) {
-		BOOST_FOREACH(const FileModule::ModuleContainer::value_type &m, *this->usedlibs_p) {
-			assert(m.second);
-			if (m.second->scope.modules.find(inst.name()) != m.second->scope.modules.end()) {
-				ModuleContext ctx(m.second, this->parent);
-				// FIXME: Set document path
-#if 0 && DEBUG
-				PRINT("New lib Context:");
-				ctx.dump(NULL, &inst);
-#endif
-				return m.second->scope.modules[inst.name()]->instantiate(&ctx, &inst, evalctx);
-			}
-		}
-	}
+	const AbstractModule *foundm = this->findLocalModule(inst.name());
+	if (foundm) return foundm->instantiate(this, &inst, evalctx);
 
 	return Context::instantiate_module(inst, evalctx);
 }
@@ -150,10 +133,57 @@ void ModuleContext::dump(const AbstractModule *mod, const ModuleInstantiation *i
 }
 #endif
 
-FileContext::FileContext(const class FileModule &module, 
-												 const Context *parent, const EvalContext *evalctx)
-	:ModuleContext(&module, parent, evalctx)
+FileContext::FileContext(const class FileModule &module, const Context *parent)
+	: usedlibs(module.usedlibs), ModuleContext(parent)
 {
-	
+	if (!module.modulePath().empty()) this->document_path = module.modulePath();
 }
 
+Value FileContext::evaluate_function(const std::string &name, const EvalContext *evalctx) const
+{
+	RecursionGuard g(*this, name);
+	if (g.recursion_detected()) { 
+		PRINTB("Recursion detected calling function '%s'", name);
+		return Value();
+	}
+
+	const AbstractFunction *foundf = findLocalFunction(name);
+	if (foundf) return foundf->evaluate(this, evalctx);
+	
+	BOOST_FOREACH(const FileModule::ModuleContainer::value_type &m, this->usedlibs) {
+		if (m.second->scope.functions.find(name) != m.second->scope.functions.end()) {
+			FileContext ctx(*m.second, this->parent);
+			ctx.initializeModule(*m.second);
+			// FIXME: Set document path
+#if 0 && DEBUG
+			PRINTB("New lib Context for %s func:", name);
+			ctx.dump(NULL, NULL);
+#endif
+			return m.second->scope.functions[name]->evaluate(&ctx, evalctx);
+		}
+	}
+
+	return ModuleContext::evaluate_function(name, evalctx);
+}
+
+AbstractNode *FileContext::instantiate_module(const ModuleInstantiation &inst, const EvalContext *evalctx) const
+{
+	const AbstractModule *foundm = this->findLocalModule(inst.name());
+	if (foundm) return foundm->instantiate(this, &inst, evalctx);
+
+	BOOST_FOREACH(const FileModule::ModuleContainer::value_type &m, this->usedlibs) {
+		assert(m.second);
+		if (m.second->scope.modules.find(inst.name()) != m.second->scope.modules.end()) {
+			FileContext ctx(*m.second, this->parent);
+			ctx.initializeModule(*m.second);
+			// FIXME: Set document path
+#if 0 && DEBUG
+			PRINT("New file Context:");
+			ctx.dump(NULL, &inst);
+#endif
+			return m.second->scope.modules[inst.name()]->instantiate(&ctx, &inst, evalctx);
+		}
+	}
+
+	return ModuleContext::instantiate_module(inst, evalctx);
+}

src/modcontext.h

@@ -14,18 +14,21 @@
 class ModuleContext : public Context
 {
 public:
-	ModuleContext(const class Module *module = NULL, const Context *parent = NULL, const EvalContext *evalctx = NULL);
+	ModuleContext(const Context *parent = NULL, const EvalContext *evalctx = NULL);
 	virtual ~ModuleContext();
 
-	void setModule(const Module &module, const EvalContext *evalctx = NULL);
+	void initializeModule(const Module &m);
 	void registerBuiltin();
+	virtual Value evaluate_function(const std::string &name, 
+																	const EvalContext *evalctx) const;
+	virtual AbstractNode *instantiate_module(const ModuleInstantiation &inst, 
+																					 const EvalContext *evalctx) const;
 
-	virtual Value evaluate_function(const std::string &name, const EvalContext *evalctx) const;
-	virtual AbstractNode *instantiate_module(const ModuleInstantiation &inst, const EvalContext *evalctx) const;
+	const AbstractModule *findLocalModule(const std::string &name) const;
+	const AbstractFunction *findLocalFunction(const std::string &name) const;
 
-	const boost::unordered_map<std::string, class AbstractFunction*> *functions_p;
-	const boost::unordered_map<std::string, class AbstractModule*> *modules_p;
-	const FileModule::ModuleContainer *usedlibs_p;
+	const LocalScope::FunctionContainer *functions_p;
+	const LocalScope::AbstractModuleContainer *modules_p;
 
   // FIXME: Points to the eval context for the call to this module. Not sure where it belongs
 	const class EvalContext *evalctx;
@@ -40,8 +43,14 @@ public:
 class FileContext : public ModuleContext
 {
 public:
-	FileContext(const class FileModule &module, const Context *parent = NULL, const EvalContext *evalctx = NULL);
+	FileContext(const class FileModule &module, const Context *parent);
 	virtual ~FileContext() {}
+	virtual Value evaluate_function(const std::string &name, const EvalContext *evalctx) const;
+	virtual AbstractNode *instantiate_module(const ModuleInstantiation &inst, 
+																					 const EvalContext *evalctx) const;
+
+private:
+	const FileModule::ModuleContainer &usedlibs;
 };
 
 #endif

src/module.cc

@@ -40,50 +40,6 @@ namespace fs = boost::filesystem;
 #include <sstream>
 #include <sys/stat.h>
 
-LocalScope::LocalScope()
-{
-}
-
-LocalScope::~LocalScope()
-{
-	BOOST_FOREACH(ModuleInstantiation *v, children) delete v;
-	BOOST_FOREACH (const Assignment &v, assignments) delete v.second;
-	BOOST_FOREACH (FunctionContainer::value_type &f, functions) delete f.second;
-	BOOST_FOREACH (AbstractModuleContainer::value_type &m, modules) delete m.second;
-}
-
-std::string LocalScope::dump(const std::string &indent) const
-{
-	std::stringstream dump;
-	BOOST_FOREACH(const FunctionContainer::value_type &f, this->functions) {
-		dump << f.second->dump(indent, f.first);
-	}
-	BOOST_FOREACH(const AbstractModuleContainer::value_type &m, this->modules) {
-		dump << m.second->dump(indent, m.first);
-	}
-	BOOST_FOREACH(const Assignment &ass, this->assignments) {
-		dump << indent << ass.first << " = " << *ass.second << ";\n";
-	}
-	BOOST_FOREACH(const ModuleInstantiation *inst, this->children) {
-		dump << inst->dump(indent);
-	}
-	return dump.str();
-}
-
-std::vector<AbstractNode*> LocalScope::instantiateChildren(const Context *evalctx) const
-{
-	Context c(evalctx); // FIXME: Is this correct, or should we use the parent?
-	BOOST_FOREACH (const Assignment &ass, this->assignments) {
-		c.set_variable(ass.first, ass.second->evaluate(&c));
-	}
-	std::vector<AbstractNode*> childnodes;
-	BOOST_FOREACH (ModuleInstantiation *modinst, this->children) {
-		AbstractNode *node = modinst->evaluate(&c);
-		if (node) childnodes.push_back(node);
-	}
-	return childnodes;
-}
-
 AbstractModule::~AbstractModule()
 {
 }
@@ -183,14 +139,14 @@ void Module::addChild(ModuleInstantiation *ch)
 
 AbstractNode *Module::instantiate(const Context *ctx, const ModuleInstantiation *inst, const EvalContext *evalctx) const
 {
-	ModuleContext c(this, ctx, evalctx);
-	// FIXME: Set document path to the path of the module
+	ModuleContext c(ctx, evalctx);
+	c.initializeModule(*this);
 	c.set_variable("$children", Value(double(inst->scope.children.size())));
+	// FIXME: Set document path to the path of the module
 #if 0 && DEBUG
 	c.dump(this, inst);
 #endif
 
-	// FIXME: this->scope.instantiateChildren(&c) and ModuleContext c() causes set_variable to be called twice, causing duplicate warning output in e.g. echotest_search-tests
 	AbstractNode *node = new AbstractNode(inst);
 	std::vector<AbstractNode *> instantiatednodes = this->scope.instantiateChildren(&c);
 	node->children.insert(node->children.end(), instantiatednodes.begin(), instantiatednodes.end());
@@ -259,3 +215,20 @@ bool FileModule::handleDependencies()
 	this->is_handling_dependencies = false;
 	return changed;
 }
+
+AbstractNode *FileModule::instantiate(const Context *ctx, const ModuleInstantiation *inst, const EvalContext *evalctx) const
+{
+	assert(evalctx == NULL);
+	FileContext c(*this, ctx);
+	c.initializeModule(*this);
+	// FIXME: Set document path to the path of the module
+#if 0 && DEBUG
+	c.dump(this, inst);
+#endif
+
+	AbstractNode *node = new AbstractNode(inst);
+	std::vector<AbstractNode *> instantiatednodes = this->scope.instantiateChildren(ctx, &c);
+	node->children.insert(node->children.end(), instantiatednodes.begin(), instantiatednodes.end());
+
+	return node;
+}

src/module.h

@@ -65,7 +65,7 @@ public:
 	Module() { }
 	virtual ~Module();
 
-	virtual AbstractNode *instantiate(const Context *ctx, const ModuleInstantiation *inst, const EvalContext *evalctx) const;
+	virtual AbstractNode *instantiate(const Context *ctx, const ModuleInstantiation *inst, const EvalContext *evalctx = NULL) const;
 	virtual std::string dump(const std::string &indent, const std::string &name) const;
 
 	void addChild(ModuleInstantiation *ch);
@@ -75,6 +75,8 @@ public:
 	LocalScope scope;
 };
 
+// FIXME: A FileModule doesn't have definition arguments, so we shouldn't really
+// inherit from a Module
 class FileModule : public Module
 {
 public:
@@ -85,6 +87,7 @@ public:
 	const std::string &modulePath() const { return this->path; }
 	void registerInclude(const std::string &filename);
 	bool handleDependencies();
+	virtual AbstractNode *instantiate(const Context *ctx, const ModuleInstantiation *inst, const EvalContext *evalctx = NULL) const;
 
 	typedef boost::unordered_map<std::string, class FileModule*> ModuleContainer;
 	ModuleContainer usedlibs;

src/openscad.cc

@@ -327,6 +327,14 @@ int main(int argc, char **argv)
 
 		if (!filename) help(argv[0]);
 
+		// Top context - this context only holds builtins
+		ModuleContext top_ctx;
+		top_ctx.registerBuiltin();
+		PRINT("Root Context:");
+#if 0 && DEBUG
+		top_ctx.dump(NULL, NULL);
+#endif
+
 		FileModule *root_module;
 		ModuleInstantiation root_inst("group");
 		AbstractNode *root_node;
@@ -348,18 +356,12 @@ int main(int argc, char **argv)
 		if (!root_module) exit(1);
 		root_module->handleDependencies();
 		
-		ModuleContext root_ctx;
-		root_ctx.registerBuiltin();
-		PRINT("Root Context:");
-#if 0 && DEBUG
-		root_ctx.dump(NULL, NULL);
-#endif
 		fs::path fpath = boosty::absolute(fs::path(filename));
 		fs::path fparent = fpath.parent_path();
 		fs::current_path(fparent);
 
 		AbstractNode::resetIndexCounter();
-		absolute_root_node = root_module->instantiate(&root_ctx, &root_inst, NULL);
+		absolute_root_node = root_module->instantiate(&top_ctx, &root_inst, NULL);
 
 		// Do we have an explicit root node (! modifier)?
 		if (!(root_node = find_root_tag(absolute_root_node)))

testdata/scad/features/child-tests.scad

@@ -1,7 +1,7 @@
 $fn=16;
 
-module parent() {
-  for (i=[0:2]) {
+module parent(range=[0:2]) {
+  for (i=range) {
     translate([2.5*i,0,0]) child(i);
   }
 }
@@ -32,3 +32,6 @@ module parent3() {
 }
 
 translate([5,3,0]) parent3() { cube(); sphere(); }
+
+// Leaking variables to child list is not allowed
+translate([0,6,0]) parent(range=[0:1], testvar=10) { sphere(); cube(testvar, center=true);}

testdata/scad/misc/variable-scope-sub.scad

@@ -0,0 +1,24 @@
+sub_global = 15;
+
+module submodule() {
+  echo($children);
+  echo(submodule_var);
+  submodule_var = 16;
+  module subsubmodule() {
+    echo($children);
+    subsubmodule_var = 17;
+    echo(subsubmodule_var);
+    child(0);
+  }
+  subsubmodule() {child(0); sphere();}
+}
+
+module submodule2() {
+  echo(sub_global);
+  echo($children);
+}
+
+module submain() {
+  echo(global_var); // Undefined global var
+  submodule() {submodule2() sphere(); cube();}
+}

testdata/scad/misc/variable-scope-tests.scad

@@ -0,0 +1,53 @@
+echo("special variable inheritance");
+module special_module(a) {
+  echo(a, $fn);
+  special_module2(a);
+}
+
+module special_module2(b) {
+  echo(a);
+  echo(b, $fn);
+}
+
+special_module(23, $fn=5);
+
+echo("inner variables shadows parameter");
+module inner_variables(a, b) {
+  b = 24;
+  echo(a, b);
+}
+
+inner_variables(5, 6);
+
+echo("user-defined special variables as parameter");
+module user_defined_special($b) {
+  echo($b);
+  user_defined_special2();
+}
+
+module user_defined_special2() {
+  echo($b);
+}
+
+user_defined_special(7);
+
+echo("assign only visible in children's scope");
+module assigning() {
+  echo(c);
+}
+
+module assigning2(c) {
+  echo(c);
+}
+
+assign(c=5) {
+  assigning();
+  assigning2(c);
+}
+
+echo("undeclared variable can still be passed and used");
+module undeclared_var() {
+  echo(d);
+}
+
+undeclared_var(d=6);

tests/CMakeLists.txt

@@ -402,6 +402,7 @@ set(CORE_SOURCES
   ../src/value.cc 
   ../src/expr.cc 
   ../src/func.cc 
+  ../src/localscope.cc 
   ../src/module.cc 
   ../src/ModuleCache.cc 
   ../src/node.cc 
@@ -751,7 +752,8 @@ list(APPEND ECHO_FILES ${FUNCTION_FILES}
             ${CMAKE_SOURCE_DIR}/../testdata/scad/misc/vector-values.scad
             ${CMAKE_SOURCE_DIR}/../testdata/scad/misc/search-tests.scad
             ${CMAKE_SOURCE_DIR}/../testdata/scad/misc/recursion-tests.scad
-            ${CMAKE_SOURCE_DIR}/../testdata/scad/misc/value-reassignment-tests.scad)
+            ${CMAKE_SOURCE_DIR}/../testdata/scad/misc/value-reassignment-tests.scad
+            ${CMAKE_SOURCE_DIR}/../testdata/scad/misc/variable-scope-tests.scad)
 
 list(APPEND DUMPTEST_FILES ${MINIMAL_FILES} ${FEATURES_FILES} ${EXAMPLE_FILES})
 list(APPEND DUMPTEST_FILES ${CMAKE_SOURCE_DIR}/../testdata/scad/misc/escape-test.scad

tests/cgalcachetest.cc

@@ -129,10 +129,10 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 
 	root_module = parsefile(filename);
@@ -145,7 +145,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	AbstractNode *absolute_root_node = root_module->instantiate(&root_ctx, &root_inst);
+	AbstractNode *absolute_root_node = root_module->instantiate(&top_ctx, &root_inst);
 	AbstractNode *root_node;
 	// Do we have an explicit root node (! modifier)?
 	if (!(root_node = find_root_tag(absolute_root_node))) root_node = absolute_root_node;

tests/cgalpngtest.cc

@@ -102,10 +102,10 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 
 	root_module = parsefile(filename);
@@ -118,7 +118,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	AbstractNode *absolute_root_node = root_module->instantiate(&root_ctx, &root_inst);
+	AbstractNode *absolute_root_node = root_module->instantiate(&top_ctx, &root_inst);
 	AbstractNode *root_node;
 	// Do we have an explicit root node (! modifier)?
 	if (!(root_node = find_root_tag(absolute_root_node))) root_node = absolute_root_node;

tests/cgalstlsanitytest.cc

@@ -84,10 +84,10 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 
 	root_module = parsefile(filename);
@@ -100,7 +100,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	AbstractNode *absolute_root_node = root_module->instantiate(&root_ctx, &root_inst);
+	AbstractNode *absolute_root_node = root_module->instantiate(&top_ctx, &root_inst);
 	AbstractNode *root_node;
 	// Do we have an explicit root node (! modifier)?
 	if (!(root_node = find_root_tag(absolute_root_node))) root_node = absolute_root_node;

tests/cgaltest.cc

@@ -81,10 +81,10 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 
 	root_module = parsefile(filename);
@@ -97,7 +97,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	AbstractNode *absolute_root_node = root_module->instantiate(&root_ctx, &root_inst);
+	AbstractNode *absolute_root_node = root_module->instantiate(&top_ctx, &root_inst);
 	AbstractNode *root_node;
 	// Do we have an explicit root node (! modifier)?
 	if (!(root_node = find_root_tag(absolute_root_node))) root_node = absolute_root_node;

tests/csgtermtest.cc

@@ -76,10 +76,10 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 	const AbstractNode *root_node;
 
@@ -93,7 +93,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	root_node = root_module->instantiate(&root_ctx, &root_inst);
+	root_node = root_module->instantiate(&top_ctx, &root_inst);
 
 	Tree tree(root_node);
 

tests/csgtestcore.cc

@@ -132,10 +132,10 @@ int csgtestcore(int argc, char *argv[], test_type_e test_type)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 
 	if (sysinfo_dump)
@@ -154,7 +154,7 @@ int csgtestcore(int argc, char *argv[], test_type_e test_type)
 	}
 
 	AbstractNode::resetIndexCounter();
-	AbstractNode *absolute_root_node = root_module->instantiate(&root_ctx, &root_inst);
+	AbstractNode *absolute_root_node = root_module->instantiate(&top_ctx, &root_inst);
 	AbstractNode *root_node;
 	// Do we have an explicit root node (! modifier)?
 	if (!(root_node = find_root_tag(absolute_root_node))) root_node = absolute_root_node;

tests/csgtexttest.cc

@@ -80,10 +80,10 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 	AbstractNode *root_node;
 
@@ -97,7 +97,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	root_node = root_module->instantiate(&root_ctx, &root_inst);
+	root_node = root_module->instantiate(&top_ctx, &root_inst);
 
 	Tree tree;
 	tree.setRoot(root_node);

tests/dumptest.cc

@@ -74,7 +74,6 @@ int main(int argc, char **argv)
 
 	const char *filename = argv[1];
 	const char *outfilename = argv[2];
-
 	int rc = 0;
 
 	Builtins::instance()->initialize();
@@ -86,10 +85,10 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 	AbstractNode *root_node;
 
@@ -103,7 +102,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	root_node = root_module->instantiate(&root_ctx, &root_inst);
+	root_node = root_module->instantiate(&top_ctx, &root_inst);
 
 	Tree tree;
 	tree.setRoot(root_node);
@@ -115,10 +114,17 @@ int main(int argc, char **argv)
 		exit(1);
 	}
 
+	fs::current_path(original_path);
 	std::ofstream outfile;
 	outfile.open(outfilename);
+	if (!outfile.is_open()) {
+		fprintf(stderr, "Error: Unable to open output file %s\n", outfilename);
+		exit(1);
+	}
+	std::cout << "Opened " << outfilename << "\n";
 	outfile << dumpstdstr << "\n";
 	outfile.close();
+	if (outfile.fail()) fprintf(stderr, "Failed to close file\n");
 
 	delete root_node;
 	delete root_module;
@@ -131,7 +137,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	root_node = root_module->instantiate(&root_ctx, &root_inst);
+	root_node = root_module->instantiate(&top_ctx, &root_inst);
 
 	tree.setRoot(root_node);
 

tests/echotest.cc

@@ -88,10 +88,10 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
+	FileModule *root_module;
 	ModuleInstantiation root_inst("group");
 	AbstractNode *root_node;
 
@@ -105,7 +105,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	root_node = root_module->instantiate(&root_ctx, &root_inst);
+	root_node = root_module->instantiate(&top_ctx, &root_inst);
 
 	delete root_node;
 	delete root_module;

tests/modulecachetest.cc

@@ -76,14 +76,13 @@ int main(int argc, char **argv)
 	parser_init(boosty::stringy(fs::path(argv[0]).branch_path()));
 	add_librarydir(boosty::stringy(fs::path(argv[0]).branch_path() / "../libraries"));
 
-	ModuleContext root_ctx;
-	root_ctx.registerBuiltin();
+	ModuleContext top_ctx;
+	top_ctx.registerBuiltin();
 
-	AbstractModule *root_module;
 	ModuleInstantiation root_inst("group");
 	AbstractNode *root_node;
 
-	root_module = parsefile(filename);
+	FileModule *root_module = parsefile(filename);
 	if (!root_module) {
 		fprintf(stderr, "Error: Unable to parse input file\n");
 		exit(1);
@@ -94,7 +93,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	root_node = root_module->instantiate(&root_ctx, &root_inst);
+	root_node = root_module->instantiate(&top_ctx, &root_inst);
 
 	delete root_node;
 	delete root_module;
@@ -109,7 +108,7 @@ int main(int argc, char **argv)
 	}
 
 	AbstractNode::resetIndexCounter();
-	root_node = root_module->instantiate(&root_ctx, &root_inst);
+	root_node = root_module->instantiate(&top_ctx, &root_inst);
 
 	delete root_node;
 	delete root_module;

tests/regression/dumptest/child-tests-expected.txt

@@ -37,4 +37,16 @@
 	multmatrix([[1, 0, 0, 5], [0, 1, 0, 3], [0, 0, 1, 0], [0, 0, 0, 1]]) {
 		group();
 	}
+	multmatrix([[1, 0, 0, 0], [0, 1, 0, 6], [0, 0, 1, 0], [0, 0, 0, 1]]) {
+		group() {
+			group() {
+				multmatrix([[1, 0, 0, 0], [0, 1, 0, 0], [0, 0, 1, 0], [0, 0, 0, 1]]) {
+					sphere($fn = 16, $fa = 12, $fs = 2, r = 1);
+				}
+				multmatrix([[1, 0, 0, 2.5], [0, 1, 0, 0], [0, 0, 1, 0], [0, 0, 0, 1]]) {
+					cube(size = [1, 1, 1], center = true);
+				}
+			}
+		}
+	}
 

tests/regression/echotest/variable-scope-tests-expected.txt

@@ -0,0 +1,16 @@
+ECHO: "special variable inheritance"
+ECHO: 23, 5
+WARNING: Ignoring unknown variable 'a'.
+ECHO: undef
+ECHO: 23, 5
+ECHO: "inner variables shadows parameter"
+ECHO: 5, 24
+ECHO: "user-defined special variables as parameter"
+ECHO: 7
+ECHO: 7
+ECHO: "assign only visible in children's scope"
+WARNING: Ignoring unknown variable 'c'.
+ECHO: undef
+ECHO: 5
+ECHO: "undeclared variable can still be passed and used"
+ECHO: 6