Improve CoreMethod checks to ensure the number of parameters matches values in the annotation

Author: eregon

src/processor/java/org/truffleruby/processor/CoreModuleChecks.java

@@ -34,7 +34,7 @@ public class CoreModuleChecks {
         this.processor = processor;
     }
 
-    void checks(int[] lowerFixnum, CoreMethod coreMethod, TypeElement klass, boolean hasZeroArgument) {
+    void checks(int[] lowerFixnum, CoreMethod coreMethod, TypeElement klass, boolean hasSelfArgument) {
         byte[] lowerArgs = null;
         List<ExecutableElement> specializationMethods = new ArrayList<>();
 
@@ -54,13 +54,11 @@ void checks(int[] lowerFixnum, CoreMethod coreMethod, TypeElement klass, boolean
                 specializationMethods.add(specializationMethod);
 
                 lowerArgs = checkLowerFixnumArguments(specializationMethod, lowerArgs);
+
                 if (coreMethod != null) {
-                    checkAmbiguousOptionalArguments(
-                            coreMethod,
-                            specializationMethod,
-                            specializationAnnotation);
+                    checkCoreMethodArguments(coreMethod, specializationMethod, specializationAnnotation,
+                            hasSelfArgument);
                 }
-
             }
 
             klassIt = processor
@@ -81,8 +79,8 @@ void checks(int[] lowerFixnum, CoreMethod coreMethod, TypeElement klass, boolean
         // Verify against the lowerFixnum annotation
         for (int i = 0; i < lowerArgs.length; i++) {
             boolean shouldLower = lowerArgs[i] == 0b01; // int without long
-            if (shouldLower && !contains(lowerFixnum, hasZeroArgument ? i : i + 1)) {
-                processor.error("should use lowerFixnum for argument " + (hasZeroArgument ? i : i + 1), klass);
+            if (shouldLower && !contains(lowerFixnum, hasSelfArgument ? i : i + 1)) {
+                processor.error("should use lowerFixnum for argument " + (hasSelfArgument ? i : i + 1), klass);
             }
         }
     }
@@ -133,10 +131,11 @@ private static boolean contains(int[] array, int value) {
         return false;
     }
 
-    private void checkAmbiguousOptionalArguments(
+    private void checkCoreMethodArguments(
             CoreMethod coreMethod,
             ExecutableElement specializationMethod,
-            Specialization specializationAnnotation) {
+            Specialization specializationAnnotation,
+            boolean hasSelfArgument) {
         List<? extends VariableElement> parameters = specializationMethod.getParameters();
         int n = getLastParameterIndex(parameters);
 
@@ -145,21 +144,24 @@ private void checkAmbiguousOptionalArguments(
                 processor.error("last argument must be a RootCallTarget for alwaysInlined ", specializationMethod);
                 return;
             }
-            n--;
+            // All other arguments are packed as Object[]
+            return;
         }
 
-        if (coreMethod.needsBlock() && !coreMethod.alwaysInlined()) {
+        final int parametersCount = getParametersCount(parameters);
+        int expectedParametersCount = coreMethod.required() + coreMethod.optional();
+        if (hasSelfArgument) {
+            expectedParametersCount++;
+        }
+
+        if (coreMethod.needsBlock()) {
             if (n < 0) {
                 processor.error("invalid block method parameter position for", specializationMethod);
                 return;
             }
             checkParameterBlock(parameters.get(n));
             n--; // Ignore block argument.
-        }
-
-        if (coreMethod.alwaysInlined()) {
-            // All other arguments are packed as Object[]
-            return;
+            expectedParametersCount++;
         }
 
         if (coreMethod.rest()) {
@@ -173,6 +175,13 @@ private void checkAmbiguousOptionalArguments(
                 return;
             }
             n--; // ignore final Object[] argument
+            expectedParametersCount++;
+        }
+
+        if (parametersCount != expectedParametersCount) {
+            processor.error("expected " + expectedParametersCount + " parameters for this @CoreMethod but there are " +
+                    parametersCount, specializationMethod);
+            return;
         }
 
         for (int i = 0; i < coreMethod.optional(); i++, n--) {
@@ -221,6 +230,25 @@ private static int getLastParameterIndex(List<? extends VariableElement> paramet
         return n;
     }
 
+    private int getParametersCount(List<? extends VariableElement> parameters) {
+        int last = getLastParameterIndex(parameters);
+        int count = last + 1;
+
+        if (count > 0) {
+            var type = parameters.get(0).asType();
+            if (processor.isSameType(type, processor.nodeType)) {
+                if (processor.isSameType(type, processor.virtualFrameType)) {
+                    return count - 2;
+                } else {
+                    return count - 1;
+                }
+            } else if (processor.isSameType(type, processor.virtualFrameType)) {
+                return count - 1;
+            }
+        }
+        return count;
+    }
+
     private void checkParameterUnguarded(Specialization specializationAnnotation, VariableElement parameter) {
         String name = parameter.getSimpleName().toString();
 

src/processor/java/org/truffleruby/processor/CoreModuleProcessor.java

@@ -93,6 +93,7 @@ public class CoreModuleProcessor extends TruffleRubyProcessor {
     TypeMirror rubyProcType;
     TypeMirror rootCallTargetType;
     // node types
+    TypeMirror nodeType;
     TypeMirror rubyNodeType;
     TypeMirror rubyBaseNodeType;
     TypeMirror primitiveNodeType;
@@ -108,6 +109,7 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment
         notProvidedType = elementUtils.getTypeElement("org.truffleruby.language.NotProvided").asType();
         rubyProcType = elementUtils.getTypeElement("org.truffleruby.core.proc.RubyProc").asType();
         rootCallTargetType = elementUtils.getTypeElement("com.oracle.truffle.api.RootCallTarget").asType();
+        nodeType = elementUtils.getTypeElement("com.oracle.truffle.api.nodes.Node").asType();
         rubyNodeType = elementUtils.getTypeElement("org.truffleruby.language.RubyNode").asType();
         rubyBaseNodeType = elementUtils.getTypeElement("org.truffleruby.language.RubyBaseNode").asType();
         primitiveNodeType = elementUtils.getTypeElement("org.truffleruby.builtins.PrimitiveNode").asType();
@@ -188,23 +190,19 @@ private void processCoreModule(TypeElement coreModuleElement) throws IOException
                         if (coreMethod != null) {
                             // Do not use needsSelf=true in module functions, it is either the module/class or the instance.
                             // Usage of needsSelf is quite rare for singleton methods (except constructors).
-                            boolean needsSelf = coreMethod.constructor() ||
+                            boolean hasSelfArgument = coreMethod.constructor() ||
                                     (!coreMethod.isModuleFunction() && !coreMethod.onSingleton() &&
                                             coreMethod.needsSelf());
 
-                            CoreMethod checkAmbiguous = !coreMethod.alwaysInlined() &&
-                                    (coreMethod.optional() > 0 || coreMethod.needsBlock())
-                                            ? coreMethod
-                                            : null;
-                            coreModuleChecks.checks(coreMethod.lowerFixnum(), checkAmbiguous, klass, needsSelf);
+                            coreModuleChecks.checks(coreMethod.lowerFixnum(), coreMethod, klass, hasSelfArgument);
                             if (!inherits(e.asType(), coreMethodNodeType) &&
                                     !inherits(e.asType(), alwaysInlinedMethodNodeType)) {
                                 error(e +
                                         " should inherit from CoreMethodArrayArgumentsNode, CoreMethodNode, AlwaysInlinedMethodNode",
                                         e);
                             }
                             processCoreMethod(stream, rubyStream, coreModuleElement, coreModule, klass, coreMethod,
-                                    needsSelf);
+                                    hasSelfArgument);
                         }
                     }
                 }
@@ -287,7 +285,7 @@ private void processCoreMethod(
             CoreModule coreModule,
             TypeElement klass,
             CoreMethod coreMethod,
-            boolean needsSelf) {
+            boolean hasSelfArgument) {
         final StringJoiner names = new StringJoiner(", ");
         for (String name : coreMethod.names()) {
             names.add(quote(name));
@@ -312,7 +310,7 @@ private void processCoreMethod(
 
         int numberOfArguments = getNumberOfArguments(coreMethod);
         String[] argumentNamesFromAnnotation = coreMethod.argumentNames();
-        final List<String> argumentNames = getArgumentNames(klass, argumentNamesFromAnnotation, needsSelf,
+        final List<String> argumentNames = getArgumentNames(klass, argumentNamesFromAnnotation, hasSelfArgument,
                 coreMethod.alwaysInlined(),
                 numberOfArguments);