[GR-18163] Detect invalid returns in module bodies.

eregon · eregon · commit 57ff630075e5 · 2020-11-24T22:59:57.000Z
PullRequest: truffleruby/2210
diff --git a/spec/tags/language/block_tags.txt b/spec/tags/language/block_tags.txt
@@ -1,4 +1,3 @@
-fails:A block yielded a single Array when non-symbol keys are in a keyword arguments Hash raises an ArgumentError
 fails:A block yielded a single Array calls #to_hash on the argument but ignores result when optional argument and keyword argument accepted
 fails:Post-args with optional args with a circular argument reference raises a SyntaxError if using an existing local with the same name as the argument
 fails:Post-args with optional args with a circular argument reference raises a SyntaxError if there is an existing method with the same name as the argument
diff --git a/spec/tags/language/constants_tags.txt b/spec/tags/language/constants_tags.txt
@@ -1,4 +1,3 @@
-fails:top-level constant lookup on a class searches Object successfully after searching other scopes
 fails:Module#private_constant marked constants NameError by #private_constant has the defined class as the :name attribute
 fails:top-level constant lookup on a class does not search Object after searching other scopes
 fails:Module#private_constant marked constants sends #const_missing to the original class or module
diff --git a/spec/tags/language/delegation_tags.txt b/spec/tags/language/delegation_tags.txt
diff --git a/spec/tags/language/for_tags.txt b/spec/tags/language/for_tags.txt
diff --git a/spec/tags/language/method_tags.txt b/spec/tags/language/method_tags.txt
@@ -1,12 +1,6 @@
 fails:A method assigns local variables from method parameters for definition 'def m() end'
 fails:A method assigns local variables from method parameters for definition 'def m(*a) a end'
-fails:A method assigns local variables from method parameters for definition 'def m(a=1, b:) [a, b] end'
-fails:A method assigns local variables from method parameters for definition 'def m(a=1, b: 2) [a, b] end'
 fails:A method assigns local variables from method parameters for definition 'def m(a=1, **) a end'
-fails:A method assigns local variables from method parameters for definition 'def m(*, a:) a end'
-fails:A method assigns local variables from method parameters for definition 'def m(*a, b:) [a, b] end'
-fails:A method assigns local variables from method parameters for definition 'def m(*, a: 1) a end'
-fails:A method assigns local variables from method parameters for definition 'def m(*a, b: 1) [a, b] end'
 fails:A method assigns local variables from method parameters for definition 'def m(*a, **) a end'
 fails:A method assigns local variables from method parameters for definition 'def m(*, **k) k end'
 fails:A method assigns local variables from method parameters for definition 'def m(a = nil, **k) [a, k] end'
diff --git a/spec/tags/language/pattern_matching_tags.txt b/spec/tags/language/pattern_matching_tags.txt
@@ -1,7 +1,5 @@
-fails:Pattern matching can be standalone in operator that deconstructs value
 fails:Pattern matching extends case expression with case/in construction
 fails:Pattern matching allows using then operator
-fails:Pattern matching warns about pattern matching is experimental feature
 fails:Pattern matching binds variables
 fails:Pattern matching cannot mix in and when operators
 fails:Pattern matching checks patterns until the first matching
@@ -41,7 +39,6 @@ fails:Pattern matching Array pattern does not match object without #deconstruct
 fails:Pattern matching Array pattern raises TypeError if #deconstruct method does not return array
 fails:Pattern matching Array pattern does not match object if elements of array returned by #deconstruct method does not match elements in pattern
 fails:Pattern matching Array pattern binds variables
-fails:Pattern matching Array pattern binds variable even if patter matches only partially
 fails:Pattern matching Array pattern supports splat operator *rest
 fails:Pattern matching Array pattern does not match partially by default
 fails:Pattern matching Array pattern does match partially from the array beginning if list + , syntax used
@@ -66,7 +63,6 @@ fails:Pattern matching Hash pattern passes keys specified in pattern as argument
 fails:Pattern matching Hash pattern passes keys specified in pattern to #deconstruct_keys method if pattern contains double splat operator **
 fails:Pattern matching Hash pattern passes nil to #deconstruct_keys method if pattern contains double splat operator **rest
 fails:Pattern matching Hash pattern binds variables
-fails:Pattern matching Hash pattern binds variable even if pattern matches only partially
 fails:Pattern matching Hash pattern supports double splat operator **rest
 fails:Pattern matching Hash pattern treats **nil like there should not be any other keys in a matched Hash
 fails:Pattern matching Hash pattern can match partially
