Warn user when forgetting def on a variable (jenkinsci#994)

* Exploring a warning when a field is set

* Limiting matches to non-null assignments to `CpsScript`s, and suppressing from some tests which could equally well have used lexical variables

* Seem to be getting warnings only in appropriate places

* Might want a redirect

* Better message

* Further tweaking message

* Forgot to reuse computed `owner`

* Kill switch jenkinsci#994 (comment)

* Ignore `@Field`s jenkinsci#994 (comment)

Author: jglick

plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/LoggingInvoker.java

@@ -30,6 +30,7 @@
 import com.cloudbees.groovy.cps.sandbox.SandboxInvoker;
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
+import groovy.lang.Closure;
 import groovy.lang.GroovyClassLoader;
 import hudson.ExtensionList;
 import java.util.Set;
@@ -137,6 +138,29 @@ private void maybeRecord(Class<?> clazz, Supplier<String> message) {
         Class<?> clazz = classOf(lhs);
         maybeRecord(clazz, () -> clazz.getName() + "." + name);
         delegate.setProperty(lhs, name, value);
+        if (SystemProperties.getBoolean(LoggingInvoker.class.getName() + ".fieldSetWarning", true)) {
+            if (value != null) {
+                var owner = findOwner(lhs);
+                if (owner instanceof CpsScript) {
+                    try {
+                        owner.getClass().getDeclaredField(name);
+                        // OK, @Field, ignore
+                    } catch (NoSuchFieldException x) {
+                        var g = CpsThreadGroup.current();
+                        if (g != null) {
+                            var e = g.getExecution();
+                            if (e != null) {
+                                e.getOwner().getListener().getLogger().println(Messages.LoggingInvoker_field_set(owner.getClass().getSimpleName(), name, value.getClass().getSimpleName()));
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+    private Object findOwner(Object o) {
+        return o instanceof Closure<?> c ? findOwner(c.getOwner()) : o;
     }
 
     @Override public Object getAttribute(Object lhs, String name) throws Throwable {

plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Messages.properties

@@ -6,3 +6,8 @@ SnippetizerLink.OnlineDocsLink.displayName=Online Documentation
 SnippetizerLink.ExamplesLink.displayName=Examples Reference
 SnippetizerLink.GDSLLink.displayName=IntelliJ IDEA GDSL
 Pipeline.Syntax=Pipeline Syntax
+# TODO perhaps create a https://jenkins.io/redirect/pipeline-field-set/
+LoggingInvoker.field_set=\
+  Did you forget the `def` keyword? \
+  {0} seems to be setting a field named {1} (to a value of type {2}) \
+  which could lead to memory leaks or other issues.

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecutionTest.java

@@ -269,6 +269,7 @@ public static class Execution extends AbstractStepExecutionImpl {
             }
             r.assertBuildStatusSuccess(r.waitForCompletion(b));
             new DepthFirstScanner().allNodes(b.getExecution()).stream().sorted(Comparator.comparing(n -> Integer.valueOf(n.getId()))).forEach(n -> System.out.println(n.getId() + " " + n.getDisplayName()));
+            r.assertLogContains(Messages.LoggingInvoker_field_set("WorkflowScript", "g", "CpsClosure2"), b);
         });
     }
 

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsVmExecutorServiceTest.java

@@ -139,7 +139,7 @@ public class CpsVmExecutorServiceTest {
     @Test public void okCatcherMetaClassImpl() throws Exception {
         p.setDefinition(new CpsFlowDefinition(
             "import org.codehaus.groovy.runtime.InvokerHelper \n" + 
-            "c = { println 'doing a thing' } \n" +
+            "def c = { println 'doing a thing' } \n" +
             "InvokerHelper.getMetaClass(c).invokeMethod(c, 'call', null)", false));
         WorkflowRun b = r.buildAndAssertSuccess(p);
         r.assertLogNotContains("MetaClassImpl", b);
@@ -150,7 +150,7 @@ public class CpsVmExecutorServiceTest {
     @Test public void okCatcherExpandoMetaClass() throws Exception {
         p.setDefinition(new CpsFlowDefinition(
             "import org.codehaus.groovy.runtime.InvokerHelper \n" + 
-            "c = { println 'doing a thing' } \n" +
+            "def c = { println 'doing a thing' } \n" +
             "c.getMetaClass().someField = 'r' \n" + 
             "InvokerHelper.getMetaClass(c).invokeMethod(c, 'call', null)", false));
         WorkflowRun b = r.buildAndAssertSuccess(p);

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/DSLTest.java

@@ -351,7 +351,7 @@ public void namedSoleParamForStep() throws Exception {
     * Tests the ability to execute a user defined closure with one arguments
     */
     @Test public void userDefinedClosure1ArgInvocationExecution() throws Exception {
-        p.setDefinition(new CpsFlowDefinition("my_closure = { String message -> \n" +
+        p.setDefinition(new CpsFlowDefinition("def my_closure = { String message -> \n" +
                                               "  echo message \n" +
                                               "}\n" + 
                                               "my_closure(\"my message!\") ", false));
@@ -363,7 +363,7 @@ public void namedSoleParamForStep() throws Exception {
     * Tests the ability to execute a user defined closure with 2 arguments
     */
     @Test public void userDefinedClosure2ArgInvocationExecution() throws Exception {
-        p.setDefinition(new CpsFlowDefinition("my_closure = { String message1, String message2 -> \n" +
+        p.setDefinition(new CpsFlowDefinition("def my_closure = { String message1, String message2 -> \n" +
                                               "  echo \"my message is ${message1} and ${message2}\" \n" +
                                               "}\n" + 
                                               "my_closure(\"string1\", \"string2\") ", false));
@@ -375,7 +375,7 @@ public void namedSoleParamForStep() throws Exception {
     * Tests untyped arguments 
     */
     @Test public void userDefinedClosureUntypedArgInvocationExecution() throws Exception {
-        p.setDefinition(new CpsFlowDefinition("my_closure = { a , b -> \n" +
+        p.setDefinition(new CpsFlowDefinition("def my_closure = { a , b -> \n" +
                                                       "  echo \"my message is ${a} and ${b}\" \n" +
                                                       "}\n" +
                                                       "my_closure(\"string1\" ,2)",false));

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/replay/ReplayPipelineCommandTest.java

@@ -54,6 +54,7 @@ public void rebuildNeedScriptApprovalCLIEdition() throws Exception {
         WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "SECURITY-3362");
         j.jenkins.getWorkspaceFor(p).child("a.groovy").write("echo 'Hello LoadedWorld'", null);
         String script =
+                "def a\n" +
                 "node() {\n" +
                 "    a = load('a.groovy')\n" +
                 "}\n";

plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/steps/ParallelStepTest.java

@@ -66,7 +66,7 @@ public void minimumViableParallelRun() throws Exception {
                 p = jenkins().createProject(WorkflowJob.class, "demo");
                 p.setDefinition(new CpsFlowDefinition(join(
                     "node {",
-                    "  x = parallel( a: { echo('echo a'); return 1; }, b: { echo('echo b'); sleep 1; return 2; } )",
+                    "  def x = parallel( a: { echo('echo a'); return 1; }, b: { echo('echo b'); sleep 1; return 2; } )",
                     "  assert x.a==1",
                     "  assert x.b==2",
                     "}"