From b08fbd3cbaefd9918d20619e21f268f812a56884 Mon Sep 17 00:00:00 2001 From: Giovanni Cappellotto Date: Mon, 4 Jul 2022 13:13:19 -0400 Subject: [PATCH 1/3] Check that def, macro, and block parameters don't end with ? or ! Fix for #10917 --- spec/compiler/parser/parser_spec.cr | 19 +++++++++++++++++++ src/compiler/crystal/syntax/parser.cr | 11 +++++++++++ 2 files changed, 30 insertions(+) diff --git a/spec/compiler/parser/parser_spec.cr b/spec/compiler/parser/parser_spec.cr index d454d213da23..8cef168f4453 100644 --- a/spec/compiler/parser/parser_spec.cr +++ b/spec/compiler/parser/parser_spec.cr @@ -253,6 +253,25 @@ module Crystal assert_syntax_error "foo { |(#{kw}))| }", "cannot use '#{kw}' as a block parameter name", 1, 9 end + # #10917 + %w( + bar? bar! + ).each do |name| + assert_syntax_error "def foo(#{name}); end", "invalid param name", 1, 14 + assert_syntax_error "def foo(foo #{name}); end", "invalid param name", 1, 17 + it_parses "def foo(#{name} foo); end", Def.new("foo", [Arg.new("foo", external_name: name.to_s)]) + + assert_syntax_error "macro foo(#{name}); end", "invalid param name", 1, 16 + assert_syntax_error "macro foo(foo #{name}); end", "invalid param name", 1, 19 + it_parses "macro foo(#{name} foo); end", Macro.new("foo", [Arg.new("foo", external_name: name.to_s)], body: MacroLiteral.new(" ")) + + it_parses "foo(#{name})", Call.new(nil, "foo", [name.call] of ASTNode) + it_parses "foo #{name}", Call.new(nil, "foo", [name.call] of ASTNode) + + assert_syntax_error "foo { |#{name})| }", "invalid param name", 1, 12 + assert_syntax_error "foo { |(#{name}))| }", "invalid param name", 1, 13 + end + it_parses "def self.foo\n1\nend", Def.new("foo", body: 1.int32, receiver: "self".var) it_parses "def self.foo()\n1\nend", Def.new("foo", body: 1.int32, receiver: "self".var) it_parses "def self.foo=\n1\nend", Def.new("foo=", body: 1.int32, receiver: "self".var) diff --git a/src/compiler/crystal/syntax/parser.cr b/src/compiler/crystal/syntax/parser.cr index ead4e2c9ed5d..1ebfa28fe09d 100644 --- a/src/compiler/crystal/syntax/parser.cr +++ b/src/compiler/crystal/syntax/parser.cr @@ -3921,6 +3921,8 @@ module Crystal raise "when specified, external name must be different than internal name", @token end + check_valid_param_name(arg_name) + uses_arg = false do_next_token = true when .instance_var? @@ -3984,6 +3986,7 @@ module Crystal raise "cannot use '#{invalid_internal_name}' as a parameter name", invalid_internal_name end arg_name = external_name + check_valid_param_name(arg_name) else unexpected_token end @@ -4033,6 +4036,12 @@ module Crystal end end + def check_valid_param_name(param_name) + if param_name.ends_with?(/\?|!/) + raise "invalid param name" + end + end + def parse_if(check_end = true) location = @token.location @@ -4351,6 +4360,7 @@ module Crystal end arg_name = @token.value.to_s + check_valid_param_name(arg_name) if all_names.includes?(arg_name) raise "duplicated block parameter name: #{arg_name}", @token @@ -4372,6 +4382,7 @@ module Crystal end sub_arg_name = @token.value.to_s + check_valid_param_name(sub_arg_name) if all_names.includes?(sub_arg_name) raise "duplicated block parameter name: #{sub_arg_name}", @token From 9004ae8ff6fab685542e46e4ae7339eeeacc97c8 Mon Sep 17 00:00:00 2001 From: Giovanni Cappellotto Date: Tue, 20 Sep 2022 22:25:19 -0400 Subject: [PATCH 2/3] Use parser warnings instead of raising an exception I've turned failures to parse params with invalid names into syntax warnings similar to #12427, as suggested by @HertzDevil. --- spec/compiler/parser/parser_spec.cr | 12 ++++++------ spec/support/syntax.cr | 10 ++++++++++ src/compiler/crystal/syntax/parser.cr | 20 +++++++++++++------- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/spec/compiler/parser/parser_spec.cr b/spec/compiler/parser/parser_spec.cr index 8cef168f4453..529c5a3effe1 100644 --- a/spec/compiler/parser/parser_spec.cr +++ b/spec/compiler/parser/parser_spec.cr @@ -257,19 +257,19 @@ module Crystal %w( bar? bar! ).each do |name| - assert_syntax_error "def foo(#{name}); end", "invalid param name", 1, 14 - assert_syntax_error "def foo(foo #{name}); end", "invalid param name", 1, 17 + assert_syntax_warning "def foo(#{name}); end", "invalid parameter name: #{name}", 1, 14 + assert_syntax_warning "def foo(foo #{name}); end", "invalid parameter name: #{name}", 1, 17 it_parses "def foo(#{name} foo); end", Def.new("foo", [Arg.new("foo", external_name: name.to_s)]) - assert_syntax_error "macro foo(#{name}); end", "invalid param name", 1, 16 - assert_syntax_error "macro foo(foo #{name}); end", "invalid param name", 1, 19 + assert_syntax_warning "macro foo(#{name}); end", "invalid parameter name: #{name}", 1, 16 + assert_syntax_warning "macro foo(foo #{name}); end", "invalid parameter name: #{name}", 1, 19 it_parses "macro foo(#{name} foo); end", Macro.new("foo", [Arg.new("foo", external_name: name.to_s)], body: MacroLiteral.new(" ")) it_parses "foo(#{name})", Call.new(nil, "foo", [name.call] of ASTNode) it_parses "foo #{name}", Call.new(nil, "foo", [name.call] of ASTNode) - assert_syntax_error "foo { |#{name})| }", "invalid param name", 1, 12 - assert_syntax_error "foo { |(#{name}))| }", "invalid param name", 1, 13 + assert_syntax_warning "foo { |#{name}| }", "invalid parameter name: #{name}", 1, 12 + assert_syntax_warning "foo { |(#{name})| }", "invalid parameter name: #{name}", 1, 13 end it_parses "def self.foo\n1\nend", Def.new("foo", body: 1.int32, receiver: "self".var) diff --git a/spec/support/syntax.cr b/spec/support/syntax.cr index e1fd8f43d951..a1360b61e4e3 100644 --- a/spec/support/syntax.cr +++ b/spec/support/syntax.cr @@ -158,6 +158,16 @@ def assert_syntax_error(str, message = nil, line = nil, column = nil, metafile = end end +def assert_syntax_warning(str, message = nil, line = nil, column = nil, metafile = __FILE__, metaline = __LINE__, metaendline = __END_LINE__) + it "says syntax warning on #{str.inspect}", metafile, metaline, metaendline do + warnings = WarningCollection.new + parse str, false, nil, warnings + if warnings.infos.find { |info| info.ends_with?(message) }.nil? + fail "Expected warnings to include '#{message}'" + end + end +end + def parse(string, wants_doc = false, filename = nil, warnings = nil) parser = Parser.new(string, warnings: warnings) parser.warnings = warnings if warnings diff --git a/src/compiler/crystal/syntax/parser.cr b/src/compiler/crystal/syntax/parser.cr index 1ebfa28fe09d..2c1476755875 100644 --- a/src/compiler/crystal/syntax/parser.cr +++ b/src/compiler/crystal/syntax/parser.cr @@ -3887,9 +3887,11 @@ module Crystal do_next_token = true found_string_literal = false invalid_internal_name = nil + external_name_token = nil if allow_external_name && (@token.type.ident? || string_literal_start?) name_location = @token.location + external_name_token = @token.dup if @token.type.ident? if @token.keyword? && invalid_internal_name?(@token.value) invalid_internal_name = @token.dup @@ -3921,7 +3923,7 @@ module Crystal raise "when specified, external name must be different than internal name", @token end - check_valid_param_name(arg_name) + check_valid_param_name uses_arg = false do_next_token = true @@ -3986,7 +3988,10 @@ module Crystal raise "cannot use '#{invalid_internal_name}' as a parameter name", invalid_internal_name end arg_name = external_name - check_valid_param_name(arg_name) + if external_name_token.nil? + raise "missing external name token" + end + check_valid_param_name(external_name_token) else unexpected_token end @@ -4036,9 +4041,10 @@ module Crystal end end - def check_valid_param_name(param_name) - if param_name.ends_with?(/\?|!/) - raise "invalid param name" + def check_valid_param_name(token : Token = @token) + param_name = token.value.to_s + if param_name[-1]?.in?('?', '!') + warnings.add_warning_at(token.location, "invalid parameter name: #{param_name}") end end @@ -4360,7 +4366,7 @@ module Crystal end arg_name = @token.value.to_s - check_valid_param_name(arg_name) + check_valid_param_name if all_names.includes?(arg_name) raise "duplicated block parameter name: #{arg_name}", @token @@ -4382,7 +4388,7 @@ module Crystal end sub_arg_name = @token.value.to_s - check_valid_param_name(sub_arg_name) + check_valid_param_name if all_names.includes?(sub_arg_name) raise "duplicated block parameter name: #{sub_arg_name}", @token From 7ba3ed3d6cd9802061a2a322fdf87b3ac1b50bd1 Mon Sep 17 00:00:00 2001 From: Oleh Prypin Date: Sun, 5 Jan 2025 18:39:52 +0100 Subject: [PATCH 3/3] Expand and simplify specs --- spec/compiler/parser/parser_spec.cr | 15 +++++++++------ spec/support/syntax.cr | 4 ++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/spec/compiler/parser/parser_spec.cr b/spec/compiler/parser/parser_spec.cr index 6829f8ecbd5b..0445974e06d5 100644 --- a/spec/compiler/parser/parser_spec.cr +++ b/spec/compiler/parser/parser_spec.cr @@ -338,19 +338,22 @@ module Crystal %w( bar? bar! ).each do |name| - assert_syntax_warning "def foo(#{name}); end", "invalid parameter name: #{name}", 1, 14 - assert_syntax_warning "def foo(foo #{name}); end", "invalid parameter name: #{name}", 1, 17 + assert_syntax_warning "def foo(#{name}); end", "invalid parameter name: #{name}" + assert_syntax_warning "def foo(foo #{name}); end", "invalid parameter name: #{name}" it_parses "def foo(#{name} foo); end", Def.new("foo", [Arg.new("foo", external_name: name.to_s)]) - assert_syntax_warning "macro foo(#{name}); end", "invalid parameter name: #{name}", 1, 16 - assert_syntax_warning "macro foo(foo #{name}); end", "invalid parameter name: #{name}", 1, 19 + assert_syntax_warning "macro foo(#{name}); end", "invalid parameter name: #{name}" + assert_syntax_warning "macro foo(foo #{name}); end", "invalid parameter name: #{name}" it_parses "macro foo(#{name} foo); end", Macro.new("foo", [Arg.new("foo", external_name: name.to_s)], body: MacroLiteral.new(" ")) it_parses "foo(#{name})", Call.new(nil, "foo", [name.call] of ASTNode) it_parses "foo #{name}", Call.new(nil, "foo", [name.call] of ASTNode) - assert_syntax_warning "foo { |#{name}| }", "invalid parameter name: #{name}", 1, 12 - assert_syntax_warning "foo { |(#{name})| }", "invalid parameter name: #{name}", 1, 13 + assert_syntax_warning "foo { |#{name}| }", "invalid parameter name: #{name}" + assert_syntax_warning "foo { |foo, (#{name})| }", "invalid parameter name: #{name}" + + assert_syntax_warning "foo do |foo, #{name}|\nend", "invalid parameter name: #{name}" + assert_syntax_warning "foo do |(#{name})|\nend", "invalid parameter name: #{name}" end it_parses "def self.foo\n1\nend", Def.new("foo", body: 1.int32, receiver: "self".var) diff --git a/spec/support/syntax.cr b/spec/support/syntax.cr index 689bd5c8fb18..df45df7d9648 100644 --- a/spec/support/syntax.cr +++ b/spec/support/syntax.cr @@ -158,11 +158,11 @@ def assert_syntax_error(str, message = nil, line = nil, column = nil, metafile = end end -def assert_syntax_warning(str, message = nil, line = nil, column = nil, metafile = __FILE__, metaline = __LINE__, metaendline = __END_LINE__) +def assert_syntax_warning(str, message, *, metafile = __FILE__, metaline = __LINE__, metaendline = __END_LINE__) it "says syntax warning on #{str.inspect}", metafile, metaline, metaendline do warnings = WarningCollection.new parse str, false, nil, warnings - if warnings.infos.find { |info| info.ends_with?(message) }.nil? + unless warnings.infos.find(&.ends_with?(message)) fail "Expected warnings to include '#{message}'" end end