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

[JENKINS-31314] Integrate and test a groovy-cps diagnostic trick #211

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jglick
Copy link
Member

jglick commented Mar 9, 2018

Downstream of cloudbees/groovy-cps#85.

  • catching cases I could think of (incl. one inspired by a question from @quivers)
  • false positives on default Groovy methods
  • false positive on metasteps (e.g., DSLTest.dollar_class_must_die3)
  • false positive on global variables (e.g., RestartingLoadStepTest.accessToSiblingScripts)
  • false positive on evaluate (e.g., CpsFlowExecutionMemoryTest)
  • false positives on stuff in CpsDefaultGroovyMethodsTest (CpsBooleanClosureWrapper?)
  • false positives on get in CpsDefaultGroovyMethodsTest.withDefaultList/map
  • look for false positives in workflow-cps-global-lib tests
  • look for false positives in pipeline-model-definition tests

@jglick jglick requested review from kohsuke and abayer Mar 9, 2018

jglick added some commits Mar 13, 2018

jglick added some commits Mar 13, 2018

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Mar 14, 2018

So the CpsBooleanClosureWrapper one actually doesn't surprise me, after my latest failed attempt at dealing with the .sort {} stuff a week or two ago: I honestly was surprised that the CpsBooleanClosureWrapper stuff even works in the first place. In fact, I'd bet that there are permutations you can create that wouldn't work (i.e., closures defined within the map.any { k, v -> ... } closure would probably short-circuit). So the fix there is probably for me to grind out an actual solution for DefaultGroovyMethods closure methods that perform/expect coercion into a boolean. Yay.

withDefaultList/withDefaultMap seems similar - [].withDefaultList { i -> i * 2}.get(1) ends up creating an instance of groovy.lang.ListWithDefault with the provided closure in its initClosure field. And then when we call get(1), it ends up routing through getDefaultValue(int idx), which returns initClosure.call(new Object[]{idx}). Again, a nested closure within the block would probably break this. So...again, a generalized solution is needed. Fun.

fwiw, the only CpsDefaultGroovyMethods that spit out an expected to call ... are the withDefault... methods and Map methods that need a boolean condition - so any, count, every, find, and findAll. So BooleanClosureWrapper interaction and ListWithDefaults interaction seem to be the only currently functioning false positives.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Mar 14, 2018

...the fun in both those cases and the general incorrect behavior of .sort {} and friends is that they all end up having the actual call of the closure happening in another class, not in DefaultGroovyMethods - i.e., BooleanClosureWrapper#call, ListWithDefaults#getDefaultValue, which are the simple cases (i.e., in the naive case, they work but do so in such a way that it's actually cheating, hence the expected to call ... message), and...the sort hell.

sort and friends are particularly hellish because their call of the closure is actually a couple levels deep: the closure ends up getting passed to an instance of a Comparator (either groovy.util.OrderBy or groovy.util.ClosureComparator), which themselves are passed to a Collections.sort call...so the actual call ends up happening when something deep in the Java sorting/comparing logic calls compareTo on the OrderBy or ClosureComparator instance. I got stuck on that because I really couldn't figure out what the right solution would be - honestly, it almost seemed like what we needed there was to have our own CpsOrderBy and CpsClosureComparator replacements that do the call in a new CPS thread or something.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Mar 14, 2018

the only currently functioning false positives

No, see RestartingLoadStepTest.accessToSiblingScripts. I suspect the same applies to any library vars/x.groovy with a call method (i.e., the most common use case for libraries): a source call x(…) will throw CpsCallableInvocation from a call method via CpsClosure, but I am not yet sure how we detect that. OTOH I was unable to reproduce a similar mismatch merely from

def x = {-> 2 + 2}; println(x())

so I am not sure exactly where the problem lies or what form the solution would take.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Mar 14, 2018

I got stuck on [handling sort]

Well, if we can get this PR working, at least those cases would become more obvious to users and there might be fewer complaints and confusion.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Mar 14, 2018

(note - looks like the BooleanClosureWrapper and ListWithDefault cases are more robust than I thought - or at least I haven't managed to break them with nested closures yet)

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Mar 14, 2018

Sorry, I meant the only current false positives in CpsDefaultGroovyMethods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.