Implementing a @NonEmpty annotation

[drejc]

I'm in the process of implementing a @NonEmpty annotation for Lombok.

The aim is to produce a non empty check for Strings, arrays, Lists, Maps, Collections and Sets.

I have took the NonNull implementation as a starting point and so far was able to produce the following de-Lomboked code:

BEFORE:

public Test(@NonEmpty String id) {
    this.id = id;
}

AFTER:

public Test(@NonEmpty String id) {
    if (id == null) {
        throw new java.lang.IllegalArgumentException("id is null or empty");
    } else {
        if (id.trim().length() == 0) {
            throw new java.lang.IllegalArgumentException("id is null or empty");
        }
    }
    this.id = id;
}

So far so good ... but once I build with ant dist and try the new lombok.jar on a separate project I get the following error:

/Test.java:15: error: cannot find symbol
    public Test(@NonEmpty String id) {
                ^
  symbol:   variable id.trim().length()
  location: class Test
1 error

Is there a way to get more information out of Lombok to find out where the problem is ... 

running: java -jar lombok.jar delombok -p Test.java 

works just fine?

My second question is ... how can I find out what type of variable is annotated (if it is a String, array, Set ... ) ... 

in order to produce the correct lenght() / size() check? I was looking all over the Lombok project but was not able to find any examples where this is done.

So how can I from the AST find out the type / parent type of the variable ... as in Java is done with instanceof ... 

Could you point me in the correct direction?

[Victor Stafusa]

Let me give you my $0.02.

Just like @NonNull, all of the lombok annotations should be removed in the generated code. Thus, the de-Lomboked code should not feature the @NonEmpty annotation anymore.

Further, the AFTER should be ideally like this:

AFTER:

public Test(String id) {

    if (id == null) {
        throw new java.lang.IllegalArgumentException("id is null or empty");
    }

    if (id.trim().length() == 0) {
        throw new java.lang.IllegalArgumentException("id is null or empty");
    }
    this.id = id;
}

Finally, I see some overlap with @NonNull, but I'm not sure what to think about it.

Victor.

[Martin Grajcar]

On Tue, May 23, 2017 at 9:18 AM, drejc <drey...@gmail.com> wrote:

I'm in the process of implementing a @NonEmpty annotation for Lombok.

The aim is to produce a non empty check for Strings, arrays, Lists, Maps, Collections and Sets.

I have took the NonNull implementation as a starting point and so far was able to produce the following de-Lomboked code:

BEFORE:

public Test(@NonEmpty String id) {
    this.id = id;
}

AFTER:

public Test(@NonEmpty String id) {
    if (id == null) {
        throw new java.lang.IllegalArgumentException("id is null or empty");
    } else {
        if (id.trim().length() == 0) {
            throw new java.lang.IllegalArgumentException("id is null or empty");
        }

Throwing exactly the same exception with exactly the same text from two different places is simply wrong. Either combine the conditions (so you get an excuse for not being user-friendly) or use different message or exception type (an NPE in the first case is pretty standard). 

    }
    this.id = id;
}

So far so good ... but once I build with ant dist and try the new lombok.jar on a separate project I get the following error:

/Test.java:15: error: cannot find symbol
    public Test(@NonEmpty String id) {
                ^
  symbol:   variable id.trim().length()
  location: class Test
1 error

Is there a way to get more information out of Lombok to find out where the problem is ... 

running: java -jar lombok.jar delombok -p Test.java 

works just fine?

No idea. 

My second question is ... how can I find out what type of variable is annotated (if it is a String, array, Set ... ) ... 

in order to produce the correct lenght() / size() check? I was looking all over the Lombok project but was not able to find any examples where this is done.

So how can I from the AST find out the type / parent type of the variable ... as in Java is done with instanceof ... 

Could you point me in the correct direction?

I'm afraid, this isn't easily possible as it requires resolution (accessing information from other classes), which is very problematic and a blocker for many potential features in Lombok. On one hand, using instanceof should be fine as the JIT is smart enough to remove impossible conditions. OTOH, unknown classes and classes having no notion of emptiness must lead to a compile-time exception. Constraining the annotation to the simplest cases is the best solution I can come with. However, @ExtensionMethod also needs resolution and it works, so looking into it should help.

[Reinier Zwitserloot]

A few things to tackle. Let's start with the concept of resolution.

Lombok has a chicken and egg problem.

If we let the compiler get far enough to have a 'database' of 'this type extends these types, and these types have these members in them', then we're too late to generate new methods.

If we get in before the compiler gets that far we can add new methods, but, that information is not available.

The true answer to solve this is to either force rounds (tell the compiler to start over, and preferably in a way that your compile times aren't doubled!), or to inject methods fully typechecked into the database, and to solve some comes-before/after craziness.

Lombok has solved this problem by injecting BEFORE that information is available, because this is MUCH simpler. There are a few things lombok does that work after (notably, 'val' and @ExtensionMethod), but those are really REALLY complicated. Definitely not worth it for this feature so let's not go there.

With that background, let's get into your questions:

My second question is ... how can I find out what type of variable is annotated (if it is a String, array, Set ... ) ... 

As per the above explanation: _You can't actually know this_.

What you CAN do is ask if a given type reference in the source is some specific fully qualified type name. So, you can ask 'is this node referring to java.util.List'? in which case lombok can tell you: Yes, or no. What you cannot do is: "Here's this typeref 'List'. Can you give me the fully qualified name of that, and also a list of its supertypes so I can check if j.u.List is in htere?". You also can't ask: "Is this type a subtype of j.u.List?".

 

in order to produce the correct lenght() / size() check? I was looking all over the Lombok project but was not able to find any examples where this is done.

There's no need for this. They all have .isEmpty() except arrays, but arraytypes are detectable without resolution; those [] or ... in the type node can only mean one thing, after all.

I'm in the process of implementing a @NonEmpty annotation for Lombok.

Given the limitations of resolution, you can't do this simply. You need to make some trade offs. There are a few ways to go with this:

1. Strict-set typed: Your @NonEmpty feature 'ships' with a long list of known 'has emptiness' types. Presumably, all arrays, j.l.String, j.l.CharSequence, j.l.StringBuffer, j.l.StringBuilder, j.u.Collection and all the various subtypes of that in j.u and java.util.concurrent, j.u.Map and subtypes of that in j.u and j.u.c, and all the collection types that guava offers, that'd be a good start. Possibly some java.awt and javax.swing lists and the like but that's reaching a bit. If the argument type is one of those EXACT types (And not an unknown subtype of any of those!), lombok will do the right thing, and if not, an error occurs that lombok doesn't know how to check for emptyiness on that.

2. Structural typed: If it's an array, call .length == 0. If not, just call .isEmpty() and pray it exists. If it doesn't, javac/ecj will eventually get around to generating a 'that method doesn't exist' error on it.

3. config system: Use lombok's config system to start at strict-set typed and then allow adding more custom types.

Given the mental complexity of option #3, and the limits of option #1, I'd go with option #2 here, even though 'structural typing' is very un-java-like (public static void main will have to remain as an ugly design wart that makes no sense whatsoever. That's an unfortunate break of the rule that java just does not do structural typing).

 

The aim is to produce a non empty check for Strings, arrays, Lists, Maps, Collections and Sets.

See, that's just not possible. at best you can either pick 'anything that has an isEmpty() method', OR you pick: A very specific list of types; no subtypes of those are allowed unless they are hardcoded into the feature.

 

I have took the NonNull implementation as a starting point and so far was able to produce the following de-Lomboked code:

BEFORE:

public Test(@NonEmpty String id) {
    this.id = id;
}

AFTER:

public Test(@NonEmpty String id) {
    if (id == null) {
        throw new java.lang.IllegalArgumentException("id is null or empty");
    } else {
        if (id.trim().length() == 0) {
            throw new java.lang.IllegalArgumentException("id is null or empty");
        }
    }
    this.id = id;
}

Bad choices.

1. if null, throw an NPE and have the message be the param name. so: if (id == null) throw new NullPointerException("id").

2. If empty, say 'empty'. Don't say 'null or empty'.

3. trim() is not sensible here; for starters only strings have that, and secondly, even string itself disagrees with you: "   ".isEmpty() returns false. We're programmers. a string with spaces isn't empty. Arrays start at 0. Dates start with a year. daylight savings time is an expensive mass delusion. This is a knock against the feature: Some programmers erroneously seem to think strings with only blanks in them are still empty, so they'd be surprised when this feature doesn't trigger on that, but other programmers would be surprised by the reverse. No way to do it without surprising somebody, I'm afraid. Best to surprise those with an inconsistent belief set; they are doomed to be surprised no matter what happens.

4. all relevant classes for this, _except_ arrays, have an .isEmpty() method. Stop calling length()/size(), there is no need for it.

I'm still in favour of this idea (but without trimming).

There are a few other things to consider:

1. Should there be an option: @NonEmpty(trim = true). I vote no. Just get rid of this obsession that strings with spaces is empty; that comes up ONLY when receiving user input, and user input should be normalized and cleaned on receipt and not propagated in its dirty state throughout your codebase. Leave the trimming of inputs to the cleaning function, not to general APIs.

2. Should @NonEmpty imply @NonNull? I vote yes, and with no way to avoid it. @NonEmpty WILL generate a null check and there's no way to tell lombok to cut that out. If you really must have a non-empty check without a non-null check, handwrite it; that is no longer boilerplate. The alternative is to only generate the empty check, but that'll throw an NPE without a sane error message, and the only thing you save is negligible class file size and processing time; not worth it.

 

So far so good ... but once I build with ant dist and try the new lombok.jar on a separate project I get the following error:

/Test.java:15: error: cannot find symbol
    public Test(@NonEmpty String id) {
                ^
  symbol:   variable id.trim().length()
  location: class Test
1 error

lombok does some special voodoo magic to ensure most of the stuff lombok uses is not going to infect your classpath. This voodoo works without any further configuration but only for lombok.* stuff. Also, you can't add another jar in addition to lombok (so you can't put your handlers in a separate jar and them try to put both lombok and your jar on the classpath. You have to build a forked lombok jar, with your additions in the lombok package).

Not sure if you're doing that or not, but if not, that'd be the problem.

if you ARE doing that, is this on github someplace? That's probably the fastest way I can tell you what's going on.

 

Is there a way to get more information out of Lombok to find out where the problem is ... 

running: java -jar lombok.jar delombok -p Test.java 

works just fine?

Yes, the magic isn't applied when you do that because there's no issue of infecting classpaths in that mode. Leads me to believe the above is the problem.

 

[Martin Grajcar]

On Mon, Jul 10, 2017 at 9:32 PM, Reinier Zwitserloot <rein...@gmail.com> wrote:

2. Should @NonEmpty imply @NonNull? I vote yes, and with no way to avoid it. @NonEmpty WILL generate a null check and there's no way to tell lombok to cut that out. If you really must have a non-empty check without a non-null check, handwrite it; that is no longer boilerplate. The alternative is to only generate the empty check, but that'll throw an NPE without a sane error message, and the only thing you save is negligible class file size and processing time; not worth it.

Having no way to avoid it is the only part I disagree with.

There's an important use case where disallowing empty strings and allowing null makes sense: database entities. I want everything to be uniquely represented and oftentimes there's no need to have both the empty string and null. Since a mapped entity must be mutable, using @NonNull makes little sense, so I'd like to use @NonEmpty(allowNull=true) (or maybe @NonEmpty @Nullable).

Currently, I'm doing it the other way round (forbidding null and allowing empty), but it has some disadvantages. I don't claim that allowing null and forbidding empty would be better, but I'd like to have a chance to try it out.

As there are tons of entities and each of them has many strings, it is boilerplate.

[pbihler]

I'm still in favour of this idea

That's great news!

When I brought up the @NonEmpty annotation a good 5 years ago on this list, you're comment was a "We don't think it's particularly common. (...) WontFix, sorry." - so I really appreciate you're reevaluation :)

Pascal