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.

Sign up for GitHub

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
Merged

Warn user when forgetting def on a variable #994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
83d3c25
Exploring a warning when a field is set
jglick Feb 27, 2025
9d03e27
Limiting matches to non-null assignments to `CpsScript`s, and suppres…
jglick Feb 27, 2025
71180bf
Seem to be getting warnings only in appropriate places
jglick Feb 27, 2025
9fbabd7
Might want a redirect
jglick Feb 27, 2025
74b77d5
Better message
jglick Feb 27, 2025
14b75a6
Further tweaking message
jglick Feb 27, 2025
64b88c4
Forgot to reuse computed `owner`
jglick Feb 27, 2025
17d3c1a
Kill switch https://github.com/jenkinsci/workflow-cps-plugin/pull/994…
jglick Feb 27, 2025
de7dad4
Ignore `@Field`s https://github.com/jenkinsci/workflow-cps-plugin/pul…
jglick Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/LoggingInvoker.java
View file
Open in desktop
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
5 changes: 5 additions & 0 deletions plugin/src/main/resources/org/jenkinsci/plugins/workflow/cps/Messages.properties
View file
Open in desktop
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 •
edited
Loading

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.

Copy link

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.
1 change: 1 addition & 0 deletions plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsBodyExecutionTest.java
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsVmExecutorServiceTest.java
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/DSLTest.java
View file
Open in desktop
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
1 change: 1 addition & 0 deletions ...in/src/test/java/org/jenkinsci/plugins/workflow/cps/replay/ReplayPipelineCommandTest.java
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/steps/ParallelStepTest.java
View file
Open in desktop
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