From c42499312f529c86f6268fdeed067d0611cacb30 Mon Sep 17 00:00:00 2001 From: Alexandre Plateau Date: Wed, 4 Dec 2024 16:58:40 +0100 Subject: [PATCH] feat(name resolution): only allow unprefix names to be fully qualified in their own namespace --- .../NameResolution/NameResolutionPass.hpp | 2 + .../Compiler/NameResolution/ScopeResolver.hpp | 11 +++++- .../Compiler/NameResolution/StaticScope.hpp | 14 ++++++- .../NameResolution/NameResolutionPass.cpp | 31 ++++++++++++--- .../Compiler/NameResolution/ScopeResolver.cpp | 39 +++++++++++++++++-- .../Compiler/NameResolution/StaticScope.cpp | 14 ++++++- .../compileTime/package/b.ark | 1 + .../compileTime/unbound_but_namespace.ark | 3 ++ .../unbound_but_namespace.expected | 7 ++++ 9 files changed, 107 insertions(+), 15 deletions(-) create mode 100644 tests/unittests/resources/DiagnosticsSuite/compileTime/package/b.ark create mode 100644 tests/unittests/resources/DiagnosticsSuite/compileTime/unbound_but_namespace.ark create mode 100644 tests/unittests/resources/DiagnosticsSuite/compileTime/unbound_but_namespace.expected diff --git a/include/Ark/Compiler/NameResolution/NameResolutionPass.hpp b/include/Ark/Compiler/NameResolution/NameResolutionPass.hpp index d3db8965..fbf4686c 100644 --- a/include/Ark/Compiler/NameResolution/NameResolutionPass.hpp +++ b/include/Ark/Compiler/NameResolution/NameResolutionPass.hpp @@ -90,6 +90,8 @@ namespace Ark::internal */ [[nodiscard]] bool mayBeFromPlugin(const std::string& name) const noexcept; + std::string updateSymbolWithFullyQualifiedName(Node& symbol); + /** * @brief Checks for undefined symbols, not present in the defined symbols table * diff --git a/include/Ark/Compiler/NameResolution/ScopeResolver.hpp b/include/Ark/Compiler/NameResolution/ScopeResolver.hpp index f2a50452..489f933a 100644 --- a/include/Ark/Compiler/NameResolution/ScopeResolver.hpp +++ b/include/Ark/Compiler/NameResolution/ScopeResolver.hpp @@ -16,6 +16,7 @@ #include #include #include +#include #include @@ -98,7 +99,15 @@ namespace Ark::internal * @param name * @return std::string */ - [[nodiscard]] std::string getFullyQualifiedNameInNearestScope(const std::string& name); + [[nodiscard]] std::string getFullyQualifiedNameInNearestScope(const std::string& name) const; + + /** + * @brief Checks if a name can be fully qualified (allows only unprefixed names to be resolved by glob namespaces or inside their own namespace) + * + * @param name + * @return std::pair if the name can be fully qualified, first element is true ; second element is the FQN + */ + [[nodiscard]] std::pair canFullyQualifyName(const std::string& name); /** * @brief Return a non-owning raw pointer to the current scope diff --git a/include/Ark/Compiler/NameResolution/StaticScope.hpp b/include/Ark/Compiler/NameResolution/StaticScope.hpp index 579f02d7..996320bf 100644 --- a/include/Ark/Compiler/NameResolution/StaticScope.hpp +++ b/include/Ark/Compiler/NameResolution/StaticScope.hpp @@ -73,7 +73,12 @@ namespace Ark::internal * @return true if the scope was saved, on NamespaceScope * @return false on StaticScope */ - virtual bool saveNamespaceAndRemove(std::unique_ptr&); + virtual bool saveNamespace(std::unique_ptr&); + + [[nodiscard]] virtual bool isNamespace() const; + [[nodiscard]] inline virtual bool withPrefix() const { return false; } + [[nodiscard]] inline virtual bool isGlob() const { return false; } + [[nodiscard]] inline virtual std::string prefix() const { return ""; } private: std::unordered_set m_vars {}; @@ -112,7 +117,12 @@ namespace Ark::internal * @return true if the scope was saved, on NamespaceScope * @return false on StaticScope */ - bool saveNamespaceAndRemove(std::unique_ptr&) override; + bool saveNamespace(std::unique_ptr&) override; + + [[nodiscard]] bool isNamespace() const override; + [[nodiscard]] inline bool withPrefix() const override { return m_with_prefix; } + [[nodiscard]] inline bool isGlob() const override { return m_is_glob; } + [[nodiscard]] inline std::string prefix() const override { return m_namespace; } private: std::string m_namespace; diff --git a/src/arkreactor/Compiler/NameResolution/NameResolutionPass.cpp b/src/arkreactor/Compiler/NameResolution/NameResolutionPass.cpp index a4e7559d..217cf27e 100644 --- a/src/arkreactor/Compiler/NameResolution/NameResolutionPass.cpp +++ b/src/arkreactor/Compiler/NameResolution/NameResolutionPass.cpp @@ -59,7 +59,7 @@ namespace Ark::internal case NodeType::Symbol: { const std::string old_name = node.string(); - node.setString(m_scope_resolver.getFullyQualifiedNameInNearestScope(old_name)); + updateSymbolWithFullyQualifiedName(node); addSymbolNode(node, old_name); break; } @@ -68,6 +68,7 @@ namespace Ark::internal for (auto& child : node.list()) { const std::string old_name = child.string(); + // in case of field, no need to check if we can fully qualify names child.setString(m_scope_resolver.getFullyQualifiedNameInNearestScope(old_name)); addSymbolNode(child, old_name); } @@ -197,8 +198,7 @@ namespace Ark::internal node.constList()[1].col(), name); - const std::string fully_qualified_name = m_scope_resolver.getFullyQualifiedNameInNearestScope(name); - node.list()[1].setString(fully_qualified_name); + updateSymbolWithFullyQualifiedName(node.list()[1]); } else if (keyword != Keyword::Set) { @@ -236,9 +236,8 @@ namespace Ark::internal // update the declared variable name to use the fully qualified name // this will prevent name conflicts, and handle scope resolution - const std::string fully_qualified_name = m_scope_resolver.getFullyQualifiedNameInNearestScope(child.string()); - addDefinedSymbol(fully_qualified_name, true); - child.setString(fully_qualified_name); + std::string fqn = updateSymbolWithFullyQualifiedName(child); + addDefinedSymbol(fqn, true); } else if (child.nodeType() == NodeType::Symbol) addDefinedSymbol(child.string(), /* is_mutable= */ true); @@ -300,6 +299,26 @@ namespace Ark::internal return it != m_plugin_names.end(); } + std::string NameResolutionPass::updateSymbolWithFullyQualifiedName(Node& symbol) + { + auto [allowed, fqn] = m_scope_resolver.canFullyQualifyName(symbol.string()); + + if (!allowed) + { + std::string match = m_scope_resolver.getFullyQualifiedNameInNearestScope(symbol.string()); + + throw CodeError( + fmt::format(R"(Unbound variable "{}". However, it exists in a namespace as "{}", did you forget to prefix it with its namespace?)", symbol.string(), match), + symbol.filename(), + symbol.line(), + symbol.col(), + symbol.repr()); + } + + symbol.setString(fqn); + return fqn; + } + void NameResolutionPass::checkForUndefinedSymbol() const { for (const auto& sym : m_symbol_nodes) diff --git a/src/arkreactor/Compiler/NameResolution/ScopeResolver.cpp b/src/arkreactor/Compiler/NameResolution/ScopeResolver.cpp index e638a24d..bb4a89e5 100644 --- a/src/arkreactor/Compiler/NameResolution/ScopeResolver.cpp +++ b/src/arkreactor/Compiler/NameResolution/ScopeResolver.cpp @@ -34,7 +34,7 @@ namespace Ark::internal { for (auto& m_scope : std::ranges::reverse_view(m_scopes) | std::ranges::views::drop(1)) { - if (m_scope->saveNamespaceAndRemove(m_scopes.back())) + if (m_scope->saveNamespace(m_scopes.back())) break; } @@ -66,16 +66,47 @@ namespace Ark::internal return m_scopes.back()->get(name, false).has_value(); } - std::string ScopeResolver::getFullyQualifiedNameInNearestScope(const std::string& name) + std::string ScopeResolver::getFullyQualifiedNameInNearestScope(const std::string& name) const { - for (const auto& m_scope : std::ranges::reverse_view(m_scopes)) + for (const auto& scope : std::ranges::reverse_view(m_scopes)) { - if (auto maybe_fqn = m_scope->get(name, true); maybe_fqn.has_value()) + if (auto maybe_fqn = scope->get(name, true); maybe_fqn.has_value()) return maybe_fqn.value().name; } return name; } + std::pair ScopeResolver::canFullyQualifyName(const std::string& name) + { + // a given name can be fully qualified if + // old == new + // old != new and new has prefix + // if the prefix namespace is glob + // if the prefix namespace is with_prefix && it is the top most scope + const std::string maybe_fqn = getFullyQualifiedNameInNearestScope(name); + + if (maybe_fqn == name) + return std::make_pair(true, maybe_fqn); + + const std::string prefix = maybe_fqn.substr(0, maybe_fqn.find_first_of(':')); + auto namespaces = + std::ranges::reverse_view(m_scopes) | std::ranges::views::filter([](const auto& e) { + return e->isNamespace(); + }); + bool top = true; + for (auto& scope : namespaces) + { + if (top && prefix == scope->prefix()) + return std::make_pair(true, maybe_fqn); + if (!top && prefix == scope->prefix() && scope->isGlob()) + return std::make_pair(true, maybe_fqn); + + top = false; + } + + return std::make_pair(false, maybe_fqn); + } + StaticScope* ScopeResolver::currentScope() const { if (!m_scopes.empty()) [[likely]] diff --git a/src/arkreactor/Compiler/NameResolution/StaticScope.cpp b/src/arkreactor/Compiler/NameResolution/StaticScope.cpp index ea59cc46..c052ab14 100644 --- a/src/arkreactor/Compiler/NameResolution/StaticScope.cpp +++ b/src/arkreactor/Compiler/NameResolution/StaticScope.cpp @@ -23,12 +23,17 @@ namespace Ark::internal return name; } - bool StaticScope::saveNamespaceAndRemove([[maybe_unused]] std::unique_ptr&) + bool StaticScope::saveNamespace([[maybe_unused]] std::unique_ptr&) { // the scope can not be saved on a static scope return false; } + bool StaticScope::isNamespace() const + { + return false; + } + NamespaceScope::NamespaceScope(std::string name, const bool with_prefix, const bool is_glob, const std::vector& symbols) : StaticScope(), m_namespace(std::move(name)), @@ -82,9 +87,14 @@ namespace Ark::internal return name; } - bool NamespaceScope::saveNamespaceAndRemove(std::unique_ptr& scope) + bool NamespaceScope::saveNamespace(std::unique_ptr& scope) { m_additional_namespaces.push_back(std::move(scope)); return true; } + + bool NamespaceScope::isNamespace() const + { + return true; + } } diff --git a/tests/unittests/resources/DiagnosticsSuite/compileTime/package/b.ark b/tests/unittests/resources/DiagnosticsSuite/compileTime/package/b.ark new file mode 100644 index 00000000..203daf20 --- /dev/null +++ b/tests/unittests/resources/DiagnosticsSuite/compileTime/package/b.ark @@ -0,0 +1 @@ +(let bar 5) diff --git a/tests/unittests/resources/DiagnosticsSuite/compileTime/unbound_but_namespace.ark b/tests/unittests/resources/DiagnosticsSuite/compileTime/unbound_but_namespace.ark new file mode 100644 index 00000000..a36bed09 --- /dev/null +++ b/tests/unittests/resources/DiagnosticsSuite/compileTime/unbound_but_namespace.ark @@ -0,0 +1,3 @@ +(import package.b) + +(print bar) diff --git a/tests/unittests/resources/DiagnosticsSuite/compileTime/unbound_but_namespace.expected b/tests/unittests/resources/DiagnosticsSuite/compileTime/unbound_but_namespace.expected new file mode 100644 index 00000000..e088b148 --- /dev/null +++ b/tests/unittests/resources/DiagnosticsSuite/compileTime/unbound_but_namespace.expected @@ -0,0 +1,7 @@ +At bar @ 3:12 + 1 | (import package.b) + 2 | + 3 | (print bar) + | ^~~~~~~~~~~ + 4 | + Unbound variable "bar". However, it exists in a namespace as "b:bar", did you forget to prefix it with its namespace?