Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn user when forgetting def on a variable #994

Merged
merged 9 commits into from
Feb 27, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what it's worth, "WorkflowScript seems to be setting a field..." sounds strange to me. I don't really have any better recommendations though. Maybe "Something is setting WorkflowScript.xyz (to ...)", but then I would expect users to ask "what something?" and there is no easy way for us to answer that question here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least in the cases I know about, it is code in the named script which is setting the field. I could try to make the SourceLocation available here (which would give you a line number) though it would be a bit more work since that is only available in groovy-cps whereas the TaskListener is only available in the plugin.

As to the naming WorkflowScript, rather than say Jenkinsfile, it would be nice to update that, though I would worry about the implications.

Copy link
Member

@dwnusbaum dwnusbaum Feb 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my concern is mainly that users may not realize that WorkflowScript means Jenkinsfile. I agree that we should always mention the real class name, but maybe it could make sense to add (your Jenkinsfile) or something if the class name is WorkflowScript, although that is not necessarily accurate depending on how the Pipeline is configured.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most simple declarative pipeline. Removed all global libraries (but I found zero reference of resolveStrategy or delgate anywhere in the code)

pipeline {
    agent none
    stages {
        stage('Hello') {
            steps {
                echo 'Hello World'
            }
        }
    }
}

declarative

Looks I'm not the only one https://issues.jenkins.io/browse/JENKINS-75353

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, did not think to check Declarative syntax, will try to fix today.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved.

which could lead to memory leaks or other issues.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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));
Expand All @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
"}"
Expand Down
Loading