Clean up, reword comments and fix typos

andrykonchin · andrykonchin · commit 55caff0be57d · 2023-11-17T15:05:38.000+02:00
diff --git a/spec/tags/language/numbered_parameters_tags.txt b/spec/tags/language/numbered_parameters_tags.txt
@@ -1 +1 @@
-fails(GR-30031):Numbered parameters warns when numbered parameter is overwritten with local variable
+fails(GR-30031):Numbered parameters warns when numbered parameter is overwritten with local variable
diff --git a/src/main/java/org/truffleruby/core/inlined/CoreMethodAssumptions.java b/src/main/java/org/truffleruby/core/inlined/CoreMethodAssumptions.java
@@ -158,7 +158,7 @@ public RubyContextSourceNode createCallNode(RubyCallNodeParameters callParameter
         final RubyNode[] args = callParameters.getArguments();
         int n = 1 /* self */ + args.length;
 
-        if (callParameters.getBlock() != null) { // TODO: could we also handle a `proc` call?
+        if (callParameters.getBlock() != null) {
             if (callParameters.getMethodName().equals("lambda") &&
                     (callParameters.getBlock() instanceof BlockDefinitionNode)) {
 
diff --git a/src/main/java/org/truffleruby/debug/TruffleASTPrinter.java b/src/main/java/org/truffleruby/debug/TruffleASTPrinter.java
@@ -10,12 +10,12 @@
 package org.truffleruby.debug;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.TreeMap;
 import java.util.TreeSet;
 import java.util.regex.Matcher;
 
@@ -152,7 +152,6 @@ private static List<Pair<String, Object>> getNodeAttributes(Node node) {
 
     // map frame slots to local variable names
     private static Map<String, String> getNodeAttributeAnnotations(Node node) {
-        final TreeMap<String, String> annotations = new TreeMap<>();
         final int frameSlot;
 
         // ignore WriteDeclarationVariableNode and ReadDeclarationVariableNode
@@ -167,25 +166,15 @@ private static Map<String, String> getNodeAttributeAnnotations(Node node) {
         } else if (node instanceof ReadFrameSlotNode readFrameSlotNode) {
             frameSlot = readFrameSlotNode.getFrameSlot();
         } else {
-            return annotations;
+            return Collections.emptyMap();
         }
 
-        Node parent = node.getParent();
-
-        while (parent != null && !(parent instanceof RootNode)) {
-            parent = parent.getParent();
-        }
-
-        assert parent != null;
-
-        final var rootNode = (RootNode) parent;
+        final var rootNode = node.getRootNode();
         final var frameDescriptor = rootNode.getFrameDescriptor();
         final String variableName = frameDescriptor.getSlotName(frameSlot).toString();
 
         // all the mentioned above classes use the same field name - "frameSlot", so just hardcode it
-        annotations.put("frameSlot", variableName);
-
-        return annotations;
+        return Collections.singletonMap("frameSlot", variableName);
     }
 
     private static List<Pair<String, Object>> getNodeChildren(Node node) {
diff --git a/src/main/java/org/truffleruby/language/arguments/ReadUserKeywordsHashNode.java b/src/main/java/org/truffleruby/language/arguments/ReadUserKeywordsHashNode.java
@@ -26,7 +26,7 @@ public RubyHash execute(VirtualFrame frame) {
         if (keywordArgumentsProfile.profile(descriptor instanceof KeywordArgumentsDescriptor)) {
             final RubyHash keywords = (RubyHash) RubyArguments.getLastArgument(frame);
             assert !keywords.empty();
-            assert assertHashMatchesDescriptor(keywords, (KeywordArgumentsDescriptor) descriptor); // TODO: seems incorrect as far as descriptor contains keyword names from the arguments, not declared in a called method
+            assert assertHashMatchesDescriptor(keywords, (KeywordArgumentsDescriptor) descriptor);
             return keywords;
         } else {
             return null;
diff --git a/src/main/java/org/truffleruby/parser/MethodTranslator.java b/src/main/java/org/truffleruby/parser/MethodTranslator.java
@@ -371,7 +371,7 @@ private static RubyNode composeBody(TranslatorEnvironment environment, SourceInd
 
         body = sequence(sourceSection, Arrays.asList(prelude, body));
 
-        if (environment.getFlipFlopStates().size() > 0) { // TODO: it could be called twice in case of conversion block->lambda or vise versa
+        if (environment.getFlipFlopStates().size() > 0) {
             body = sequence(sourceSection, Arrays.asList(initFlipFlopStates(environment, sourceSection), body));
         }
 
diff --git a/src/main/java/org/truffleruby/parser/TranslatorEnvironment.java b/src/main/java/org/truffleruby/parser/TranslatorEnvironment.java
@@ -36,10 +36,30 @@
 import com.oracle.truffle.api.frame.FrameDescriptor;
 import org.truffleruby.language.objects.SelfNode;
 import org.truffleruby.language.threadlocal.SpecialVariableStorage;
+import org.truffleruby.parser.parser.ParserSupport;
 
 public final class TranslatorEnvironment {
 
+    /* Names of hidden local variables.
+     * 
+     * For each parameter in methods and blocks a local variable is declared to keep actual argument value. Use the
+     * following names for parameters that don't have explicit names - anonymous rest, keyword rest and block.
+     * 
+     * Store values of anonymous parameters to forward them either implicitly to a super method call or explicitly to a
+     * method call with *, **, & or "...". */
+
+    /** local variable to access a block argument */
     public static final String METHOD_BLOCK_NAME = Layouts.TEMP_PREFIX + "method_block_arg";
+    /** local variable name for * parameter */
+    static final String DEFAULT_REST_NAME = ParserSupport.REST_VAR;
+    /** local variable name for ** parameter */
+    static final String DEFAULT_KEYWORD_REST_NAME = ParserSupport.KWREST_VAR;
+    /** local variable name for * parameter caused by desugaring ... parameter (forward-everything) */
+    static final String FORWARDED_REST_NAME = ParserSupport.FORWARD_ARGS_REST_VAR;
+    /** local variable name for ** parameter caused by desugaring ... parameter (forward-everything) */
+    static final String FORWARDED_KEYWORD_REST_NAME = ParserSupport.FORWARD_ARGS_KWREST_VAR;
+    /** local variable name for & parameter caused by desugaring ... parameter (forward-everything) */
+    static final String FORWARDED_BLOCK_NAME = ParserSupport.FORWARD_ARGS_BLOCK_VAR;
 
     private final ParseEnvironment parseEnvironment;
 
diff --git a/src/main/java/org/truffleruby/parser/YARPBlockNodeTranslator.java b/src/main/java/org/truffleruby/parser/YARPBlockNodeTranslator.java
@@ -40,9 +40,7 @@
 import org.truffleruby.language.methods.Arity;
 import org.truffleruby.language.methods.BlockDefinitionNodeGen;
 
-import java.util.ArrayDeque;
 import java.util.Arrays;
-import java.util.Deque;
 import java.util.function.Supplier;
 
 public final class YARPBlockNodeTranslator extends YARPTranslator {
@@ -139,7 +137,7 @@ public RubyNode compileBlockNode(Nodes.Node body, String[] locals, boolean isSta
         } else {
             // 100% proc
 
-            // it could be converted lately to lambda, e.g. if passed as argument to Module#define_method
+            // it could be converted later to lambda, e.g. if passed as argument to Module#define_method
             callTargets = new ProcCallTargets(procCompiler.get(), null, lambdaCompiler);
         }
 
@@ -225,8 +223,6 @@ private static Supplier<RootCallTarget> procCompiler(
                     ? body.cloneUninitialized() // previously compiled as lambda, must copy
                     : body;
 
-            // TODO: shouldn't we clone preludeProc as well?
-            //      It contains loadArguments node that could be used for lambda
             final RubyNode bodyProc = composeBody(environment, preludeProc, bodyForProc);
 
             final RubyProcRootNode newRootNodeForProcs = new RubyProcRootNode(
@@ -249,8 +245,8 @@ private static Supplier<RootCallTarget> procCompiler(
                 // Note that the compilation to lambda does not alter the original returnID (instead it's "hijacked"
                 // and used in RubyLambdaRootNode).
                 //
-                // This needs to run after nodes are adopted for replace() (during callTarget initialisation) to work
-                // and nodes to know their parent.
+                // replace() should be called **after** nodes are adopted (in RootNode#getCallTarget() using adoptChildren())
+                // so nodes know their parent.
                 for (DynamicReturnNode returnNode : NodeUtil
                         .findAllNodeInstances(bodyForProc, DynamicReturnNode.class)) {
                     if (returnNode.returnID == ReturnID.MODULE_BODY) {
diff --git a/src/main/java/org/truffleruby/parser/YARPDefNodeTranslator.java b/src/main/java/org/truffleruby/parser/YARPDefNodeTranslator.java
@@ -22,16 +22,8 @@
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.api.source.Source;
 import org.prism.Nodes;
-import org.truffleruby.parser.parser.ParserSupport;
 
 public final class YARPDefNodeTranslator extends YARPTranslator {
-    // TODO: not the best place for these constants. Is YARPLoadArgumentsTranslator better? Or YARPTranslator?
-    static final String DEFAULT_KEYWORD_REST_NAME = ParserSupport.KWREST_VAR; // local variable name for ** parameter
-    static final String DEFAULT_REST_NAME = ParserSupport.REST_VAR; // local variable name for * parameter
-    static final String FORWARDED_REST_NAME = ParserSupport.FORWARD_ARGS_REST_VAR; // local variable name for * parameter caused by desugaring ...
-    static final String FORWARDED_KEYWORD_REST_NAME = ParserSupport.FORWARD_ARGS_KWREST_VAR; // local variable name for ** parameter caused by desugaring ...
-    static final String FORWARDED_BLOCK_NAME = ParserSupport.FORWARD_ARGS_BLOCK_VAR; // local variable name for & parameter caused by desugaring ...
-
     private final boolean shouldLazyTranslate;
 
     public YARPDefNodeTranslator(
@@ -105,15 +97,15 @@ private void declareLocalVariables(Nodes.DefNode node) {
 
         for (String name : node.locals) {
             switch (name) {
-                case "*" -> environment.declareVar(DEFAULT_REST_NAME);
-                case "**" -> environment.declareVar(DEFAULT_KEYWORD_REST_NAME);
+                case "*" -> environment.declareVar(TranslatorEnvironment.DEFAULT_REST_NAME);
+                case "**" -> environment.declareVar(TranslatorEnvironment.DEFAULT_KEYWORD_REST_NAME);
                 case "&" ->
                     // we don't support yet Ruby 3.1's anonymous block parameter
                     throw CompilerDirectives.shouldNotReachHere("Anonymous block parameters aren't supported yet");
                 case "..." -> {
-                    environment.declareVar(YARPDefNodeTranslator.FORWARDED_REST_NAME);
-                    environment.declareVar(YARPDefNodeTranslator.FORWARDED_KEYWORD_REST_NAME);
-                    environment.declareVar(YARPDefNodeTranslator.FORWARDED_BLOCK_NAME);
+                    environment.declareVar(TranslatorEnvironment.FORWARDED_REST_NAME);
+                    environment.declareVar(TranslatorEnvironment.FORWARDED_KEYWORD_REST_NAME);
+                    environment.declareVar(TranslatorEnvironment.FORWARDED_BLOCK_NAME);
                 }
                 default -> environment.declareVar(name);
             }
diff --git a/src/main/java/org/truffleruby/parser/YARPLoadArgumentsTranslator.java b/src/main/java/org/truffleruby/parser/YARPLoadArgumentsTranslator.java
@@ -239,11 +239,11 @@ public RubyNode visitRestParameterNode(Nodes.RestParameterNode node) {
         int to = -parametersNode.posts.length;
         readNode = new ReadRestArgumentNode(from, -to, hasKeywordArguments());
 
-        final String name = (node.name != null) ? node.name : YARPDefNodeTranslator.DEFAULT_REST_NAME;
+        final String name = (node.name != null) ? node.name : TranslatorEnvironment.DEFAULT_REST_NAME;
 
-        // When a rest parameter in a block is nameless then YARP doesn't add '*' to block's locals,
-        // and we don't declare this hidden variable beforehand.
-        // So declare it here right before usage.
+        // When a rest parameter in a block is nameless then YARP doesn't add '*' to block's locals
+        // (what is expected as far as arguments forwarding doesn't work in blocks), and we don't
+        // declare this hidden variable beforehand. So declare it here right before usage.
         final int slot = environment.declareVar(name);
 
         return new WriteLocalVariableNode(slot, readNode);
@@ -252,11 +252,11 @@ public RubyNode visitRestParameterNode(Nodes.RestParameterNode node) {
     @Override
     public RubyNode visitKeywordRestParameterNode(Nodes.KeywordRestParameterNode node) {
         final RubyNode readNode = new ReadKeywordRestArgumentNode(language, arity);
-        final String name = (node.name != null) ? node.name : YARPDefNodeTranslator.DEFAULT_KEYWORD_REST_NAME;
+        final String name = (node.name != null) ? node.name : TranslatorEnvironment.DEFAULT_KEYWORD_REST_NAME;
 
-        // When a keyword rest parameter in a block is nameless then YARP doesn't add '**' to block's locals,
-        // and we don't declare this hidden variable beforehand.
-        // So declare it here right before usage.
+        // When a keyword rest parameter in a block is nameless then YARP doesn't add '**' to block's locals
+        // (what is expected as far as arguments forwarding doesn't work in blocks), and we don't declare this
+        // hidden variable beforehand. So declare it here right before usage.
         final int slot = environment.declareVar(name);
 
         return new WriteLocalVariableNode(slot, readNode);
@@ -281,9 +281,9 @@ public RubyNode visitForwardingParameterNode(Nodes.ForwardingParameterNode node)
         ArrayList<RubyNode> sequence = new ArrayList<>();
 
         // desugar ... to *, **, and & parameters
-        final var rest = new Nodes.RestParameterNode(YARPDefNodeTranslator.FORWARDED_REST_NAME, 0, 0);
-        final var keyrest = new Nodes.KeywordRestParameterNode(YARPDefNodeTranslator.FORWARDED_KEYWORD_REST_NAME, 0, 0);
-        final var block = new Nodes.BlockParameterNode(YARPDefNodeTranslator.FORWARDED_BLOCK_NAME, 0, 0);
+        final var rest = new Nodes.RestParameterNode(TranslatorEnvironment.FORWARDED_REST_NAME, 0, 0);
+        final var keyrest = new Nodes.KeywordRestParameterNode(TranslatorEnvironment.FORWARDED_KEYWORD_REST_NAME, 0, 0);
+        final var block = new Nodes.BlockParameterNode(TranslatorEnvironment.FORWARDED_BLOCK_NAME, 0, 0);
 
         sequence.add(rest.accept(this));
         sequence.add(keyrest.accept(this));
diff --git a/src/main/java/org/truffleruby/parser/YARPMultiTargetNodeTranslator.java b/src/main/java/org/truffleruby/parser/YARPMultiTargetNodeTranslator.java
@@ -144,7 +144,7 @@ public AssignableNode visitSplatNode(Nodes.SplatNode node) {
     @Override
     public AssignableNode visitRequiredParameterNode(Nodes.RequiredParameterNode node) {
         // TODO: this could be done more directly but the logic of visitLocalVariableWriteNode() needs to be simpler first
-        // TODO: depth is not suppose to be used anyway so pass 0 value.
+        // NOTE: depth is not supposed to be used anyway so pass 0 value.
         final RubyNode rubyNode = yarpTranslator.visitLocalVariableWriteNode(
                 new Nodes.LocalVariableWriteNode(node.name, 0, null, node.startOffset, node.length));
         return ((AssignableNode) rubyNode).toAssignableNode();
diff --git a/src/main/java/org/truffleruby/parser/YARPParametersNodeToDestructureTranslator.java b/src/main/java/org/truffleruby/parser/YARPParametersNodeToDestructureTranslator.java
@@ -175,26 +175,26 @@ public RubyNode visitOptionalParameterNode(Nodes.OptionalParameterNode node) {
 
     @Override
     public RubyNode visitRestParameterNode(Nodes.RestParameterNode node) {
-        // TODO: we actually could do nothing if parameter is anonymous
+        // NOTE: we actually could do nothing if parameter is anonymous
         final RubyNode readNode;
 
         int from = parameters.requireds.length + parameters.optionals.length;
         int to = -parameters.posts.length;
         readNode = ArraySliceNodeGen.create(from, to, readArrayNode);
 
-        final String name = (node.name != null) ? node.name : YARPDefNodeTranslator.DEFAULT_REST_NAME;
+        final String name = (node.name != null) ? node.name : TranslatorEnvironment.DEFAULT_REST_NAME;
         final int slot = environment.findFrameSlot(name);
         return new WriteLocalVariableNode(slot, readNode);
     }
 
     @Override
     public RubyNode visitKeywordRestParameterNode(Nodes.KeywordRestParameterNode node) {
-        // TODO: we actually could do nothing if parameter is anonymous
+        // NOTE: we actually could do nothing if parameter is anonymous
         final RubyNode readNode = new ReadKeywordRestArgumentNode(language, arity);
-        final String name = (node.name != null) ? node.name : YARPDefNodeTranslator.DEFAULT_KEYWORD_REST_NAME;
+        final String name = (node.name != null) ? node.name : TranslatorEnvironment.DEFAULT_KEYWORD_REST_NAME;
         final int slot = environment.declareVar(name);
 
-        // TODO: actually we can immediately assign `{}` value
+        // NOTE: actually we can immediately assign `{}` value
         return new WriteLocalVariableNode(slot, readNode);
     }
 
diff --git a/src/main/java/org/truffleruby/parser/YARPTranslator.java b/src/main/java/org/truffleruby/parser/YARPTranslator.java

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-fails(GR-30031):Numbered parameters warns when numbered parameter is overwritten with local variable`
	`1`	`+fails(GR-30031):Numbered parameters warns when numbered parameter is overwritten with local variable`
Original file line number	Diff line number	Diff line change
`@@ -371,7 +371,7 @@ private static RubyNode composeBody(TranslatorEnvironment environment, SourceInd`
`371`	`371`
`372`	`372`	`body = sequence(sourceSection, Arrays.asList(prelude, body));`
`373`	`373`
`374`		`- if (environment.getFlipFlopStates().size() > 0) { // TODO: it could be called twice in case of conversion block->lambda or vise versa`
	`374`	`+ if (environment.getFlipFlopStates().size() > 0) {`
`375`	`375`	`body = sequence(sourceSection, Arrays.asList(initFlipFlopStates(environment, sourceSection), body));`
`376`	`376`	`}`
`377`	`377`