[JENKINS-53825] Choose from ambiguous class names via a simple heuristic

[abayer]

JENKINS-53825

If there are multiple possible classes matching a name on resolution,
filter for just classes that are either inner classes of the model
class, are in the same package as the model class, or are in
subpackages of the model class's package. If more than one class
passes those filters, we'll still fail.

Also starts reporting all possible clases when there are more than two
of them.

[stephenc]

Not entirely clear that heuristics are the best way. I think an extension point to help narrow options might be a better route

[abayer]

I'm thinking of this initial work as just a first step - still needs symbol resolution, and I would like to add an extension point as well. I'm starting small. =)

[abayer]

Yeah, symbol ambiguity resolution would definitely require a new API in SymbolLookup and a relevant extension point. I'll see if I can get anywhere with that in the next day or so, otherwise I'll pick that back up separately from this, since this should Do No Harm and does make the situation a little better.

[abayer]

@stephenc - I added some initial work on adding context to Symbol in the last commit - let me know if this smells like it's going down the right path. I feel like it is - it'll need some changes in workflow-cps's code to take advantage of it, but it works as is for DescribableModel instantiation and doesn't break existing behavior.

[abayer]

...check that, I wrote less tests by end of day yesterday than I thought I had, so I'm not 100% sure it works for Symbols and DescribableModel instantiation yet. =)

[stephenc]

I think I like this idea.

Would help to express your plan in the javadoc for this.

I think what you want is that where there are multiple options with the same name, if there is one with the "closest" context, then that one will win.

TBH I like that, because we can do something like @Symbol(value="discoverOriginPRs", context={GitHubSCMSource.class,GitHubSCMNavigator.class}) and we get the win.

The problem, as I see it, is in deciding which is the nearer context if the context is not an exact match.

i.e. If the context is an interface or a parent class.

You'll need to clearly spell out how you decide the nearest context... and if multiple symbols are the same name at the same distance, we fall back to ambiguous

[stephenc]

My original idea was to just have an extension point that the symbol library could consult in the case where it had conflicts.

The extension point would basically be given a list of candidate symbols and the context in which these were requested and remove any that it wants from the list.

We then walk all implementations of the extension in order. If the list is empty, we stop and say "no match". If we get to the end and there is more than one, we stop and say "ambiguous" and if we have exactly one "winner"

And we need to process the list of extensions all the way to the end, as we don't know if a later impl might remove the one option we are left with bringing us to an empty list

[stephenc]

But with this... while requiring changes on the implementation plugins... we get a less "magic" solution... so as long as you define what is "closest" I think this will work really well

[jglick]

This all sounds overengineered to me. Use

@Symbol("discoverGitHubForkPRs")

and go home.

[stephenc]

No. We’ll have 7-8 of the logical equivalent but all with redundant names repeating the SCM. I want to encourage a common set of patterns across the different SCMs

[abayer]

The first part of this (heuristically-determined disambiguation of short class names) is relevant before we even get to symbol naming philosophy - getting, say, '$class': 'SSHCheckoutTrait' in a map passed to DescribableModel.instantiate(GitHubSCMSource.class) to not cause a barf. The class names are already named the same, so that ship has sailed from that perspective, and I feel that needs to be addressed.

I'm a bit more soft on the symbol disambiguation - my gut instincts are to go with @jglick's approach of deliberately disambiguated symbol names, but I can see the argument in the other direction's merits.

[abayer]

Ok, actually, the more I work on implementing the symbol disambiguation and dealing with the pile of corner cases around nested UninstantiatedDescribables, and meta steps, etc...the more I think that symbol disambiguation may just be too damned messy to be worth implementing.

[jglick]

encourage a common set of patterns across the different SCMs

Then define a single SCMTrait for discovery of forks with its own extension point for that implementation. But keep the complexity out of this critical and widely used component. We do not, for example, want JEP-201 to have to be patched to deal with the same subtlety. It is just not worth it for this quite minor enhancement.

[abayer]

So, for the admittedly not publicly visible use case for this, what I need is a way to have '$class': 'SSHCheckoutTrait' actually resolve to the right SSHCheckoutTrait. The first chunk of changes here (before I started messing around with symbols) would let us figure out (with a similar heuristic to jenkinsci/scm-api-plugin#58, but as with there, I'd be completely open to a better implementation/heuristic) which of the possible classes is "closest" and use that one. That wouldn't require any changes to the branch source plugins, and wouldn't require the symbol disambiguation either.

Symbol disambiguation is only needed for what @stephenc wants - it'd be a slightly nicer user experience, but as I said above, I think it's probably more complex and edge-case-error-prone than is worth doing. I've got it "working" locally in workflow-cps for step parameters, nested parameters to other UninstantiatedDescribables in steps, and for meta step resolution, but it's complicated and I'm not confident that it won't hit edge case problems.

The simplest option would just be to add non-ambiguous symbols to everything I care about (namely various SCMSourceTraits), as @jglick suggested. That'd be a slightly less optimal user experience, and would involve actually adding all those symbols and releasing a bunch of branch source plugins, but it would get me to where I need to be without requiring any changes here or in workflow-cps. That's my first choice at this point, but if @stephenc feels strongly enough to block that approach, I'm stuck pretty badly.

[stephenc]

So here is my thoughts on the matter:

• If you enable non-FQCN simple classname disambiguation, then it seems really shitty to not provide the same for symbols. In fact, one could see it as an argument in favour of not using the @Symbol annotation in the first place... because you would just have the simple class name... and the only difference is that there is an initial capital letter and perhaps some trailing stuff that could be ignored.

• I do not think it is realistic to continue to bury our heads in the sand with respect to symbol conflicts. Unless there is a Jenkins global registry of who has grabbed which names, this is basically an accident waiting to happen. Consider the case where I have one plugin with a notificationPolicy symbol and my master is using this in many Jenkinsfiles. Then some other completely unrelated plugin is upgraded and also happens to have picked notificationPolicy for its symbol name. These would be two completely unrelated classes that are not assignment compatible other than through Object. The way things currently work, this becomes ambiguous and I am forced to rewrite all the Jenkinsfiles to use the ugly FQCN.

What I think is that we need to filter the list of candidates based on the setter/getter signature and allow other extensions to assist in that filtering. Ideally the filtering should also take into account the generic type bounds, as this would actually solve the SCM API use case... but I can see other cases where the API defining plugin may need to provide a "smarter" assistant to the filtering.

If you want to go the other way, then I respectfully ask that you setup a global registry of symbol names across all plugins and provide a process whereby people can register that they have grabbed that name in order to remove the risk of conflicts. If the absurdity of maintaining such a registry does not encourage you to see that we actually need symbol disambiguation, well I give up!