Migrated from Google Code (issue 19)

👤 danielmalcolmabraham 🕗 Aug 19, 2013 at 12:58 UTC <p dir="au

Now for some specific points: I now get bugs because I did wan

👤 serverperformance 🕗 Jul 03, 2012 at 23:07 UTC <p dir="auto"

👤 askoning 🕗 May 09, 2013 at 08:15 UTC Issue <a

@Data needs a callSuper.,about projectlombok/lombok

Comments (29)

dabrowt1 commented on July 17, 2024 2

This seems simple but would be very helpful.

from lombok.

lombokissues commented on July 17, 2024 1

👤 danielmalcolmabraham 🕗 Aug 19, 2013 at 12:58 UTC

I'm sure this is obvious, but when this happens, could it happen on @ Value as well please?

Thank you very much.

from lombok.

randakar commented on July 17, 2024 1

You folks misunderstand what resolution means. Lombok has no access to the superclass at all when it is invoked. It can access the class itself, but nothing outside it. It's not just "more difficult". It's close to impossible. It's not a compiler nor does it have the same level of access that javac does. If it were possible it would require cooperation of the java compiler that does not currently exist. And the lombok devs have bigger fish to fry. All of that means that it can't know if there is a sensible implementation of toString, hashcode, or equals at all. So it can't warn you if it what it generates makes zero sense. For the constructor of the superclass the picture is a bit easier. If the superclass doesn't have a default constructor you will get the same compile error that you'd get if you were to call the wrong constructor in your own code, and the user can fix it in the same way. But with the equals and hashcode methods especially calling the superclass version blindly is dangerous and liable to introduce subtle bugs. Lombok is there to make your life easier, and that means not to giving you "features" that may blow up in your face.

…

On Sun, Mar 1, 2020, 13:55 Jordan Silva ***@***.***> wrote: @janrieke <https://github.com/janrieke> I got a bug because I annotated my class with @ToStringAndHashCode and the super methods were not called (I assume it would be the default). In my opinion, implementing toString and hashCode is mandatory and any developer should follow it because not doing so creates lots of hard to find bugs. Even worse than not implementing those methods, is to implement in the child class and not in the parent class (when it is not Object). If you got a case when you only need to look at the child to see if these objects are equal or not, then most likely the class should not have a parent. This is a good OO pattern and libraries should follow and enforce, as maximum as possible, good patterns. Therefore, have callSuper = true as default is the most appropriated. For the developers that don't follow it, this behavior is configurable, so they can set callSuper = false. Regarding the warning, in my current company, they use @ToStringAndHashCode(callSuper = false) and I asked them why and the answer was that without it they get a warning! I guess the same is happening in other companies and I think this is not the intention of the warning. So I suggest you guys improve the message so people will *think* if they should set it to true or false. *Orignal topic* Will it make things more complicated? As you said, yes. However, it will be more complicated for Lombok and the developers that don't follow good practices. And I don't think it is a big deal for Lombok to solve the problems from it, because you guys already solved for other annotations. For instance, when annotating a class with @value @builder, or when in the class level we have @DaTa and in a field, we have @getter(AccessLevel.PRIVATE). It is just a matter of setting the rules, which should not be complicated. I also don't think it would be difficult to implement, the current solution for the Constructor annotations requires the parent class to have a default constructor and this constraint could be kept as @DaTa is just an umbrella for the other annotations. If we stick the solution to the behaviour we have currently, it is completely doable. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#92>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABIERIRWY52N4CQKMO67NDRFJLLFANCNFSM4CIJNVQQ> .

from lombok.

rzwitserloot commented on July 17, 2024 1

Now for some specific points:

I now get bugs because I did want to call super impls, and lombok by default does not.

That's what the warning is for. Unfortunately, the semantic meaning of 'warning' is not universally understood. Team lombok espouses the idea that warnings are virtually as bad as errors are, they only real difference is that, unlike with an error, you can continue your test run or debug session with warnings. Just like with errors, you should definitely not leave them in, check them into source control (unless in a debug branch), and they should absolutely never make it to production.

With that mindset, this problem goes away. Unfortunately it is not possible to cater to the mindset that warnings may safely be ignored; there is no default behaviour that works in all cases. You've run into a situation where 'call the supers' was a better choice, but there are cases where it is the wrong choice; there is no one-answer-fits-all, which is why we generate that warning. Somebody needs to make a choice and lombok is not technically capable of automatically figuring out which one is correct. We cannot fix this.

... but my team did read the warning, didn't understand it, stuck 'callSuper = false' on it because it makes the warning go away, but now we still have bugs.

At risk of belittling your team's intelligence, I'm disregarding this one as a problem of lombok. For the same reason: If we posit that that's how development works, it is impossible for lombok to do the right thing. I'm taking this mindset (hey, if I do this, the warning goes away! Clearly that means I wrote correct code) – as a kind of code style I sometimes observe: You more or less type random stuff into your editor until things seem to work out (no errors, in this case no warnings, and a first run seems to indicate things appear to be working). If you are neither willing to fully test your code nor willing to take the time to understand what code (or in this case, a warning) means, then you're going to have bugs. There is nothing lombok can do to stop it from happening, and thus whilst I am totally on board that the best would be that lombok somehow prevents bugs here, we... can't. There's no point keeping an impossible feature request on the books.

how does SuperBuilder work?

... it works, and without resolution. If you delombok an @SuperBuilder class you can see the magic. It's.. a lot of weird generics. The way superbuilder was done simply cannot be applied sensibly to normal classes, and note that superbuilder inherently only works if the entire chain was all built with superbuilder; lombok's callSuper parameter is intended to work even if your parent's class has no lombok-generated implementations of toString and the like (though, given that it is impossible to just get it right silently if they aren't, lombok generates that warning: It's asking you to tell lombok that the parent's impls are compatible, and that you are willing to sign up to the notion that the API of the parent class is now signed up to maintaining this compatibility. Lombok obviously cannot silently inject future promises, which is why it has to be explicit).

Okay, so back to the simple case, the original intent of this feature request: @Data(callSuper = false)

Other than the precedence issue (which can be solved with Maaartinus' idea: The more specific one takes precedence), there's the constructor issue: @Data covers many annotations, and one of them doesn't have a callSuper: The @RequiredArgsConstructor. I can imagine someone reading the code would assume that Data(callSuper=true) also implies the non-existent @RequiredArgsConstructor(callSuper = true), and that this non-existent feature implies to call that constructor which covers each required arg. Which can be done – but only with resolution, and I assume everyone is aware that many lombok users are unaware that lombok cannot easily provide resolution-based features, and if not, well, I submit this very thread as Exhibit A :)

Given that I feel unsure that the text that appears in your source code will be properly understood, and that the feature only saves a relatively small and stable amount of boilerplate (does not expand if you add more fields, nor does it require that you rewrite it if you change your fields), the feature is not accepted.

PRs will not be merged.

To change this conclusion, I think it'd be a good idea to start up a new feature request (this one has been all over the place), referencing this one for historic context, but it would need a solid, implementable plan to solve the above issues. And frankly, I don't see any feasible route to that other than 'lombok does resolution now, with no caveats or maintenance burden'. Which we do want, one day. That day isn't today. I'm not worried about this feature once that day arrives: Changing what @Data and friends do will be the very first thing we'd use our shiny new 'oooh we have resolution now!' hammer on, I don't need an issue to remind me of how nice that'd be :)

from lombok.