[Ruby 3.0 support] Kernel#lambda now warns if called without a literal block

[andrykonchin]

The PR fixes the following item:

[easy] Kernel#lambda now warns if called without a literal block.
[Feature #15973]

Related issue - #2453

Changes

• added spec for Kernel#lambda
• added the warning

Concerns

I am not sure whether this PR should contain changes to the RubySpec tests. Maybe it will be better to create a PR directly to RubySpec.

Also, I haven't found any place for unit tests for changes in the KernelNodes class.

Regarding the new RubySpec test - I am confused a bit. While I was reading https://bugs.ruby-lang.org/issues/15973 I was under impression that any argument like &... should cause a warning. A commit mentioned in the ticket contains tests with lambda(&:symbol) that triggers a warning. But actually on Ruby 3.0.0.p0 lambda(&:symbol) doesn't trigger a warning... Only lambda(&proc)...

Please let me know if I need to change anything.

[graalvmbot]

Hello Andrew Konchin, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address andry -(dot)- konchin -(at)- gmail -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

[andrykonchin]

I have signed the contributor agreement. Please check.

The link to the contributor agreement in the comment above is broken. I used a link mentioned here:

https://www.oracle.com/technical-resources/oracle-contributor-agreement.html

[graalvmbot]

Andrew Konchin has signed the Oracle Contributor Agreement (based on email address andry -(dot)- konchin -(at)- gmail -(dot)- com) so can contribute to this repository.

[eregon]

Thank you for the PR, it looks good.
Could you apply my suggestions?

[eregon]

I am not sure whether this PR should contain changes to the RubySpec tests. Maybe it will be better to create a PR directly to RubySpec.

Either is fine and both have pros & cons.
Adding them here means it's 1 PR vs 2 and we can directly test the TruffleRuby behavior (needs to set PRETEND_RUBY_VERSION=3.0.2, see here). Adding to ruby/spec means slightly less chance for conflict.
So here is good, and I'll sync specs soon anyway.

Also, I haven't found any place for unit tests for changes in the KernelNodes class.

A spec in ruby/spec is the way to go.

While I was reading https://bugs.ruby-lang.org/issues/15973 I was under impression that any argument like &... should cause a warning

Indeed there was a test added in ruby/ruby@2188d6d#diff-8b146f8f5073c5ee6526a128c3e9c05ae7a52d2cd496f601351470137f2a87a0
but it was removed in ruby/ruby@2188d6d#diff-8b146f8f5073c5ee6526a128c3e9c05ae7a52d2cd496f601351470137f2a87a0
I think I figured it out, see https://bugs.ruby-lang.org/issues/15973#note-50

[andrykonchin]

I think I figured it out, see https://bugs.ruby-lang.org/issues/15973#note-50

Cool, thank you. I will try to reflect it in the tests.

[andrykonchin]

Could you please suggest if there is any naming convention for commits? Also, should multiple working commits be squashed?

[eregon]

Could you please suggest if there is any naming convention for commits? Also, should multiple working commits be squashed?

No specific convention, just of the description of what the commit does.
Yes, please squash working commits, it's fine to mix specs and implementation changes in the same commit (although it can be a little bit weird then for the commit description).

[andrykonchin]

Done.

[eregon]

Looks great, thank you!

[eregon]

I pushed a couple fixes as CI was failing for MRI tests.
WarningNode was not handling shouldWarnForDeprecation() correctly, and also WarnNode should be used instead of WarningNode since the warning happens even with $VERBOSE = false and not only with $VERBOSE = true (like other deprecation warnings).

[andrykonchin]

Thank you 🙇