Merge pull request #468 from ArkScript-lang/feat/formatter

run the formatter twice in tests

CHANGELOG.md

@@ -17,6 +17,7 @@
 - fuzzing step in the CI
 - better error reporting on unknown import
 - check on number of arguments passed to `type`
+- warning when the formatter deletes comment(s) by mistake
 
 ### Changed
 - instructions are on 4 bytes: 1 byte for the instruction, 1 byte of padding, 2 bytes for an immediate argument

include/CLI/Formatter.hpp

@@ -14,9 +14,11 @@ constexpr struct FormatterConfig
 class Formatter final
 {
 public:
+    explicit Formatter(bool dry_run);
     Formatter(std::string filename, bool dry_run);
 
     void run();
+    void runWithString(const std::string& code);
 
     [[nodiscard]] const std::string& output() const;
 
@@ -26,6 +28,9 @@ class Formatter final
     Ark::internal::Parser m_parser;
     std::string m_output;
 
+    void processAst(const Ark::internal::Node& ast);
+    void warnIfCommentsWereRemoved(const std::string& original_code, const std::string& filename);
+
     static bool isListStartingWithKeyword(const Ark::internal::Node& node, Ark::internal::Keyword keyword);
     static bool isBeginBlock(const Ark::internal::Node& node);
     static bool isFuncDef(const Ark::internal::Node& node);
@@ -45,7 +50,7 @@ class Formatter final
      */
     static std::size_t lineOfLastNodeIn(const Ark::internal::Node& node);
 
-    bool should_split_on_newline(const Ark::internal::Node& node);
+    bool shouldSplitOnNewline(const Ark::internal::Node& node);
 
     static std::string prefix(const std::size_t indent)
     {

src/arkreactor/Compiler/AST/BaseParser.cpp

@@ -13,6 +13,13 @@ namespace Ark::internal
             }) != m_it_to_row.end())
             return;
 
+        // if the mapping is empty, the loop while never hit and we'll never insert anything
+        if (m_it_to_row.empty())
+        {
+            m_it_to_row.emplace_back(it, row);
+            return;
+        }
+
         for (std::size_t i = 0, end = m_it_to_row.size(); i < end; ++i)
         {
             auto current = m_it_to_row[i].first;

src/arkreactor/Compiler/AST/Parser.cpp

@@ -686,6 +686,7 @@ namespace Ark::internal
     {
         if (!accept(IsChar('(')))
             return std::nullopt;
+        auto cursor = getCursor();
         std::string comment;
         newlineOrComment(&comment);
 
@@ -708,7 +709,7 @@ namespace Ark::internal
         }
 
         std::optional<Node> leaf { call_type };
-        setNodePosAndFilename(leaf.value());
+        setNodePosAndFilename(leaf.value(), cursor);
         leaf->push_back(func.value());
 
         while (!isEOF())

src/arkscript/Formatter.cpp

@@ -1,6 +1,8 @@
+#include <Ark/Constants.hpp>
 #include <CLI/Formatter.hpp>
 
 #include <fmt/core.h>
+#include <termcolor/proxy.hpp>
 
 #include <Ark/Files.hpp>
 #include <Ark/Exceptions.hpp>
@@ -9,6 +11,10 @@
 using namespace Ark;
 using namespace Ark::internal;
 
+Formatter::Formatter(bool dry_run) :
+    m_dry_run(dry_run), m_parser(/* interpret= */ false)
+{}
+
 Formatter::Formatter(std::string filename, bool dry_run) :
     m_filename(std::move(filename)), m_dry_run(dry_run), m_parser(/* interpret= */ false)
 {}
@@ -19,28 +25,22 @@ void Formatter::run()
     {
         const std::string code = Utils::readFile(m_filename);
         m_parser.process(m_filename, code);
+        processAst(m_parser.ast());
+        warnIfCommentsWereRemoved(code, ARK_NO_NAME_FILE);
+    }
+    catch (const CodeError& e)
+    {
+        Diagnostics::generate(e);
+    }
+}
 
