search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.51k stars 1.62k forks source link

[Interpreter] Auto-indentation don't work everywhere #13115

Open I3oris opened 1 year ago

I3oris commented 1 year ago

Bug Report

Hello!,

The auto-indentation on crystal interpreter shell doesn't work everywhere.

For example, it works on classes (indentation of 1 is added after typing 'enter'): Snapshot_2023-02-24_00-16-16 But not for if: Snapshot_2023-02-24_00-17-18 Or brackets: Snapshot_2023-02-24_00-39-16

Ditto for lib, enum, case, begin, while, (, {, do, macro if, macro for, and union.

The reason is because we use the Crystal::Parser#type_nest, #def_nest and #fun_nest to retrieve the level of indention to apply. However those doesn't take in account the if or brackets. https://github.com/crystal-lang/crystal/blob/94c1f02d64ff8da176e82739321e911fc921dc9d/src/compiler/crystal/interpreter/repl_reader.cr#L86

My Proposal

Modify the parser to add a nest attribute, that count more nestable thing.

Maybe, if necessary, create a subclass of the parser to this modification so the initial parser is not impacted. However this small modification seems to make no detectable difference of perfos.

crystal -v

Crystal 1.7.2 (2023-01-30)

I3oris commented 1 year ago

I did the following benchmark to see the impact of my modifications:

parser_benchmark.cr:

require "./src/compiler/crystal/syntax"
require "benchmark"

# Test the parser against the parser file itself. (~ 6k loc)
source = File.read("src/compiler/crystal/syntax/parser.cr")

parser = Crystal::Parser.new(source)

puts(Benchmark.measure do
  1000.times do
    parser.current_pos = 0
    parser.parse
  end
end)

modifications.patch:

diff --git a/src/compiler/crystal/syntax/parser.cr b/src/compiler/crystal/syntax/parser.cr
index cb9a9124e..21b307d25 100644
--- a/src/compiler/crystal/syntax/parser.cr
+++ b/src/compiler/crystal/syntax/parser.cr
@@ -17,6 +17,8 @@ module Crystal
     property def_nest : Int32
     property fun_nest : Int32
     property type_nest : Int32
+    property nest : Int32
+
     getter? wants_doc : Bool
     @block_arg_name : String?

@@ -36,6 +38,7 @@ module Crystal
       @def_nest = 0
       @fun_nest = 0
       @type_nest = 0
+      @nest = 0
       @is_constant_assignment = false

       # Keeps track of current call args starting locations,
@@ -1378,6 +1381,7 @@ module Crystal
     end

     def parse_begin
+      @nest += 1
       begin_location = @token.location
       slash_is_regex!
       next_token_skip_statement_end
@@ -1386,6 +1390,7 @@ module Crystal
       if !node.is_a?(ExceptionHandler) && (!node.is_a?(Expressions) || !node.keyword.none?)
         node = Expressions.new([node])
       end
+      @nest -= 1
       node.at(begin_location).at_end(end_location)
       node.keyword = :begin if node.is_a?(Expressions)
       node
@@ -1524,6 +1529,7 @@ module Crystal
     end

     def parse_while_or_until(klass)
+      @nest += 1
       slash_is_regex!
       next_token_skip_space_or_newline

@@ -1539,6 +1545,7 @@ module Crystal
       check_ident :end
       next_token_skip_space

+      @nest -= 1
       klass.new(cond, body).at_end(end_location)
     end

@@ -1675,6 +1682,7 @@ module Crystal

     def parse_class_def(is_abstract = false, is_struct = false, doc = nil)
       @type_nest += 1
+      @nest += 1

       doc ||= @token.doc

@@ -1706,6 +1714,7 @@ module Crystal
       next_token_skip_space

       @type_nest -= 1
+      @nest -= 1

       class_def = ClassDef.new name, body, superclass, type_vars, is_abstract, is_struct, splat_index
       class_def.doc = doc
@@ -1760,6 +1769,7 @@ module Crystal

     def parse_module_def
       @type_nest += 1
+      @nest += 1

       location = @token.location
       doc = @token.doc
@@ -1780,6 +1790,7 @@ module Crystal
       next_token_skip_space

       @type_nest -= 1
+      @nest -= 1

       module_def = ModuleDef.new name, body, type_vars, splat_index
       module_def.doc = doc
@@ -1811,6 +1822,7 @@ module Crystal
     end

     def parse_parenthesized_expression
+      @nest += 1
       location = @token.location
       slash_is_regex!
       next_token_skip_space_or_newline
@@ -1819,6 +1831,7 @@ module Crystal
         end_location = token_end_location
         node = Expressions.new([Nop.new] of ASTNode).at(location).at_end(end_location)
         node.keyword = :paren
+        @nest -= 1
         return node_and_next_token node
       end

@@ -1856,6 +1869,7 @@ module Crystal

       unexpected_token if @token.type.op_lparen?

+      @nest -= 1
       node = Expressions.new(exps).at(location).at_end(end_location)
       node.keyword = :paren
       node
@@ -1863,6 +1877,7 @@ module Crystal

     def parse_fun_literal
       location = @token.location
+      @nest += 1

       next_token_skip_space_or_newline

@@ -1873,7 +1888,10 @@ module Crystal
       when .op_lparen?, .op_lcurly?, .op_colon?
         # do nothing
       else
-        return parse_fun_pointer unless @token.keyword?(:do)
+        unless @token.keyword?(:do)
+          @nest -= 1
+          return parse_fun_pointer
+        end
       end

       params = [] of Arg
@@ -1922,6 +1940,7 @@ module Crystal
           unexpected_token
         end

+        @nest -= 1
         a_def = Def.new("->", params, body, return_type: return_type).at(location).at_end(end_location)
         ProcLiteral.new(a_def).at(location).at_end(end_location)
       end
@@ -2410,6 +2429,7 @@ module Crystal
     end

     def parse_array_literal
+      @nest += 1
       line = @line_number
       column = @token.column_number

@@ -2456,6 +2476,7 @@ module Crystal
         raise "for empty arrays use '[] of ElementType'", line, column
       end

+      @nest -= 1
       ArrayLiteral.new(exps, of).at_end(end_location)
     end

@@ -2528,6 +2549,13 @@ module Crystal
       end
     end

+    def parse_hash_or_tuple_literal(allow_of = true)
+      @nest += 1
+      node = previous_def
+      @nest -= 1
+      node
+    end
+
     def parse_hash_literal(first_key, location, allow_of)
       line = @line_number
       column = @token.column_number
@@ -2725,6 +2753,7 @@ module Crystal
     end

     def parse_case
+      @nest += 1
       slash_is_regex!
       next_token_skip_space_or_newline
       while @token.type.op_semicolon?
@@ -2840,6 +2869,7 @@ module Crystal
         end
       end

+      @nest -= 1
       Case.new(cond, whens, a_else, exhaustive.nil? ? false : exhaustive)
     end

@@ -2948,6 +2978,7 @@ module Crystal
     end

     def parse_select
+      @nest += 1
       slash_is_regex!
       next_token_skip_space
       skip_statement_end
@@ -2998,6 +3029,7 @@ module Crystal
         end
       end

+      @nest -= 1
       Select.new(whens, a_else)
     end

@@ -3039,6 +3071,7 @@ module Crystal
     def parse_to_def(a_def)
       prepare_parse_def
       @def_nest += 1
+      @nest += 1

       result = parse

@@ -3086,6 +3119,8 @@ module Crystal
     end

     def parse_macro
+      @nest += 1
+
       doc = @token.doc

       # Force lexer return if possible a def or macro name
@@ -3182,6 +3217,7 @@ module Crystal
           body, end_location = parse_macro_body(name_location)
         end

+        @nest -= 1
         node = Macro.new name, params, body, block_param, splat_index, double_splat: double_splat
         node.name_location = name_location
         node.doc = doc
@@ -3214,7 +3250,9 @@ module Crystal
           check_macro_expression_end
           skip_whitespace = check_macro_skip_whitespace
         when .macro_control_start?
+          @nest += 1
           macro_control = parse_macro_control(start_location, macro_state)
+          @nest -= 1
           if macro_control
             skip_space_or_newline
             check :OP_PERCENT_RCURLY
@@ -3324,6 +3362,7 @@ module Crystal
     end

     def parse_percent_macro_control
+      @nest += 1
       raise "can't nest macro expressions", @token if @in_macro_expression

       macro_control = parse_macro_control(@token.location)
@@ -3331,6 +3370,7 @@ module Crystal
         skip_space_or_newline
         check :OP_PERCENT_RCURLY
         next_token_skip_space
+        @nest -= 1
         macro_control
       else
         unexpected_token_in_atomic
@@ -3524,6 +3564,7 @@ module Crystal
     def parse_def_helper(is_abstract = false)
       @doc_enabled = false
       @def_nest += 1
+      @nest += 1

       # At this point we want to attach the "do" to calls inside the def,
       # not to calls that might have this def as a macro argument.
@@ -3732,6 +3773,7 @@ module Crystal
       end

       @def_nest -= 1
+      @nest -= 1
       @doc_enabled = !!@wants_doc

       node = Def.new name, params, body, receiver, block_param, return_type, @is_macro_def, @block_arity, is_abstract, splat_index, double_splat: double_splat, free_vars: free_vars
@@ -4129,13 +4171,17 @@ module Crystal
     end

     def parse_if(check_end = true)
+      @nest += 1 if check_end
       location = @token.location

       slash_is_regex!
       next_token_skip_space_or_newline

       cond = parse_op_assign_no_control allow_suffix: false
-      parse_if_after_condition cond, location, check_end
+      node = parse_if_after_condition cond, location, check_end
+
+      @nest -= 1 if check_end
+      node
     end

     def parse_if_after_condition(cond, location, check_end)
@@ -4171,13 +4217,16 @@ module Crystal
     end

     def parse_unless
+      @nest += 1
       location = @token.location

       slash_is_regex!
       next_token_skip_space_or_newline

       cond = parse_op_assign_no_control allow_suffix: false
-      parse_unless_after_condition(cond, location)
+      node = parse_unless_after_condition(cond, location)
+      @nest -= 1
+      node
     end

     def parse_unless_after_condition(cond, location)
@@ -4399,16 +4448,24 @@ module Crystal
     end

     def parse_block(block, stop_on_do = false)
-      if @token.keyword?(:do)
-        return block if stop_on_do
+      @nest += 1
+      node =
+        if @token.keyword?(:do)
+          if stop_on_do
+            @nest -= 1
+            return block
+          end

-        raise "block already specified with &" if block
-        parse_block2 do |body|
-          parse_exception_handler body, implicit: true
+          raise "block already specified with &" if block
+          parse_block2 do |body|
+            parse_exception_handler body, implicit: true
+          end
+        else
+          parse_curly_block(block)
         end
-      else
-        parse_curly_block(block)
-      end
+
+      @nest -= 1
+      node
     end

     def parse_curly_block(block)
@@ -4639,6 +4696,13 @@ module Crystal
       end
     end

+    def parse_call_args(stop_on_do_after_space = false, allow_curly = false, control = false)
+      @nest += 1
+      ret = previous_def
+      @nest -= 1
+      ret
+    end
+
     def parse_call_args_space_consumed(check_plus_and_minus = true, allow_curly = false, control = false, end_token : Token::Kind = :OP_RPAREN,
                                        allow_beginless_range = false)
       if @token.keyword?(:end) && !next_comes_colon_space?
@@ -5584,6 +5648,7 @@ module Crystal
     end

     def parse_lib
+      @nest += 1
       location = @token.location
       next_token_skip_space_or_newline

@@ -5597,6 +5662,7 @@ module Crystal
       end_location = token_end_location
       next_token_skip_space

+      @nest -= 1
       lib_def = LibDef.new(name, body).at(location).at_end(end_location)
       lib_def.name_location = name_location
       lib_def
@@ -5915,6 +5981,7 @@ module Crystal
     end

     def parse_c_struct_or_union(union : Bool)
+      @nest += 1
       location = @token.location
       next_token_skip_space_or_newline
       name = check_const
@@ -5924,6 +5991,7 @@ module Crystal
       end_location = token_end_location
       next_token_skip_space

+      @nest -= 1
       CStructOrUnionDef.new(name, Expressions.from(body), union: union).at(location).at_end(end_location)
     end

@@ -5991,6 +6059,7 @@ module Crystal
     def parse_enum_def
       location = @token.location
       doc = @token.doc
+      @nest += 1

       next_token_skip_space_or_newline

@@ -6014,6 +6083,7 @@ module Crystal
       end_location = token_end_location
       next_token_skip_space

+      @nest -= 1
       enum_def = EnumDef.new name, members, base_type
       enum_def.doc = doc
       enum_def.at(location).at_end(end_location)

Test without modifications:

crystal build --release parser_benchmark.cr (CPU time/system time/sum/real time) ./parser_benchmark # replay 1 => 5.472998 0.000000 5.472998 ( 5.477665) ./parser_benchmark # replay 2 => 5.418210 0.001784 5.419994 ( 5.424658) ./parser_benchmark # replay 3 => 5.433616 0.003326 5.436942 ( 5.441782) ./parser_benchmark # replay 4 => 5.403801 0.000000 5.403801 ( 5.410336) ./parser_benchmark # replay 5 => 5.305517 0.006657 5.312174 ( 5.317298)

Test with modifications:

git apply modifications.patch crystal build --release parser_benchmark.cr ./parser_benchmark # replay 1 => 5.010850 0.003328 5.014178 ( 5.018396) ./parser_benchmark # replay 2 => 5.009936 0.001801 5.011737 ( 5.015780) ./parser_benchmark # replay 3 => 5.010802 0.006663 5.017465 ( 5.021573) ./parser_benchmark # replay 4 => 5.108364 0.000000 5.108364 ( 5.113850) ./parser_benchmark # replay 5 => 5.064556 0.003330 5.067886 ( 5.072312)

Surprisingly, the modifications even make the parser faster! (may due to a different compiler optimization??) Could you test on your end if you reproduce the same results?

Thanks!