crystal-lang / crystal

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

Def, macro, and block parameters could end with `?` or `!` #10917

Open HertzDevil opened 3 years ago

HertzDevil commented 3 years ago

The following code is valid in Crystal but not in Ruby:

def foo(x?, y!)
  x? + y!
end

foo 1, 2

{"foo" => "bar"}.each do |key?, value!|
  puts key? + value!
end

I would expect ? and ! to always denote a call rather than a variable name.

vlazar commented 3 years ago

Interestingly enough I sometimes wish I can use ? suffix in variable names in Ruby, so that I'm not forced to use prefix like is_valid to show it's boolean and use valid? instead just like with method names.

straight-shoota commented 3 years ago

I agree on the expectation that ? and ! suffixes denote a call. They can't be used for regular local variables, so I think it makes much sense to restrict parameters the same.

This can technically be seen as a parser bug (it unintentionally allows these names). But at this point, it's also a compiler feature (even if undocumented) and changing it could potentially break existing code. I don't think it's urgent to disallow these parameter names, so this should probably wait for 2.0.

lbguilherme commented 3 years ago

If the parser were updated to show a deprecated warning on these, but still parse, it wouldn't be a breaking change, right? This could be the way forward for compiler changes waiting for 2.0.

HertzDevil commented 3 years ago

See also: https://github.com/crystal-lang/crystal/issues/2617#issuecomment-220323655

potomak commented 2 years ago

If this issue is still relevant can I claim it?

I found in the parser the piece of code that parses functions. My plan was to add a test and update the code to output a warning in case input code uses ‘?’ or ‘!’ in function parameters identifiers as suggested in the comments to keep the backward compatibility.

Does it sound good?

potomak commented 2 years ago

I've published a #12197 to fix this issue. I decided to raise an exception because I didn't find how to log a warning + test warning message. In parser_spec.cr I just found it_parses and assert_syntax_error. See discussion in the PR.