Improve resolving variables.

src/Bindings.cc

@@ -31,9 +31,11 @@ IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include "Bindings.h"
 
+#include <Exception.h>
+
 #include "FilenameVariable.h"
 #include "TextVariable.h"
-#include <Exception.h>
+#include "VariableDependencies.h"
 
 Bindings::Bindings(Tokenizer& tokenizer) {
     auto token = tokenizer.next(Tokenizer::Skip::WHITESPACE);
@@ -61,15 +63,30 @@ Bindings::Bindings(Tokenizer& tokenizer) {
 }
 
 void Bindings::print(std::ostream& stream, const std::string& indent) const {
+    auto variable_names = std::vector<std::string>{};
+
     for (auto& pair : *this) {
+        variable_names.emplace_back(pair.first);
+    }
+
+    sort(variable_names.begin(), variable_names.end());
+
+    for (const auto& variable: variable_names) {
         stream << indent;
-        pair.second->print_definition(stream);
+        variables.find(variable)->second->print_definition(stream);
     }
 }
 
-void Bindings::resolve(const Scope& scope, bool expand_variables) {
-    auto context = ResolveContext{scope, expand_variables};
-    for (auto& pair : variables) {
-        pair.second->resolve(context);
+void Bindings::resolve(const Scope& scope, bool expand_variables, bool classify_filenames) {
+    auto dependencies = VariableDependencies(variables);
+    ResolveResult result;
+    auto context = ResolveContext{scope, result, expand_variables, classify_filenames};
+
+    while (!dependencies.finished()) {
+        for (auto& name: dependencies.get_next()) {
+            result.unresolved_used_variables.clear();
+            variables[name]->resolve(context);
+            dependencies.update(name, result.unresolved_used_variables);
+        }
     }
 }

src/Bindings.h

@@ -32,7 +32,7 @@ IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #ifndef BINDINGS_H
 #define BINDINGS_H
 
-#include <map>
+#include <unordered_map>
 
 #include "Tokenizer.h"
 #include "Variable.h"
@@ -45,7 +45,7 @@ class Bindings {
     explicit Bindings(Tokenizer& tokenizer);
 
     void print(std::ostream& stream, const std::string& indent) const;
-    void resolve(const Scope& scope, bool expand_variables = true);
+    void resolve(const Scope& scope, bool expand_variables = true, bool classify_variables = true);
     void add(std::shared_ptr<Variable> variable) {variables[variable->name] = std::move(variable);}
 
     [[nodiscard]] auto empty() const {return variables.empty();}
@@ -57,7 +57,7 @@ class Bindings {
     [[nodiscard]] auto find(const std::string& name) const {return variables.find(name);}
 
   private:
-    std::map<std::string, std::shared_ptr<Variable>> variables;
+    std::unordered_map<std::string, std::shared_ptr<Variable>> variables;
 };
 
 #endif // BINDINGS_H

src/CMakeLists.txt

@@ -12,13 +12,15 @@ ADD_EXECUTABLE(fast-ninja
         FilenameWord.cc
         Pool.cc
         ResolveContext.cc
+        ResolveResult.cc
         Rule.cc
         Scope.cc
         ScopedDirective.cc
         Text.cc
         TextVariable.cc
         Tokenizer.cc
         Variable.cc
+        VariableDependencies.cc
         VariableReference.cc
         Word.cc
 )

src/Dependencies.cc

@@ -69,11 +69,15 @@ Dependencies::Dependencies(Tokenizer& tokenizer, bool is_build) {
 
 
 void Dependencies::resolve(const Scope& scope) {
-    auto context = ResolveContext{scope};
+    ResolveResult result;
+    auto context = ResolveContext{scope, result};
     direct.resolve(context);
     implicit.resolve(context);
     order.resolve(context);
     validation.resolve(context);
+    if (!result.unresolved_used_variables.empty()) {
+        throw Exception("unresolved variables"); // TODO: include list of variables
+    }
 }
 
 void Dependencies::serialize(std::ostream& stream) const {

src/File.cc

@@ -58,17 +58,19 @@ File::File(const std::filesystem::path& filename, const std::filesystem::path& b
 }
 
 void File::process() {
-    auto bindings = Bindings{};
-    bindings.add(std::shared_ptr<Variable>(new TextVariable{ "command", Text{ std::vector<Word>{ Word{ "fast-ninja", false }, Word{ " ", false }, Word{ source_directory.string(), true } } } }));
-    bindings.add(std::shared_ptr<Variable>(new TextVariable{ "generator", Text{ "1", false } }));
+    auto generator_bindings = Bindings{};
+    generator_bindings.add(std::shared_ptr<Variable>(new TextVariable{ "command", Text{ std::vector<Word>{ Word{ "fast-ninja", false }, Word{ " ", false }, Word{ source_directory.string(), true } } } }));
+    generator_bindings.add(std::shared_ptr<Variable>(new TextVariable{ "generator", Text{ "1", false } }));
 
-    rules["fast-ninja"] = Rule(this, "fast-ninja", bindings);
+    rules["fast-ninja"] = Rule(this, "fast-ninja", generator_bindings);
     auto ninja_outputs = std::vector<Filename>{};
     auto ninja_inputs = std::vector<Filename>{};
     add_generator_build(ninja_outputs, ninja_inputs);
 
     builds.emplace_back(this, "fast-ninja", Dependencies{ FilenameList{ ninja_outputs } }, Dependencies{ FilenameList{ ninja_inputs } }, Bindings{});
 
+    bindings.resolve(*this, true, false);
+
     process_output();
     process_rest();
 }
@@ -100,8 +102,12 @@ void File::process_rest() { // NOLINT(misc-no-recursion)
         build.process(*this);
     }
 
-    auto context = ResolveContext{*this};
+    ResolveResult result;
+    auto context = ResolveContext{*this, result};
     defaults.resolve(context);
+    if (!result.unresolved_used_variables.empty()) {
+        // TODO: error: unresolved variables
+    }
 
     for (const auto& file : subfiles) {
         file->process_rest();

src/Filename.cc

@@ -36,6 +36,9 @@ IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include "File.h"
 
 void Filename::resolve(const ResolveContext& context) {
+    if (!context.classify_filenames) {
+        return;
+    }
     const auto file = context.scope.get_file();
 
     if (type == Type::UNKNOWN) {

src/FilenameList.cc

@@ -44,6 +44,9 @@ FilenameList::FilenameList(Tokenizer& tokenizer, Type type) {
         auto word = FilenameWord{tokenizer, force_build};
         if (!word.empty()) {
             words.emplace_back(word);
+            if (!word.is_resolved()) {
+                resolved = false;
+            }
         }
 
         if (scoped) {
@@ -69,15 +72,25 @@ FilenameList::FilenameList(Tokenizer& tokenizer, Type type) {
 }
 
 void FilenameList::resolve(const ResolveContext& context) {
+    resolved = true;
     for (auto& word : words) {
         word.resolve(context);
-        word.collect_filenames(filenames);
+        if (!word.is_resolved()) {
+            resolved = false;
+        }
     }
-    for (auto& filename: filenames) {
-        if (force_build) {
-            filename.type = Filename::Type::BUILD;
+    if (resolved) {
+        if (filenames.empty()) {
+            for (const auto& word: words) {
+                word.collect_filenames(filenames);
+            }
+        }
+        for (auto& filename : filenames) {
+            if (force_build) {
+                filename.type = Filename::Type::BUILD;
+            }
+            filename.resolve(context);
         }
-        filename.resolve(context);
     }
 }
 

src/FilenameList.h

@@ -51,8 +51,10 @@ class FilenameList {
 
     void resolve(const ResolveContext& context);
     [[nodiscard]] bool empty() const {return words.empty() && filenames.empty();}
+    [[nodiscard]] bool is_resolved() const {return resolved;}
     void serialize(std::ostream& stream) const;
     [[nodiscard]] std::string string() const;
+    [[nodiscard]] bool contains_unknown_file() const {return false;} // TODO
     void collect_output_files(std::unordered_set<std::string>& output_files) const;
 
     void collect_filenames(std::vector<Filename>& collector) const {collector.insert(collector.end(), filenames.begin(), filenames.end());}
@@ -61,6 +63,7 @@ class FilenameList {
     std::vector<FilenameWord> words;
     std::vector<Filename> filenames;
     bool force_build{false};
+    bool resolved{true};
 };
 
 std::ostream& operator<<(std::ostream& stream, const FilenameList& filename_list);

src/FilenameVariable.h

@@ -43,10 +43,12 @@ class FilenameVariable : public Variable {
     FilenameVariable(std::string name, Tokenizer& tokenizer);
     FilenameVariable(std::string name, FilenameList value): Variable(std::move(name)), value{std::move(value)} {}
 
-    void resolve_sub(const ResolveContext& context) override {value.resolve(context);}
+    void resolve(const ResolveContext& context) override {value.resolve(context);}
     void print_definition(std::ostream& stream) const override;
     [[nodiscard]] std::string string() const override {return value.string();}
+    [[nodiscard]] bool contains_unknown_file() const override {return value.contains_unknown_file();}
     void collect_filenames(std::vector<Filename>& collector) const {return value.collect_filenames(collector);}
+    bool is_resolved() const override {return value.is_resolved();}
 
 private:
     FilenameList value;

src/FilenameWord.cc

@@ -35,6 +35,7 @@ IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include "FilenameVariable.h"
 
+
 FilenameWord::FilenameWord(Tokenizer& tokenizer, bool force_build): force_build{force_build} {
     std::string string;
 
@@ -81,6 +82,7 @@ FilenameWord::FilenameWord(Tokenizer& tokenizer, bool force_build): force_build{
                 string = "";
             }
             elements.emplace_back(VariableReference(token.value));
+            resolved = false;
         }
         else if (braced || token.type == Tokenizer::TokenType::WORD) {
             string += token.value;
@@ -98,22 +100,37 @@ FilenameWord::FilenameWord(Tokenizer& tokenizer, bool force_build): force_build{
 
 void FilenameWord::resolve(const ResolveContext& context) {
     auto contains_filename_variable = false;
+    resolved = true;
+
     for (auto& element : elements) {
         if (std::holds_alternative<VariableReference>(element)) {
             auto& variable_reference = std::get<VariableReference>(element);
-            variable_reference.resolve(context);
-            if (variable_reference.is_text_variable()) {
-                element = variable_reference.variable->string();
+            if (auto variable = variable_reference.resolve(context)) {
+                if (variable->is_filename()) {
+                    contains_filename_variable = true;
+                }
+                if (!variable->is_resolved()) {
+                    context.result.add_unresolved_variable_use(variable->name);
+                    resolved = false;
+                }
+                element = variable;
             }
             else {
-                contains_filename_variable = true;
+                throw Exception("unknown variable %s", variable_reference.name.c_str());
             }
         }
     }
-    if (!contains_filename_variable) {
+
+    // TODO: this shouldn't be neccessary
+    if (resolved && !contains_filename_variable) {
         auto name = std::string();
         for (const auto& element: elements) {
-            name += std::get<std::string>(element);
+            if (std::holds_alternative<std::string>(element)) {
+                name += std::get<std::string>(element);
+            }
+            else if (std::holds_alternative<const Variable*>(element)) {
+                name += std::get<const Variable*>(element)->string();
+            }
         }
         filename = Filename{force_build ? Filename::Type::BUILD : Filename::Type::UNKNOWN, name};
         filename->resolve(context);
@@ -134,19 +151,23 @@ void FilenameWord::collect_filenames(std::vector<Filename>& filenames) const {
             if (std::holds_alternative<std::string>(element)) {
                 *current_string += std::get<std::string>(element);
             }
+            else if (std::holds_alternative<VariableReference>(element)) {
+                auto variable_reference = std::get<VariableReference>(element);
+                throw Exception("unresolved variable %s", variable_reference.name.c_str());
+            }
             else {
-                auto variable = std::get<VariableReference>(element);
-                if (variable.is_filename_variable()) {
+                auto variable = std::get<const Variable*>(element);
+                if (variable->is_filename()) {
                     if (filename_variable) {
                         throw Exception("multiple filename variables in filename not allowed");
                     }
                     else {
-                        filename_variable = variable.variable->as_filename();
+                        filename_variable = variable->as_filename();
                         current_string = &postfix;
                     }
                 }
                 else {
-                    *current_string += variable.variable->string();
+                    *current_string += variable->string();
                 }
             }
         }

src/FilenameWord.h

@@ -48,14 +48,16 @@ class FilenameWord {
     explicit FilenameWord(std::string word): elements{std::move(word)} {}
 
     [[nodiscard]] bool empty() const {return elements.empty();}
+    [[nodiscard]] bool is_resolved() const {return resolved;}
     void resolve(const ResolveContext& context);
 
     void collect_filenames(std::vector<Filename>& filenames) const;
 
 private:
-    std::vector<std::variant<std::string, VariableReference>> elements;
+    std::vector<std::variant<std::string, VariableReference, const Variable*>> elements;
     std::optional<Filename> filename;
     bool force_build{false};
+    bool resolved{true};
 };
 
 #endif // EXPLICITFILENAME_H

src/ResolveContext.cc

@@ -34,20 +34,7 @@ IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include <Exception.h>
 
-ResolveContext ResolveContext::resolving(const std::string& name) const {
-    if (resolving_variables.contains(name)) {
-        throw Exception("circular variable definition involving %s", name.c_str()); // TODO: include all variables in cycle
-    }
-
-    auto new_context = *this;
-    new_context.resolving_variables.insert(name);
-    return new_context;
-}
-
 const Variable* ResolveContext::get_variable(const std::string& name) const {
     const auto variable = scope.get_variable(name);
-    if (variable) {
-        variable->resolve(*this);
-    }
     return variable;
 }

src/ResolveContext.h

@@ -29,27 +29,27 @@ OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN
 IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */
 
-#ifndef RESOLVECONTEXT_H
-#define RESOLVECONTEXT_H
+#ifndef RESOLVE_CONTEXT_H
+#define RESOLVE_CONTEXT_H
 
 #include <unordered_set>
 
+#include "ResolveResult.h"
 #include "Scope.h"
 
 class ResolveContext {
 public:
-    explicit ResolveContext(const Scope& scope, bool expand_variables = false): scope{scope}, expand_variables{expand_variables} {}
+  // TODO: expand_variables should default to true
+    ResolveContext(const Scope& scope, ResolveResult& result, bool expand_variables = false, bool classify_filenames = true): scope{scope}, result{result}, expand_variables{expand_variables}, classify_filenames{classify_filenames} {}
 
-    [[nodiscard]] ResolveContext resolving(const std::string& name) const;
     [[nodiscard]] const Variable* get_variable(const std::string& name) const;
 
     const Scope& scope;
-    bool expand_variables{false};
-
-private:
-    std::unordered_set<std::string> resolving_variables;
+    bool expand_variables;
+    bool classify_filenames;
+    ResolveResult& result;
 };
 
 
 
-#endif //RESOLVECONTEXT_H
+#endif //RESOLVE_CONTEXT_H