From 2b1876f862d4a9db267abd64e0c0d46371025139 Mon Sep 17 00:00:00 2001 From: Tyler Wilding Date: Sat, 11 Nov 2023 17:51:55 -0500 Subject: [PATCH] formatter: Add support for a few common functions and fix an LSP startup issue (#3190) --- .vs/launch.vs.json | 4 +- common/formatter/formatter.cpp | 14 ++- common/formatter/rules/rule_config.cpp | 49 +++++++++- common/formatter/rules/rule_config.h | 9 +- common/log/log.cpp | 4 +- decompiler/config/jak2/ntsc_v1/art_info.jsonc | 2 +- goal_src/jak2/pc/util/popup-menu-h.gc | 12 +-- goal_src/jak2/pc/util/popup-menu.gc | 41 ++++----- lsp/main.cpp | 3 +- .../formatter/corpus/cond-and-case.test.gc | 91 +++++++++++++++++++ .../common/formatter/corpus/iteration.test.gc | 15 +++ test/common/formatter/corpus/lists.test.gc | 10 ++ test/common/formatter/corpus/types.test.gc | 35 +++++++ tools/formatter/main.cpp | 9 +- 14 files changed, 249 insertions(+), 49 deletions(-) create mode 100644 test/common/formatter/corpus/cond-and-case.test.gc create mode 100644 test/common/formatter/corpus/iteration.test.gc create mode 100644 test/common/formatter/corpus/types.test.gc diff --git a/.vs/launch.vs.json b/.vs/launch.vs.json index d9373e069ae..4c3237e8897 100644 --- a/.vs/launch.vs.json +++ b/.vs/launch.vs.json @@ -267,8 +267,8 @@ "type" : "default", "project" : "CMakeLists.txt", "projectTarget" : "formatter.exe (bin\\formatter.exe)", - "name" : "Tools - Formatter", - "args" : ["--new", "--file", "C:\\Users\\xtvas\\Repos\\opengoal\\jak-project\\test-formatter.gc"] + "name" : "Tools - Formatter - Inplace", + "args" : ["--write", "--file", "C:\\Users\\xtvas\\Repos\\opengoal\\jak-project\\goal_src\\jak2\\pc\\util\\popup-menu-h.gc"] }, { "type": "default", diff --git a/common/formatter/formatter.cpp b/common/formatter/formatter.cpp index a6f2694028e..46202deb970 100644 --- a/common/formatter/formatter.cpp +++ b/common/formatter/formatter.cpp @@ -114,6 +114,8 @@ void apply_formatting_config( if (predefined_config && predefined_config->index_configs.find(i) != predefined_config->index_configs.end()) { apply_formatting_config(ref, predefined_config->index_configs.at(i)); + } else if (predefined_config && predefined_config->default_index_config) { + apply_formatting_config(ref, predefined_config->default_index_config); } else { apply_formatting_config(ref); } @@ -179,7 +181,7 @@ bool can_node_be_inlined(const FormatterTreeNode& curr_node, int cursor_pos) { return false; } // If this is set in the config, then the form is intended to be partially inlined - if (curr_node.formatting_config.inline_until_index != -1) { + if (curr_node.formatting_config.inline_until_index({})) { return false; } // let's see if we can inline the form all on one line to do that, we recursively explore @@ -193,7 +195,8 @@ std::vector apply_formatting(const FormatterTreeNode& curr_node, int cursor_pos = 0) { using namespace formatter_rules; if (!curr_node.token && curr_node.refs.empty()) { - return output; + // special case to handle an empty list + return {"()"}; } // If its a token, just print the token and move on @@ -211,6 +214,7 @@ std::vector apply_formatting(const FormatterTreeNode& curr_node, // // This means we may combine elements onto the same line in this step. std::vector form_lines = {}; + for (int i = 0; i < (int)curr_node.refs.size(); i++) { const auto& ref = curr_node.refs.at(i); // Add new line entry @@ -265,10 +269,10 @@ std::vector apply_formatting(const FormatterTreeNode& curr_node, } // Consolidate any lines if the configuration requires it - if (curr_node.formatting_config.inline_until_index != -1) { + if (curr_node.formatting_config.inline_until_index(form_lines)) { std::vector new_form_lines = {}; for (int i = 0; i < (int)form_lines.size(); i++) { - if (i < curr_node.formatting_config.inline_until_index) { + if (i < curr_node.formatting_config.inline_until_index(form_lines)) { if (new_form_lines.empty()) { new_form_lines.push_back(form_lines.at(i)); } else { @@ -311,7 +315,7 @@ std::vector apply_formatting(const FormatterTreeNode& curr_node, } std::string join_formatted_lines(const std::vector lines) { - // TODO - respect original file line endings? + // TODO - respect original file line endings return fmt::format("{}", fmt::join(lines, "\n")); } diff --git a/common/formatter/rules/rule_config.cpp b/common/formatter/rules/rule_config.cpp index 121c00cb354..4b578165595 100644 --- a/common/formatter/rules/rule_config.cpp +++ b/common/formatter/rules/rule_config.cpp @@ -5,13 +5,34 @@ namespace formatter_rules { namespace config { -// TODO - populate these more - // TODO - this could be greatly simplified with C++20's designated initialization FormFormattingConfig new_flow_rule(int start_index) { FormFormattingConfig cfg; cfg.hang_forms = false; - cfg.inline_until_index = start_index; + cfg.inline_until_index = [start_index](const std::vector& curr_lines) { + return start_index; + }; + return cfg; +} + +FormFormattingConfig new_flow_rule_prevent_inlining_indexes( + int start_index, + const std::vector& inlining_preventation_indices) { + FormFormattingConfig cfg; + cfg.hang_forms = false; + cfg.inline_until_index = [start_index](std::vector curr_lines) { + if (curr_lines.size() >= 4 && curr_lines.at(3) == "()") { + return 4; + } + return start_index; + }; + for (const auto& index : inlining_preventation_indices) { + auto temp_config = std::make_shared(); + temp_config->prevent_inlining = true; + temp_config->hang_forms = false; + temp_config->indentation_width = 1; + cfg.index_configs.emplace(index, temp_config); + } return cfg; } @@ -41,9 +62,27 @@ FormFormattingConfig new_binding_rule() { return cfg; } +FormFormattingConfig new_pair_rule(bool combine_first_two_expr) { + FormFormattingConfig cfg; + cfg.hang_forms = false; + cfg.prevent_inlining = true; + cfg.combine_first_two_lines = combine_first_two_expr; + auto pair_config = std::make_shared(); + pair_config->hang_forms = false; + pair_config->indentation_width = 1; + cfg.default_index_config = pair_config; + return cfg; +} + const std::unordered_map opengoal_form_config = { + {"case", new_pair_rule(true)}, + {"cond", new_pair_rule(false)}, + {"defmethod", new_flow_rule(3)}, + {"deftype", new_flow_rule_prevent_inlining_indexes(3, {3, 4, 5})}, {"defun", new_flow_rule(3)}, - {"defmethod", new_flow_rule(4)}, - {"let", new_binding_rule()}}; + {"dotimes", new_flow_rule(2)}, + {"let", new_binding_rule()}, + {"when", new_flow_rule(2)}, + {"with-dma-buffer-add-bucket", new_flow_rule(2)}}; } // namespace config } // namespace formatter_rules diff --git a/common/formatter/rules/rule_config.h b/common/formatter/rules/rule_config.h index 173e07d7bbd..170a2f498fa 100644 --- a/common/formatter/rules/rule_config.h +++ b/common/formatter/rules/rule_config.h @@ -19,13 +19,16 @@ struct FormFormattingConfig { std::function indentation_width_for_index = [](FormFormattingConfig config, int /*index*/) { return config.indentation_width; }; bool combine_first_two_lines = - false; // NOTE - basically hang, but will probably stick around after hang is gone - int inline_until_index = -1; + false; // NOTE - basically hang, but will probably stick around after hang is gone, may be + // redundant (inline_until_index!) + std::function(const std::vector& curr_lines)> inline_until_index = + [](std::vector curr_lines) { return std::nullopt; }; bool has_constant_pairs = false; - bool prevent_inlining = false; + bool prevent_inlining = false; // TODO - duplicate of below std::function should_prevent_inlining = [](FormFormattingConfig config, int /*num_refs*/) { return config.prevent_inlining; }; int parent_mutable_extra_indent = 0; + std::optional> default_index_config; std::unordered_map> index_configs = {}; }; diff --git a/common/log/log.cpp b/common/log/log.cpp index 47423624c7c..a985dcc3178 100644 --- a/common/log/log.cpp +++ b/common/log/log.cpp @@ -153,7 +153,9 @@ void set_file(const std::string& filename, } } } else { - complete_filename += ".log"; + if (!str_util::ends_with(complete_filename, ".log")) { + complete_filename += ".log"; + } } if (append) { diff --git a/decompiler/config/jak2/ntsc_v1/art_info.jsonc b/decompiler/config/jak2/ntsc_v1/art_info.jsonc index ea1e615fcd2..a0fe31b5cd0 100644 --- a/decompiler/config/jak2/ntsc_v1/art_info.jsonc +++ b/decompiler/config/jak2/ntsc_v1/art_info.jsonc @@ -45,7 +45,7 @@ // remap names for types in an entire file (higher priority) "file_override": { - "target-indax": {"target": "daxter-ag"} // in target-indax.gc, the remap for 'target' will be set to 'daxter-ag' + "target-indax": { "target": "daxter-ag" } // in target-indax.gc, the remap for 'target' will be set to 'daxter-ag' }, // some art groups (like robotboss-ag) have a name for their model that differs diff --git a/goal_src/jak2/pc/util/popup-menu-h.gc b/goal_src/jak2/pc/util/popup-menu-h.gc index 19d086e0ba9..a81d5877e35 100644 --- a/goal_src/jak2/pc/util/popup-menu-h.gc +++ b/goal_src/jak2/pc/util/popup-menu-h.gc @@ -10,7 +10,7 @@ ((label string) (on-press (function none))) (:methods - (draw! (_type_ font-context dma-buffer) none))) + (draw! (_type_ font-context dma-buffer) none))) (deftype popup-menu-label (popup-menu-entry) ()) @@ -23,8 +23,8 @@ ((entries (array popup-menu-entry)) (curr-entry-index int32)) (:methods - (draw! (_type_) none) - (move-up! (_type_) none) - (move-down! (_type_) none) - (press! (_type_) none) - (get-widest-label (_type_ font-context) float))) + (draw! (_type_) none) + (move-up! (_type_) none) + (move-down! (_type_) none) + (press! (_type_) none) + (get-widest-label (_type_ font-context) float))) diff --git a/goal_src/jak2/pc/util/popup-menu.gc b/goal_src/jak2/pc/util/popup-menu.gc index c05ff40d5aa..77f6aa3f435 100644 --- a/goal_src/jak2/pc/util/popup-menu.gc +++ b/goal_src/jak2/pc/util/popup-menu.gc @@ -17,32 +17,30 @@ (set! (-> font-ctx origin y) 75.0) (with-dma-buffer-add-bucket ((buf (-> (current-frame) global-buf)) (bucket-id debug-no-zbuf1)) ;; background border - (draw-sprite2d-xy buf 6 64 - (+ 17 (the int (get-widest-label this font-ctx))) ;; width - (+ 17 (* 15 (-> this entries length))) ;; height - (new 'static 'rgba :r 255 :g 255 :b 255 :a 75)) + (draw-sprite2d-xy buf + 6 + 64 + (+ 17 (the int (get-widest-label this font-ctx))) ;; width + (+ 17 (* 15 (-> this entries length))) ;; height + (new 'static 'rgba :r 255 :g 255 :b 255 :a 75)) ;; background - (draw-sprite2d-xy buf 7 65 - (+ 15 (the int (get-widest-label this font-ctx))) ;; width - (+ 15 (* 15 (-> this entries length))) ;; height - (new 'static 'rgba :r 0 :g 0 :b 0 :a 255)) + (draw-sprite2d-xy buf + 7 + 65 + (+ 15 (the int (get-widest-label this font-ctx))) ;; width + (+ 15 (* 15 (-> this entries length))) ;; height + (new 'static 'rgba :r 0 :g 0 :b 0 :a 255)) ;; menu contents (dotimes (i (-> this entries length)) (cond ;; TODO - probably just handle this in the draw methods - ((type? (-> this entries i) popup-menu-label) - (set! (-> font-ctx color) (font-color progress-option-off))) + ((type? (-> this entries i) popup-menu-label) (set! (-> font-ctx color) (font-color progress-option-off))) ((type? (-> this entries i) popup-menu-flag) (set! (-> font-ctx color) - (if (or ((-> (the-as popup-menu-flag (-> this entries i)) is-toggled?)) - (= i (-> this curr-entry-index))) - (font-color cyan) - (font-color default)))) - (else - (set! (-> font-ctx color) - (if (= i (-> this curr-entry-index)) - (font-color cyan) - (font-color default))))) + (if (or ((-> (the-as popup-menu-flag (-> this entries i)) is-toggled?)) (= i (-> this curr-entry-index))) + (font-color cyan) + (font-color default)))) + (else (set! (-> font-ctx color) (if (= i (-> this curr-entry-index)) (font-color cyan) (font-color default))))) (draw! (-> this entries i) font-ctx buf) (set! (-> font-ctx origin y) (+ 15.0 (-> font-ctx origin y)))))) (none)) @@ -57,13 +55,12 @@ (defmethod move-down! ((this popup-menu)) (set! (-> this curr-entry-index) (min (dec (-> this entries length)) (inc (-> this curr-entry-index)))) ;; skip labels - (when (type? (-> this entries (-> this curr-entry-index)) popup-menu-label) + (when (type? (-> this entries (-> this curr-entry-index)) popup-menu-label) (set! (-> this curr-entry-index) (min (dec (-> this entries length)) (inc (-> this curr-entry-index))))) (none)) (defmethod press! ((this popup-menu)) - (let ((entry (-> this entries (-> this curr-entry-index)))) - ((-> entry on-press))) + (let ((entry (-> this entries (-> this curr-entry-index)))) ((-> entry on-press))) (none)) (defmethod get-widest-label ((this popup-menu) (font-ctx font-context)) diff --git a/lsp/main.cpp b/lsp/main.cpp index 3fabc05ccfa..bfa7c26d041 100644 --- a/lsp/main.cpp +++ b/lsp/main.cpp @@ -36,7 +36,8 @@ void setup_logging(bool verbose, std::string log_file, bool disable_ansi_colors) { if (!log_file.empty()) { - lg::set_file(log_file, false, true, fs::path(log_file).parent_path().string()); + lg::set_file(fs::path(log_file).filename().string(), false, true, + fs::path(log_file).parent_path().string()); } if (verbose) { lg::set_file_level(lg::level::debug); diff --git a/test/common/formatter/corpus/cond-and-case.test.gc b/test/common/formatter/corpus/cond-and-case.test.gc new file mode 100644 index 00000000000..117b65c13e6 --- /dev/null +++ b/test/common/formatter/corpus/cond-and-case.test.gc @@ -0,0 +1,91 @@ +=== +Cond - Only Inlinable +=== + +(cond + ((type? (-> this entries i) popup-menu-label) + (set! x 1)) + ((type? (-> this entries i) popup-menu-flag) + (set! x 2)) + (else + (set! x 3))) + +--- + +(cond + ((type? (-> this entries i) popup-menu-label) (set! x 1)) + ((type? (-> this entries i) popup-menu-flag) (set! x 2)) + (else (set! x 3))) + +=== +Cond - Mix and Match Inlining +=== + +(cond + ((type? (-> this entries i) popup-menu-flag) + (set! x (+ 89716239781623717236123 1239817239817298319287312 31298371298371239817231 231892739182739129837 231892739182739129837 231892739182739129837))) + ((type? (-> this entries i) popup-menu-label) + (set! x 1)) + (else + (set! x 3))) + +--- + +(cond + ((type? (-> this entries i) popup-menu-flag) + (set! x + (+ 89716239781623717236123 + 1239817239817298319287312 + 31298371298371239817231 + 231892739182739129837 + 231892739182739129837 + 231892739182739129837))) + ((type? (-> this entries i) popup-menu-label) (set! x 1)) + (else (set! x 3))) + +=== +Case - Only Inlinable +=== + +(case (-> arg3 param 0) + (('dark) + (set! x 1)) + (('light) + (set! x 2)) + (else + (set! x 3))) + +--- + +(case (-> arg3 param 0) + (('dark) (set! x 1)) + (('light) (set! x 2)) + (else (set! x 3))) + +=== +Case - Mix and Match Inlining +=== + +(case (-> arg3 param 0) + (('dark) + (set! x 1)) + (('light) + (set! x (+ 89716239781623717236123 1239817239817298319287312 31298371298371239817231 231892739182739129837 231892739182739129837 231892739182739129837 231892739182739129837 231892739182739129837))) + (else + (set! x 3))) + +--- + +(case (-> arg3 param 0) + (('dark) (set! x 1)) + (('light) + (set! x + (+ 89716239781623717236123 + 1239817239817298319287312 + 31298371298371239817231 + 231892739182739129837 + 231892739182739129837 + 231892739182739129837 + 231892739182739129837 + 231892739182739129837))) + (else (set! x 3))) diff --git a/test/common/formatter/corpus/iteration.test.gc b/test/common/formatter/corpus/iteration.test.gc new file mode 100644 index 00000000000..cff4a3511ff --- /dev/null +++ b/test/common/formatter/corpus/iteration.test.gc @@ -0,0 +1,15 @@ +=== +dotimes +=== + +(dotimes (i (-> this entries length)) + (let ((label-len (-> (get-string-length (-> this entries i label) font-ctx) length))) + (when (> label-len max-len) + (set! max-len label-len)))) + +--- + +(dotimes (i (-> this entries length)) + (let ((label-len (-> (get-string-length (-> this entries i label) font-ctx) length))) + (when (> label-len max-len) + (set! max-len label-len)))) diff --git a/test/common/formatter/corpus/lists.test.gc b/test/common/formatter/corpus/lists.test.gc index de64bdafe72..2b7b337fbfe 100644 --- a/test/common/formatter/corpus/lists.test.gc +++ b/test/common/formatter/corpus/lists.test.gc @@ -22,3 +22,13 @@ Constant List - Too Long to Inline 444444444444444444444444444 555555555555555555555555555555555 666666666666666666666666666666666666666) + +=== +Empty List +=== + +() + +--- + +() diff --git a/test/common/formatter/corpus/types.test.gc b/test/common/formatter/corpus/types.test.gc new file mode 100644 index 00000000000..b8e44e0c6f1 --- /dev/null +++ b/test/common/formatter/corpus/types.test.gc @@ -0,0 +1,35 @@ +=== +Types - With Methods +=== + +(deftype popup-menu (basic) + ((entries (array popup-menu-entry)) + (curr-entry-index int32)) + (:methods + (draw! (_type_) none) + (move-up! (_type_) none) + (move-down! (_type_) none) + (press! (_type_) none) + (get-widest-label (_type_ font-context) float))) + +--- + +(deftype popup-menu (basic) + ((entries (array popup-menu-entry)) + (curr-entry-index int32)) + (:methods + (draw! (_type_) none) + (move-up! (_type_) none) + (move-down! (_type_) none) + (press! (_type_) none) + (get-widest-label (_type_ font-context) float))) + +=== +Types - Empty +=== + +(deftype popup-menu-label (popup-menu-entry) ()) + +--- + +(deftype popup-menu-label (popup-menu-entry) ()) diff --git a/tools/formatter/main.cpp b/tools/formatter/main.cpp index 35a2a706f73..6c38e3f30ce 100644 --- a/tools/formatter/main.cpp +++ b/tools/formatter/main.cpp @@ -47,13 +47,16 @@ int main(int argc, char** argv) { // TODO - support recursing directories // Read in source code const auto source_code = file_util::read_text_file(file_path); - const auto result = formatter::format_code(source_code); - if (write_newfile && result) { + if (result) { // TODO - i don't like this implementation, return a new string instead - if (str_util::replace(file_path, ".gc", ".new.gc")) { + if (write_inplace) { file_util::write_text_file(file_path, result.value()); + } else if (write_newfile) { + if (str_util::replace(file_path, ".gc", ".new.gc")) { + file_util::write_text_file(file_path, result.value()); + } } }