Raise syntax error when method argument name is keyword #5930
Conversation
Closes crystal-lang#5895 However some keywords are valid argument name still. Invalid argument names are listed here: begin nil true false yield with abstract def macro require case select if unless include extend class struct module enum while until return next break lib fun alias pointerof sizeof instance_sizeof typeof private protected asm end Above names except `end` are handled as keyword by `Crystal::Parser.parse_atomic_without_location`. It means, we cannot assign value into them and never reference them. In other words we have no way using them, so it does not make sense to allow these names as argument name. Additionally `end` is confusable. It maybe terminate `def` block.
| @@ -163,6 +163,20 @@ describe "Parser" do | |||
| assert_syntax_error "def foo!=; end", "unexpected token: !=" | |||
| assert_syntax_error "def foo?=(x); end", "unexpected token: ?" | |||
|
|
|||
| # #5895 | |||
| %w( | |||
| begin nil true false yield with abstract | |||
There was a problem hiding this comment.
The manual list is bad enough, let's not put in two places. Just add a few examples.
There was a problem hiding this comment.
And don't use any interpolation in the examples because tests should be "unrolled"
There was a problem hiding this comment.
Why bad enough? parser_spec.cr uses such a list in 10 places at least. And I found the implementation bug by this test before committing in fact.
spec/compiler/parser/parser_spec.cr
Outdated
| ).each do |kw| | ||
| assert_syntax_error "def foo(#{kw}); end", "cannot use '#{kw}' as argument name", 1, 9 | ||
| assert_syntax_error "def foo(foo #{kw}); end", "cannot use '#{kw}' as argument name", 1, 13 | ||
| it_parses "def foo(#{kw} foo); end", Def.new("foo", [Arg.new("foo", external_name: kw.to_s)]) |
There was a problem hiding this comment.
Nice to see that external names are not forgotten
| @@ -3588,6 +3592,10 @@ module Crystal | |||
|
|
|||
| case @token.type | |||
| when :IDENT | |||
| if @token.keyword? && invalid_internal_name?(@token.value) | |||
| raise "cannot use '#{@token}' as argument name", @token | |||
There was a problem hiding this comment.
... as an argument name?
ditto below
|
How about prohibiting use of such keywords as variable names too? |
| @@ -3648,6 +3659,24 @@ module Crystal | |||
| {arg_name, external_name, found_space, uses_arg} | |||
| end | |||
|
|
|||
| def invalid_internal_name?(keyword) | |||
| case keyword | |||
| # These names are handled as keyword by `parser_atomic_without_location`. | |||
There was a problem hiding this comment.
The reference should be parse_atomic_without_location (without the r). And I would add a note in Parser#parse_atomic_without_location as well to keep both list in sync.
There was a problem hiding this comment.
Thank you. I'll fix it.
|
@Sija At first we cannot use such keywords as variable names in reality currently. However there is an exception: begin : Int32This syntax is used with a macro normally. For example: property begin : Int32The first case should be prohibited and the second should be allowed. But I don't know how to implement this. Sorry. |
Closes #5895
However some keywords are valid argument name still.
Invalid argument names are listed here:
Above names except
endare handled as keyword byCrystal::Parser.parse_atomic_without_location.It means, we cannot assign value into them and never reference them.
In other words we have no way using them, so it does not make sense to allow these names as argument name.
Additionally
endis confusable. It maybe terminatedefblock.Thank you.