Search patched method also by parameters #3136
Conversation
|
The one question that comes up: That patch we do there - to remove generated simple names, should we also apply it to the overloaded |
|
Do you think it might be advisable to expand other MethodTarget-definitions with the parameter specifications in EclipsePatcher to prevent something similar happening with a future version of Eclipse? I guess adding overloaded methods might occur again and the error seems obscure enough that people don't know lombok needs an update, but go to the Eclipse site. Or maybe any other ideas how to best deal with probable version incompatibilites? Would it be e.g. advisable for Eclipse to have some explicitly defined extension point lombok could hook into, instead of having to rely on reflection? And another question: does the provided fix still work with older versions of Eclipse? As there are now hooks for two variants of findByNode -- what happens in an older Eclipse version? Is this silently ignored or will it cause refactoring to break in older versions? |
|
Yes, I think that expanding all definitions should prevent issues like this in the future. It will break some lombok related stuff but at least people will figure out that it is a lombok problem. I was also working on automated tests for the refactoring patches, this should also help to find problems like this easier. Hooks would be nice but adding them is a bunch of work and it will only work in new eclipse/ecj versions. The fix should also work in older versions, if it can not find a method to patch it simply does nothing. |
|
Okay, so Eclipse-hooks could only be a long term solution. I'm not deep enough in Eclipse internals yet, but if that changes some day I'm willing to contribute, as lombok is one of my favorite extensions :-). Looking at the original stack trace in #3134 I guess the reason for the exception was that MethodTarget was specifying a method in an ambiguous way, and lombok simply used probably the first found of them. Probably it would be good to check for such situations explicitly, and reject or ignore such ambiguous MethodTargets. Thus it would (at least partially) break proper functioning of the lombok part, but not break the general Eclipse functioning per se. |
|
In this case the patcher patched all methods with the name and ignores return type and arguments. This actually is quite handy in cases where you do not care about it or if the patched code is stable. Adding return and argument types for all |
This PR fixes #3134
Eclipse added a new
findByNodemethod toLinkedNodeFinder. Because both use the same name we need to include the parameters in the search to find the correct one. I also added a patch for the new method.