eaa for java.util.Properties.getProperty(String,String) is incorrect

[JohnLussmyer]

The file has it as: getProperty (Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String; (L1java/lang/String;L1java/lang/String;)L1java/lang/String; which specifies that the 2nd parameter and return value must be NonNull. Looking at the code, they both can be null.

[agentgt]

It’s actually PolyNull which Eclipse NPE desperately needs to be honest.

Ditto for Optional.orElse.

Consequently I recommend avoiding defaultValue parameter methods if you can and instead use Objects.requireNonNullElse

[JohnLussmyer]

PolyNull might be nice, but I'm talking about current code. This is a LARGE project, and changing every occurrence where getProperty(key,def) is used - isn't going to happen.

[J-N-K]

Considering #127 we should change to @Nullable. Even if IMO a getOrDefault is quite useless if you add null as default.

I agree with @agentgt that the correct solution would be a @PolyNull.

[agentgt]

@JohnLussmyer It actually is fairly painful to have defaultValue parameter as @Nullable in code base that favors @NonNull. @J-N-K is absolutely right in that those methods become useless. Ditto for Apache commons lang3.

It is so painful I have contemplated switching Optional.orElse(@Nulalble ...) -> Optional.orElse(@NonNull ...). However Optional at least has orElseGet and there is also no way to get a null out of Optional chaining method other than orElse (As much as I don't like Guava... Guava's Optional is vastly superior to Java's Optional in this regard as it has orNull).

On the other hand Maps and Properties return @Nullable for their non defaultValue (e.g. getProperty()) parameters so you can and should wrap every call with something like Objects.requireNonNullElse or pre 9 you can use Eclipse JDT library org.eclipse.jdt.annotation.Checks#nonNullElse which if you are using the annotations is builtin.

This is a LARGE project, and changing every occurrence where getProperty(key,def) is used - isn't going to happen.

I would seriously investigate where (which is trivial with most IDEs) because everyone of those places is probably expecting getProperty to return a @NonNull. If you switch it to @Nullable you will have to wrap or handle everyone of those calls anyway.

[simoneparca]

OK, the implementation can accept null, and return null, so we may think is better to annotate as nullable. Of course who implemented this way tried to avoid null pointers inside the implementation, but the purpose of this function is to replace a null with something more meanignfull, otherwise is useless. Maybe it is not formally correct, but nonnull is more usefull in this case, and will require less re-engineering of legacy code. Ok, the method CAN accept null, but is better to restrict the usage to be nonnull. If both second parameter and return value are annotated as NonNull, we reach the main goal to warranty that the code will not have null pointer errors. This is a restriction on how the method can be used, but will make the overall usage simpler.

Inviato da Posta per Windows 10

Da: Adam Gent Inviato: giovedì 24 giugno 2021 22:21 A: lastnpe/eclipse-null-eea-augments Cc: Subscribed Oggetto: Re: [lastnpe/eclipse-null-eea-augments] eaa forjava.util.Properties.getProperty(String,String) is incorrect (#141)

@JohnLussmyer It actually is fairly painful to have defaultValue parameter as @Nullable in code base that favors @NonNull. @J-N-K is absolutely right in that those methods become useless. Ditto for Apache commons lang3. It is so painful I have contemplated switching Optional.orElse(@Nulalble ...) -> Optional.orElse(@NonNull ...). However Optional at least has orElseGet and there is also no way to get a null out of Optional chaining method other than orElse (As much as I don't like Guava... Guava's Optional is vastly superior to Java's Optional in this regard as it has orNull). On the other hand Maps and Properties return @Nullable for their non defaultValue (e.g. getProperty()) parameters so you can and should wrap every call with something like Objects.requireNonNullElse or pre 9 you can use Eclipse JDT library org.eclipse.jdt.annotation.Checks#nonNullElse which if you are using the annotations is builtin. This is a LARGE project, and changing every occurrence where getProperty(key,def) is used - isn't going to happen. I would seriously investigate where (which is trivial with most IDEs) because everyone of those places is probably expecting getProperty to return a @NonNull. If you switch it to @Nullable you will have to wrap or handle everyone of those calls anyway. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[JohnLussmyer]

I would seriously investigate where (which is trivial with most IDEs) because everyone of those places is probably expecting getProperty to return a @NonNull. If you switch it to @Nullable you will have to wrap or handle everyone of those calls anyway.

Nope. There is a whole bunch of code where the "it isn't set" value is expected to be null. For many of them, I'd have to add a "special" "it's not set" value and check for that instead. Yes, it's NOT a good design, but there are hundreds of these calls spread through dozens of files. Over 10 years of legacy code here. (Which I am trying to refactor as time permits - which is rarely.)

[agentgt]

@JohnLussmyer The problem is like I said before that it isn't @Nullable but @PolyNull for other frameworks.

If Eclipse NP analysis adds @PolyNull and or you use Checker Framework you will effectively add tons of "Dead Code" warnings to people's code base.

String something = someProperties.getProperty("blah", "someConstantThatIsNonNullOrSomeMethodNonNull");
if (something != null) {  // This will generate a warning in Checker as dead code and hopefully someday Eclipse
}

Lets Looking java.util.Properties java doc:

    /**
     * Searches for the property with the specified key in this property list.
     * If the key is not found in this property list, the default property list,
     * and its defaults, recursively, are then checked. The method returns the
     * default value argument if the property is not found.
     *
     * @param   key            the hashtable key.
     * @param   defaultValue   a default value.
     *
     * @return  the value in this property list with the specified key value.
     * @see     #setProperty
     * @see     #defaults
     */

Now lets look at Optional.orElse:

    /**
     * If a value is present, returns the value, otherwise returns
     * {@code other}.
     *
     * @param other the value to be returned, if no value is present.
     *        May be {@code null}.
     * @return the value, if present, otherwise {@code other}
     */

Notice the May be {@code null}.. I stress that because the JDK javadoc is pretty good about stressing when it can be null. getProperties(k, defaultValue) doesn't make any suggestions.

Speaking of Checker I recommend reading Checker's Tip: "Library annotations should reflect the specification, not the implementation".

Think of it this way... should every method that has `@NonNull parameters have to do this:

if (someNonNullParameter == null) throw new NullPointerException();

[J-N-K]

@agentgt The Javadoc is a very good argument against changing the annotation.

[J-N-K]

FTR: I created https://bugs.eclipse.org/bugs/show_bug.cgi?id=575822

[agentgt]

It might be best to see what JSpecify does in this regard. https://jspecify.dev/

JSpecify still hasn't implemented PolyNull yet. I'm not sure how they plan on handling it.

I'm not sure why Eclipse isn't involved in JSpecify either.