-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Raise syntax error when method argument name is keyword #5930
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And don't use any interpolation in the examples because tests should be "unrolled"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... as an argument name
?
ditto below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good 👍 💯 🎉
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 : Int32 This syntax is used with a macro normally. For example: property begin : Int32 The 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
end
are 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
end
is confusable. It maybe terminatedef
block.Thank you.