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

@StandardException annotation for generating exception constructors #2702

Merged
merged 2 commits into from Apr 16, 2021

Conversation

ttzn
Copy link
Contributor

@ttzn ttzn commented Jan 6, 2021

Fixes #1559, related to #375 and discussed in this thread.

This is a pretty advanced attempt at implementing the proposed @StandardException feature (naming to be discussed). It turns the following:

@StandardException
public class ExampleException extends Exception {
}

... into:

public class ExampleException extends Exception {
    public ExampleException() {
    }
    public ExampleException(String message) {
        super(message);
    }
    public ExampleException(Throwable cause) {
        super(cause);
    }
    public ExampleException(String message, Throwable cause) {
        super(message, cause);
    }
}

If one of those 4 constructors is already declared, the handlers skip it. Naturally, compilation fails if any of the parent constructors is missing, as this is really meant to be used on subclasses of JDK throwables. Right now there is no configuration option to control the behaviour.

Before going further with cleanup and documentation, I'd like this to be thoroughly discussed. Some words on the implementation:

  • a huge portion of the code is copy-pasted from the HandleConstructors classes, with lots of cuts and modifications; some degree of factorization may be achieved by moving stuff into utility classes, which I plan to do if this feature is validated (along with a more comprehensive test suite);
  • I had to add stringType and throwableType in the javac Symtab stub, as I found no way to reference these types using available APIs.

I hope this proves useful. Thanks to the maintainers for all the hard work by the way.

@esend7881
Copy link

I thought this would have been part of the new 1.18.18 version but don't see it in there.... When will @StandardException get introduced into lombok, I'd like to use it.

@rzwitserloot
Copy link
Collaborator

I like it, but we're on the hot seat to push 1.18.20 out the door. Only real hold up is a thorough review. @rspilker - what are you thoughts on the feature itself?

TODO:

  • @ttzn did you add your name to AUTHORS?
  • docs - this needs a website/template/StandardException.html page with examples and a tag line.
  • We need to consider if this should go into experimental instead, to give it some time: That gives us a window to break how it works later on (whereas if we release it in the lombok package, that door is mostly closed).

* annotation with javac and Eclipse handlers
* single test file
* move feature under experimental
* replace ProviderFor with Provides
* add doc material (to be completed)
* add author
@ttzn
Copy link
Contributor Author

ttzn commented Mar 24, 2021

@rzwitserloot treating this as experimental seems fair, so I moved it. I also rebased, added my name and started documentation.

@rzwitserloot
Copy link
Collaborator

Reviewing the basic feature idea itself, I see some problems.

Some exceptions simply do not have the 'set of 4', so implementing all 4 constructors as, respectively:

  • super();
  • super(msg);
  • super(cause);
  • super(msg, cause);

will fail for these. Admittedly a lot of exceptions are growing this 'standard set', but there are still plenty that just do not have them. For example, java.lang.NumberFormatException still does not have them, even in JDK16.

I'm not quite sure why you'd want to extend that class, but you can (it is not final), and it would be surprising if @StandardException public class MyNumberProblemException extends NumberFormatException {} results in weird 'superconstructor not found' compiler errors. Lombok cannot resolve these types so cannot check if the expected 'set of 4' isn't there. Lombok can't even tell if the parent type is some sort of throwable in the first place, unfortunately. I'm willing to overlook that one, though.

A second issue is that you may want to do stuff in addition to the standard super call. By implementing the set-of-4 with a super call we make this impossible, and make the @StandardException feature unavailable for such cases.

We can fix both of these problems in one fell swoop but that brings in new problems.

Proposal:

