[GR-59866] Emit a warning when it call without arguments is used in…

… a block without parameters

PullRequest: truffleruby/4408

CHANGELOG.md

@@ -35,6 +35,7 @@ Compatibility:
 * `Thread::Queue#freeze` now raises `TypeError` when called (#3681, @Th3-M4jor).
 * `Thread::SizedQueue#freeze` now raises `TypeError` when called (#3681, @Th3-M4jor).
 * Add `Range#reverse_each` (#3681, @andrykonchin).
+* Emit a warning when `it` call without arguments is used in a block without parameters (#3681, @andrykonchin).
 
 Performance:
 

spec/ruby/language/block_spec.rb

@@ -1072,3 +1072,32 @@ def all_kwrest(arg1, arg2, *rest, post1, post2, kw1: 1, kw2: 2, okw1:, okw2:, **
     end
   end
 end
+
+describe "`it` calls without arguments in a block with no ordinary parameters" do
+  ruby_version_is "3.3"..."3.4" do
+    it "emits a deprecation warning" do
+      -> {
+        eval "proc { it }"
+      }.should complain(/warning: `it` calls without arguments will refer to the first block param in Ruby 3.4; use it\(\) or self.it/)
+    end
+
+    it "does not emit a deprecation warning when a block has parameters" do
+      -> { eval "proc { |a, b| it }" }.should_not complain
+      -> { eval "proc { |*rest| it }" }.should_not complain
+      -> { eval "proc { |*| it }" }.should_not complain
+      -> { eval "proc { |a:, b:| it }" }.should_not complain
+      -> { eval "proc { |**kw| it }" }.should_not complain
+      -> { eval "proc { |**| it }" }.should_not complain
+      -> { eval "proc { |&block| it }" }.should_not complain
+      -> { eval "proc { |&| it }" }.should_not complain
+    end
+
+    it "does not emit a deprecation warning when `it` calls with arguments" do
+      -> { eval "proc { it(42) }" }.should_not complain
+    end
+
+    it "does not emit a deprecation warning when `it` calls with explicit empty arguments list" do
+      -> { eval "proc { it() }" }.should_not complain
+    end
+  end
+end

src/main/java/org/truffleruby/language/methods/Arity.java

@@ -115,6 +115,10 @@ public boolean acceptsKeywords() {
         return hasKeywords() || hasKeywordsRest();
     }
 
+    public boolean acceptsPositionalArguments() {
+        return getRequired() > 0 || getOptional() > 0 || hasRest();
+    }
+
     public boolean hasKeywords() {
         return keywordArguments.length != 0;
     }

src/main/java/org/truffleruby/parser/YARPBlockNodeTranslator.java

@@ -43,10 +43,17 @@
 public final class YARPBlockNodeTranslator extends YARPTranslator {
 
     private final Arity arity;
+    /** Whether a block itself accepts a block parameter (&block). The Arity class does not contain this information. */
+    private final boolean acceptsBlockParameter;
 
-    public YARPBlockNodeTranslator(TranslatorEnvironment environment, Arity arity) {
-        super(environment);
+    public YARPBlockNodeTranslator(
+            TranslatorEnvironment environment,
+            Arity arity,
+            boolean acceptsBlockParameter,
+            RubyDeferredWarnings rubyWarnings) {
+        super(environment, rubyWarnings);
         this.arity = arity;
+        this.acceptsBlockParameter = acceptsBlockParameter;
     }
 
     public RubyNode compileBlockNode(Nodes.Node body, Nodes.ParametersNode parameters, String[] locals,
@@ -310,4 +317,22 @@ private boolean shouldConsiderDestructuringArrayArg(Arity arity) {
         }
     }
 
+    @Override
+    public RubyNode visitCallNode(Nodes.CallNode node) {
+        // emit a warning if `it` is called without arguments in a block without ordinal parameters
+        boolean noBlockParameters = !arity.acceptsKeywords() && !arity.acceptsPositionalArguments() &&
+                !acceptsBlockParameter;
+        if (node.name.equals("it") && node.receiver == null && node.isVariableCall() && noBlockParameters) {
+            SourceSection section = rubySource.getSource().createSection(node.startOffset, node.length);
+
+            String sourcePath = rubySource.getSourcePath(language).intern();
+            int lineNumber = section.getStartLine() + rubySource.getLineOffset();
+            String message = "`it` calls without arguments will refer to the first block param in Ruby 3.4; use it() or self.it";
+
+            rubyWarnings.warn(sourcePath, lineNumber, message);
+        }
+
+        return super.visitCallNode(node);
+    }
+
 }

src/main/java/org/truffleruby/parser/YARPDefNodeTranslator.java

@@ -24,8 +24,9 @@ public final class YARPDefNodeTranslator extends YARPTranslator {
 
     public YARPDefNodeTranslator(
             RubyLanguage language,
-            TranslatorEnvironment environment) {
-        super(environment);
+            TranslatorEnvironment environment,
+            RubyDeferredWarnings rubyWarnings) {
+        super(environment, rubyWarnings);
 
         if (parseEnvironment.parserContext.isEval() || parseEnvironment.isCoverageEnabled()) {
             shouldLazyTranslate = false;

src/main/java/org/truffleruby/parser/YARPTranslator.java

@@ -175,6 +175,9 @@ public class YARPTranslator extends YARPBaseTranslator {
     public static final RescueNode[] EMPTY_RESCUE_NODE_ARRAY = new RescueNode[0];
 
     public Deque<Integer> frameOnStackMarkerSlotStack = new ArrayDeque<>();
+
+    protected RubyDeferredWarnings rubyWarnings;
+
     /** whether a while-loop body is translated; needed to check correctness of operators like break/next/etc */
     private boolean translatingWhile = false;
     /** whether a for-loop body is translated; needed to enforce variables in the for-loop body to be declared outside
@@ -198,8 +201,9 @@ public class YARPTranslator extends YARPBaseTranslator {
     /** all the encountered BEGIN {} blocks; they will be added finally at the beginning of the program AST */
     private final ArrayList<RubyNode> beginBlocks = new ArrayList<>();
 
-    public YARPTranslator(TranslatorEnvironment environment) {
+    public YARPTranslator(TranslatorEnvironment environment, RubyDeferredWarnings rubyWarnings) {
         super(environment);
+        this.rubyWarnings = rubyWarnings;
     }
 
     public RubyNode[] getBeginBlocks() {
@@ -537,7 +541,9 @@ private RubyNode translateBlockAndLambda(Nodes.Node node, Nodes.Node parametersN
 
         final YARPBlockNodeTranslator methodCompiler = new YARPBlockNodeTranslator(
                 newEnvironment,
-                arity);
+                arity,
+                parameters.block != null,
+                rubyWarnings);
 
         methodCompiler.frameOnStackMarkerSlotStack = frameOnStackMarkerSlotStack;
 
@@ -1548,7 +1554,8 @@ public RubyNode visitDefNode(Nodes.DefNode node) {
 
         final var defNodeTranslator = new YARPDefNodeTranslator(
                 language,
-                newEnvironment);
+                newEnvironment,
+                rubyWarnings);
         var callTargetSupplier = defNodeTranslator.buildMethodNodeCompiler(node, parameters, arity);
 
         final boolean isDefSingleton = singletonClassNode != null;
@@ -3562,7 +3569,7 @@ private RubyNode openModule(Nodes.Node moduleNode, RubyNode defineOrGetNode, Str
             newEnvironment.declareVar(name);
         }
 
-        final YARPTranslator moduleTranslator = new YARPTranslator(newEnvironment);
+        final YARPTranslator moduleTranslator = new YARPTranslator(newEnvironment, rubyWarnings);
         final ModuleBodyDefinition definition = moduleTranslator.compileClassNode(moduleNode, bodyNode);
         return new RunModuleDefinitionNode(definition, defineOrGetNode);
     }

src/main/java/org/truffleruby/parser/YARPTranslatorDriver.java

@@ -222,7 +222,7 @@ public RootCallTarget parse(RubySource rubySource, ParserContext parserContext,
         // Translate to Ruby Truffle nodes
 
         // use source encoding detected by manually, before source file is fully parsed
-        final YARPTranslator translator = new YARPTranslator(environment);
+        final YARPTranslator translator = new YARPTranslator(environment, rubyWarnings);
 
         RubyNode truffleNode;
         printParseTranslateExecuteMetric("before-translate", context, source);