Skip to content
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

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

Open
HertzDevil opened this issue Jul 10, 2021 · 6 comments · May be fixed by #12197
Open

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

HertzDevil opened this issue Jul 10, 2021 · 6 comments · May be fixed by #12197
Labels
breaking-change help wanted This issue is generally accepted and needs someone to pick it up status:discussion topic:compiler:parser topic:lang

Comments

@HertzDevil
Copy link
Contributor

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.

@HertzDevil HertzDevil added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Jul 10, 2021
@HertzDevil HertzDevil changed the title Def, macro, and block arguments could end with ? or ! Def, macro, and block parameters could end with ? or ! Jul 10, 2021
@vlazar
Copy link
Contributor

vlazar commented Jul 10, 2021

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 straight-shoota added topic:compiler:parser status:discussion topic:lang breaking-change and removed kind:bug A bug in the code. Does not apply to documentation, specs, etc. labels Jul 10, 2021
@straight-shoota
Copy link
Member

straight-shoota commented Jul 12, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

See also: #2617 (comment)

@straight-shoota straight-shoota added the help wanted This issue is generally accepted and needs someone to pick it up label Sep 16, 2021
@potomak
Copy link
Contributor

potomak commented Jul 3, 2022

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
Copy link
Contributor

potomak commented Jul 4, 2022

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.

potomak added a commit to potomak/crystal that referenced this issue Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change help wanted This issue is generally accepted and needs someone to pick it up status:discussion topic:compiler:parser topic:lang
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants