Warn user when forgetting def on a variable

[jglick]

Following up #925: not only avoiding the symptom, but warning about the mistake. (Though not from @NonCPS methods.)

Note that this uses a distinct logging system from either #211 or #280, since in this case at the point at which the warning is printed we have easy access to a TaskListener (though PropertyAccessBlock would have additional access to a SourceLocation which may be helpful).

[jglick]

Seems to work as desired also in libraries: assigning to an implicit field of a vars/*.groovy produces a warning, while assigning to a field of a named class in src/**/*.groovy does not.

[dwnusbaum]

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.

[jglick]

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.

[dwnusbaum]

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.

[jonesbusy]

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'
            }
        }
    }
}

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

[jglick]

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

[jglick]

Resolved.

[dwnusbaum]

Oh, and it might be worth adding a system property to disable this. Previously, when security fixes broke @Field there were lots of complaints, so for people setting script fields intentionally these warning messages might be something they do not care about and will not fix.

[dwnusbaum]

(Or I guess you could also check if the property matches a field defined on the script if @Field specifically is a concern)

[PayBas]

@jglick I couldn't get rid of the warnings in some specific cases.

I've boiled it down to:

pipeline {
  agent none
  
  stages {
    stage('FOO') {
      matrix {
        axes {
          axis {
            name 'OS'
            values 'Windows', 'Linux'
          }
        }
        stages {
          stage('BAR') {
            agent any
            steps {
              script {
                echo "${OS}"
              }
            }
          }
        }
      }
    }
  }
}

Leading to:

[Pipeline] Start of Pipeline
Did you forget the `def` keyword? WorkflowScript seems to be setting a field named __model__Closure_0_1201446834__ (to a value of type CpsClosure2) which could lead to memory leaks or other issues.
[Pipeline] stage
[Pipeline] { (FOO)
[Pipeline] parallel
[Pipeline] { (Branch: Matrix - OS = 'Windows')
[Pipeline] { (Branch: Matrix - OS = 'Linux')
[Pipeline] stage
...

That's with 4043.va_fb_de6a_a_8b_f5

[dwnusbaum]

@PayBas From a quick look, I think that anyone using the matrix directive as well as anyone using the SCRIPT_SPLITTING_TRANSFORMATION mode described in JENKINS-37984 will get false positive warnings similar to what you listed due to code like this and this. Whether this could be easily fixed, I do not know. Perhaps Wrapper.createScriptContextVariable could be adjusted to explicitly insert variables into Script.bindings to avoid these warnings.

[PayBas]

@dwnusbaum it's not a big deal. We can ignore it. But some of our developers were wondering if they missed something somewhere, which it turns out they didn't.
So I thought I'd at least notify you guys.

Keep up the good work!