The set-of-4 is implemented as follows (so, this is the delomboked version of @StandardException class Foo extends Throwable {}:

public class Foo extends Throwable {
    public Foo() {
        this(null, null);
    }
    public Foo(String msg) {
        this(msg, null);
    }
    public Foo(Throwable cause) {
        // This is duplicating the default behaviour of Throwable itself.
        this(cause == null ? null : cause.getMessage(), null);
    }
    public Foo(String msg, Throwable cause) {
        super(msg);
        if (cause != null) super.initCause(cause);
    }
}

This has considerable advantages:

  • If you want to perform some extra operations in your exception class, you can do so - by only manually inserting the full-args constructor. Lombok will notice it and simply not generate that one. The other 3 will then invoke the one you wrote. Convenient.
  • By default this works for virtually all extant Throwable subtypes, in java core and many third party libraries. They virtually always have the (String msg) constructor, and initCause is from Throwable itself, so that'll be there.

It also has disadvantages:

  • It is slightly less standard, even if more convenient.
  • It is functionally slightly different! super(msg, null) and super(msg) are not the same: The second call means you can later call initCause, whereas the first one (passing null for cause) locks away the ability to set the cause later with initCause, forever. Therefore the unfortunate upshot of the above example is that our code is very slightly different in the following manner: If you invoke e.g new IOException(msg, null), then you have locked the ability to initCause permanently. However, in our code, invoking new MyCustomExMadeWithLombok(msg, null), then initCause can still be invoked.

I think in the grand scheme of things this is the best available option, but I could be convinced there are better alternatives.

Thoughts?

@rzwitserloot
Copy link
Collaborator

Also, there is room for an expansion to the feature (perhaps better left as a second milestone and not for a first release):

What if we make it possible to annotate a constructor in a class with @StandardException. This then means: There must be both a String message and AnyType cause parameter, with those exact names; if either one is not there it's a compile-time lombok error. If they are there, lombok will generate the 3 missing constructors, derived by removing first the message param, then the cause param, then both, and they are implemented in the exact same fashion: a this call to the annotated constructor, with the caveat that the message-less, cause-containing variant uses cause == null ? null : cause.getMessage().

This would let you make constructors that take additional arguments.

Admittedly vastly more exotic than the usual 'just gimme the standard batch of 4 please' strategy.

@rzwitserloot rzwitserloot merged commit 2e212de into projectlombok:master Apr 16, 2021
@rzwitserloot
Copy link
Collaborator

I've reviewed and implemented the above changes (but not the 'annotate an existing constructor' suggestion). @rspilker still needs to review the feature, though. I'll try my best to make the case for this feature :)

@rzwitserloot
Copy link
Collaborator

@ttzn I had to refactor quite a bit but please don't take that as an indicator of quality of the PR - your PR was a welcome sight and helped a ton! Thank you!

@rzwitserloot
Copy link
Collaborator

One more update I'd like to put in: What should lombok do if the annotated class extends nothing at all? E.g:

@StandardException
public class MyException {}

The two obvious actions are:

  • Do not change anything, and put an error message on the annotation stating that you must extend some sort of Throwable type.
  • Inject extends Exception, for the highest levels of convenience.

Ordinarily I always prefer 'do not presume to know what the developer wants if there are multiple plausible answers, and instead generate an error that explains precisely what needs to be done', but there is a case to be made that 'just extend Exception' is convenient, and it does mean you can just write @StandardException public class Whatever{} which is such a pretty one-liner.

Thoughts?

@esend7881
Copy link

One more update I'd like to put in: What should lombok do if the annotated class extends nothing at all? E.g:

@StandardException
public class MyException {}

The two obvious actions are:

  • Do not change anything, and put an error message on the annotation stating that you must extend some sort of Throwable type.
  • Inject extends Exception, for the highest levels of convenience.

Ordinarily I always prefer 'do not presume to know what the developer wants if there are multiple plausible answers, and instead generate an error that explains precisely what needs to be done', but there is a case to be made that 'just extend Exception' is convenient, and it does mean you can just write @StandardException public class Whatever{} which is such a pretty one-liner.

Thoughts?

To make this logic work, perhaps making a nearly equivelent @StandardRuntimeException which is similar to @StandardException but instead extends RuntimeException.

I haven't tried to implement this, but I'm sure there is a way to do this with minimal repeated code. I suppose we could have

  • @StandardThrowable
  • @StandardRuntimeException
  • @StandardException

Where @StandardThrowable contains most of the logic and @StandardException or @StandardRuntimeException contain the specifics related to extending Exception or RuntimeException.

With this in mind, a user can either extend Exception/RuntimeException or not extend Exception/RuntimeException, it'd be a redundant statement.

@rspilker
Copy link
Collaborator

@esend7881 If I would choose between having the different annotations, or having the author type an extends clause, the latter would always win.

@esend7881
Copy link

esend7881 commented Apr 17, 2021

@esend7881 If I would choose between having the different annotations, or having the author type an extends clause, the latter would always win.

I agree, the user should manually specify the exception superclass. That actually makes more sense because a user could want to extend a specific exception such as IOException or IllegalArgumentException, etc.

@martianch
Copy link

Can you please finally build and release Lombok with this feature?

@rzwitserloot rzwitserloot added this to the next-version milestone Oct 7, 2021
@rzwitserloot
Copy link
Collaborator

@martiench release is imminent.

@NSnietol
Copy link

NSnietol commented Jan 7, 2022

Hi,
is this feature working nowadays? I'm using the version 1.18.22

I have this code

@StandardException
public class MyOwnException extends RuntimeException {

}

public class TestClass{

 public static void main(String ars[]) {
       throw new MyOwnException();
    }
}

At main method level I can't use any of those constructors you mentioned we can get by using the annotation
CC: @ttzn , @rzwitserloot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use lombok to create custom exception faster.
6 participants