Skip to content

Commit

Permalink
Use parser warnings instead of raising an exception
Browse files Browse the repository at this point in the history
I've turned failures to parse params with invalid names into syntax
warnings similar to #12427, as suggested by @HertzDevil.
  • Loading branch information
potomak committed Sep 21, 2022
1 parent b08fbd3 commit 9004ae8
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 13 deletions.
12 changes: 6 additions & 6 deletions spec/compiler/parser/parser_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 10 additions & 0 deletions spec/support/syntax.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 13 additions & 7 deletions src/compiler/crystal/syntax/parser.cr
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 9004ae8

Please sign in to comment.