diff --git a/spec/tags/language/rescue_tags.txt b/spec/tags/language/rescue_tags.txt
@@ -1,3 +1,2 @@
-fails:The rescue keyword allows 'rescue' in method arguments
 fails:The rescue keyword raises SyntaxError when else is used without rescue and ensure
 fails:The rescue keyword inline form rescues with multiple assignment
diff --git a/spec/tags/language/return_tags.txt b/spec/tags/language/return_tags.txt
@@ -1,13 +1,5 @@
 fails:The return keyword in a Thread raises a LocalJumpError if used to exit a thread
 slow:The return keyword at top level stops file execution
 slow:The return keyword at top level within a begin fires ensure block before returning
-slow:The return keyword at top level return with argument does not affect exit status
-fails:The return keyword at top level within a class is allowed
-fails:The return keyword at top level file loading stops file loading and execution
-fails:The return keyword at top level file requiring stops file loading and execution
-fails:The return keyword at top level return with argument does not affect exit status
-fails:The return keyword at top level within a block within a class is allowed
-fails:The return keyword at top level within a class raises a SyntaxError
-fails(escapes the interpreter):The return keyword at top level within a block within a class is not allowed
 slow:The return keyword at top level return with argument warns but does not affect exit status
 fails:The return keyword at top level return with argument warns but does not affect exit status
diff --git a/spec/tags/language/string_tags.txt b/spec/tags/language/string_tags.txt
@@ -3,4 +3,3 @@ slow:Ruby String literals with a magic frozen comment produce the same object fo
 slow:Ruby String literals with a magic frozen comment produce the same object for literals with the same content in different files
 slow:Ruby String literals with a magic frozen comment produce different objects for literals with the same content in different files if the other file doesn't have the comment
 slow:Ruby String literals with a magic frozen comment produce different objects for literals with the same content in different files if they have different encodings
-fails:Ruby character strings don't get confused by partial interpolation character sequences
diff --git a/src/main/java/org/truffleruby/language/control/InvalidReturnNode.java b/src/main/java/org/truffleruby/language/control/InvalidReturnNode.java
@@ -0,0 +1,31 @@
+/*
+ * Copyright (c) 2020 Oracle and/or its affiliates. All rights reserved. This
+ * code is released under a tri EPL/GPL/LGPL license. You can use it,
+ * redistribute it and/or modify it under the terms of the:
+ *
+ * Eclipse Public License version 2.0, or
+ * GNU General Public License version 2, or
+ * GNU Lesser General Public License version 2.1.
+ */
+package org.truffleruby.language.control;
+
+import org.truffleruby.language.RubyContextSourceNode;
+import org.truffleruby.language.RubyNode;
+
+import com.oracle.truffle.api.frame.VirtualFrame;
+
+public class InvalidReturnNode extends RubyContextSourceNode {
+
+    @Child public RubyNode value;
+
+    public InvalidReturnNode(RubyNode value) {
+        this.value = value;
+    }
+
+    @Override
+    public Object execute(VirtualFrame frame) {
+        value.doExecuteVoid(frame);
+        throw new RaiseException(getContext(), coreExceptions().unexpectedReturn(this));
+    }
+
+}
diff --git a/src/main/java/org/truffleruby/language/control/ReturnID.java b/src/main/java/org/truffleruby/language/control/ReturnID.java
@@ -9,6 +9,10 @@
  */
 package org.truffleruby.language.control;
 