-        // remove useless surrounding begin (generated by the parser)
-        if (isBeginBlock(m_parser.ast()))
-        {
-            std::size_t previous_line = 0;
-            for (std::size_t i = 1, end = m_parser.ast().constList().size(); i < end; ++i)
-            {
-                const Node node = m_parser.ast().constList()[i];
-                if (node.line() - previous_line > 1 && !m_output.empty())
-                    m_output += "\n";
-                previous_line = lineOfLastNodeIn(node);
-                m_output += format(node, 0, false) + "\n";
-            }
-        }
-        else
-            m_output = format(m_parser.ast(), 0, false);
-
-        if (!m_dry_run)
-        {
-            std::ofstream stream(m_filename);
-            stream << m_output;
-        }
+void Formatter::runWithString(const std::string& code)
+{
+    try
+    {
+        m_parser.process(ARK_NO_NAME_FILE, code);
+        processAst(m_parser.ast());
+        warnIfCommentsWereRemoved(code, ARK_NO_NAME_FILE);
     }
     catch (const CodeError& e)
     {
@@ -53,6 +53,44 @@ const std::string& Formatter::output() const
     return m_output;
 }
 
+void Formatter::processAst(const Node& ast)
+{
+    // remove useless surrounding begin (generated by the parser)
+    if (isBeginBlock(ast))
+    {
+        std::size_t previous_line = 0;
+        for (std::size_t i = 1, end = ast.constList().size(); i < end; ++i)
+        {
+            const Node node = ast.constList()[i];
+            if (node.line() - previous_line > 1 && !m_output.empty())
+                m_output += "\n";
+            previous_line = lineOfLastNodeIn(node);
+            m_output += format(node, 0, false) + "\n";
+        }
+    }
+    else
+        m_output = format(ast, 0, false);
+
+    if (!m_dry_run)
+    {
+        std::ofstream stream(m_filename);
+        stream << m_output;
+    }
+}
+
+void Formatter::warnIfCommentsWereRemoved(const std::string& original_code, const std::string& filename)
+{
+    if (std::ranges::count(original_code, '#') != std::ranges::count(m_output, '#'))
+    {
+        std::cout << termcolor::yellow << "Warning" << termcolor::reset << ": one or more comments from the original source code seem to have been removed by mistake while formatting ";
+        if (filename != ARK_NO_NAME_FILE)
+            std::cout << "'" << filename << "'" << std::endl;
+        else
+            std::cout << "file" << std::endl;
+        std::cout << "Please fill an issue on GitHub: https://github.com/ArkScript-lang/Ark" << std::endl;
+    }
+}
+
 bool Formatter::isListStartingWithKeyword(const Node& node, const Keyword keyword)
 {
     return node.isListLike() && !node.constList().empty() && node.constList()[0].nodeType() == NodeType::Keyword && node.constList()[0].keyword() == keyword;
@@ -99,7 +137,7 @@ std::size_t Formatter::lineOfLastNodeIn(const Node& node)
     return node.line();
 }
 
-bool Formatter::should_split_on_newline(const Node& node)
+bool Formatter::shouldSplitOnNewline(const Node& node)
 {
     const std::string formatted = format(node, 0, false);
     const std::string::size_type sz = formatted.find_first_of('\n');
@@ -241,7 +279,7 @@ std::string Formatter::formatFunction(const Node& node, const std::size_t indent
     else
         formatted_args = format(args_node, indent, false);
 
-    if (!should_split_on_newline(body_node))
+    if (!shouldSplitOnNewline(body_node))
         return fmt::format("(fun {} {})", formatted_args, format(body_node, indent + 1, false));
     return fmt::format("(fun {}\n{})", formatted_args, format(body_node, indent + 1, true));
 }
@@ -253,7 +291,7 @@ std::string Formatter::formatVariable(const Node& node, const std::size_t indent
     const Node body_node = node.constList()[2];
     std::string formatted_body = format(body_node, indent + 1, false);
 
-    if (!should_split_on_newline(body_node) || isFuncDef(body_node))
+    if (!shouldSplitOnNewline(body_node) || isFuncDef(body_node))
         return fmt::format("({} {} {})", keyword, format(node.constList()[1], indent, false), formatted_body);
     return fmt::format("({} {}\n{})", keyword, format(node.constList()[1], indent, false), format(node.constList()[2], indent + 1, true));
 }
@@ -274,7 +312,7 @@ std::string Formatter::formatCondition(const Node& node, const std::size_t inden
         cond_on_newline ? "\n" : " ",
         formatted_cond);
 
-    const bool split_then_newline = should_split_on_newline(then_node);
+    const bool split_then_newline = shouldSplitOnNewline(then_node);
 
     // (if cond then)
     if (node.constList().size() == 3)
@@ -302,7 +340,7 @@ std::string Formatter::formatLoop(const Node& node, const std::size_t indent)
     if (formatted_cond.find('\n') != std::string::npos)
         cond_on_newline = true;
 
-    if (cond_on_newline || should_split_on_newline(body_node))
+    if (cond_on_newline || shouldSplitOnNewline(body_node))
         return fmt::format(
             "(while{}{}\n{})",
             cond_on_newline ? "\n" : " ",

tests/unittests/FormatterSuite.cpp

@@ -12,13 +12,26 @@ ut::suite<"Formatter"> formatter_suite = [] {
     iter_test_files(
         "FormatterSuite",
         [](TestData&& data) {
-            Formatter formatter(data.path, /* dry_run= */ true);
+            std::string formatted_code;
+
             should("output a correctly formatted code for " + data.stem) = [&] {
+                Formatter formatter(data.path, /* dry_run= */ true);
                 expect(nothrow([&] {
                     mut(formatter).run();
                 }));
+
+                formatted_code = formatter.output();
+                expect(that % formatted_code == data.expected);
+            };
+
+            should("not update an already correctly formatted code (" + data.stem + ")") = [&] {
+                Formatter formatter(/* dry_run= */ true);
+                expect(nothrow([&] {
+                    mut(formatter).runWithString(formatted_code);
+                }));
+
                 std::string code = formatter.output();
-                expect(that % code == data.expected);
+                expect(that % code == formatted_code);
             };
         });
 };

tests/unittests/resources/FormatterSuite/comments_after_call.expected

@@ -8,7 +8,6 @@
   c # last element
 ]
 [a b c] # list
-
 (foo
   # func
   bar

tests/unittests/resources/FormatterSuite/field.expected

@@ -1,5 +1,4 @@
 (let a foo.closure.name)
-
 (foo.closure.name
   # test
   this.bar.egg.qux)