-public class ReturnID {
+public final class ReturnID {
+
+    /** Returning to or through class/module bodies is a LocalJumpError. However, the surrounding block might be turned
+     * into a lambda, in which case it becomes valid and returns from the lambda. */
+    public static final ReturnID MODULE_BODY = new ReturnID();
 
 }
diff --git a/src/main/java/org/truffleruby/parser/BodyTranslator.java b/src/main/java/org/truffleruby/parser/BodyTranslator.java
@@ -85,6 +85,7 @@
 import org.truffleruby.language.control.FrameOnStackNode;
 import org.truffleruby.language.control.IfElseNode;
 import org.truffleruby.language.control.IfNode;
+import org.truffleruby.language.control.InvalidReturnNode;
 import org.truffleruby.language.control.LocalReturnNode;
 import org.truffleruby.language.control.NextNode;
 import org.truffleruby.language.control.NotNode;
@@ -846,18 +847,10 @@ private RubyNode openModule(SourceIndexLength sourceSection, RubyNode defineOrGe
                     null,
                     null);
 
-            final ReturnID returnId;
-
-            if (type == OpenModule.SINGLETON_CLASS) {
-                returnId = environment.getReturnID();
-            } else {
-                returnId = environment.getParseEnvironment().allocateReturnID();
-            }
-
             final TranslatorEnvironment newEnvironment = new TranslatorEnvironment(
                     environment,
                     environment.getParseEnvironment(),
-                    returnId,
+                    ReturnID.MODULE_BODY,
                     true,
                     true,
                     true,
@@ -2872,7 +2865,12 @@ public RubyNode visitReturnNode(ReturnParseNode node) {
         final RubyNode ret;
 
         if (environment.isBlock()) {
-            ret = new DynamicReturnNode(environment.getReturnID(), translatedChild);
+            final ReturnID returnID = environment.getReturnID();
+            if (returnID == ReturnID.MODULE_BODY) {
+                ret = new InvalidReturnNode(translatedChild);
+            } else {
+                ret = new DynamicReturnNode(returnID, translatedChild);
+            }
         } else {
             ret = new LocalReturnNode(translatedChild);
         }
diff --git a/src/main/java/org/truffleruby/parser/MethodTranslator.java b/src/main/java/org/truffleruby/parser/MethodTranslator.java
@@ -24,7 +24,9 @@
 import org.truffleruby.language.arguments.ReadPreArgumentNode;
 import org.truffleruby.language.arguments.ShouldDestructureNode;
 import org.truffleruby.language.control.AndNode;
+import org.truffleruby.language.control.DynamicReturnNode;
 import org.truffleruby.language.control.IfElseNode;
+import org.truffleruby.language.control.InvalidReturnNode;
 import org.truffleruby.language.control.NotNode;
 import org.truffleruby.language.control.SequenceNode;
 import org.truffleruby.language.locals.FlipFlopStateNode;
@@ -172,9 +174,9 @@ public BlockDefinitionNode compileBlockNode(SourceIndexLength sourceSection, Par
         body = new ExceptionTranslatingNode(body, UnsupportedOperationBehavior.TYPE_ERROR);
 
         // Procs
-        final RubyNode bodyProc = new CatchForProcNode(
-                composeBody(sourceSection, preludeProc, NodeUtil.cloneNode(body)));
-        bodyProc.unsafeSetSourceSection(enclosing(sourceSection, body));
+        final RubyNode bodyCopyForProc = NodeUtil.cloneNode(body);
+        final RubyNode bodyProc = new CatchForProcNode(composeBody(sourceSection, preludeProc, bodyCopyForProc));
+        bodyProc.unsafeSetSourceSection(enclosing(sourceSection, bodyCopyForProc));
 
         final RubyRootNode newRootNodeForProcs = new RubyRootNode(
                 language,
@@ -185,7 +187,8 @@ public BlockDefinitionNode compileBlockNode(SourceIndexLength sourceSection, Par
                 Split.HEURISTIC);
 
         // Lambdas
-        RubyNode composed = composeBody(sourceSection, preludeLambda, body /* no copy, last usage */);
+        RubyNode bodyForLambda = body; // no copy, last usage
+        RubyNode composed = composeBody(sourceSection, preludeLambda, bodyForLambda);
 
         composed = new CatchForLambdaNode(environment.getReturnID(), environment.getBreakID(), composed);
 
@@ -201,6 +204,14 @@ public BlockDefinitionNode compileBlockNode(SourceIndexLength sourceSection, Par
         final RootCallTarget callTargetAsLambda = Truffle.getRuntime().createCallTarget(newRootNodeForLambdas);
         final RootCallTarget callTargetAsProc = Truffle.getRuntime().createCallTarget(newRootNodeForProcs);
 
+        if (isProc) {
+            // If we end up executing this block as a lambda, but don't know it statically, e.g., `lambda {}` or
+            // `define_method(:foo, proc {})`), then returns are always valid and return from that lambda.
+            // This needs to run after nodes are adopted for replace() to work and nodes to know their parent.
+            for (InvalidReturnNode returnNode : NodeUtil.findAllNodeInstances(bodyForLambda, InvalidReturnNode.class)) {
+                returnNode.replace(new DynamicReturnNode(environment.getReturnID(), returnNode.value));
+            }
+        }
 
         Object frameOnStackMarkerSlot;
 

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,3 @@`
`1`		`-fails:top-level constant lookup on a class searches Object successfully after searching other scopes`
`2`	`1`	`fails:Module#private_constant marked constants NameError by #private_constant has the defined class as the :name attribute`
`3`	`2`	`fails:top-level constant lookup on a class does not search Object after searching other scopes`
`4`	`3`	`fails:Module#private_constant marked constants sends #const_missing to the original class or module`
Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,2 @@`
`1`		`-fails:The rescue keyword allows 'rescue' in method arguments`
`2`	`1`	`fails:The rescue keyword raises SyntaxError when else is used without rescue and ensure`
`3`	`2`	`fails:The rescue keyword inline form rescues with multiple assignment`