-
Notifications
You must be signed in to change notification settings - Fork 26
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
What about lazy initialization/setters #431
Comments
We've gone around in circles on this topic a few times, so it is probably not the kind of thing I should try to answer in my last 15 minutes before the weekend. And yet... :) A big part of our answer is a principle that we do have settled: The goal of JSpecify isn't to specify how nullness checkers behave; it's "just" to provide an agreed-upon way to specify the types of fields and other elements. So, we define rules that imply things like " So, in many ways, we're punting on answering this question. Different tools will take different approaches to answering it. (And in fact they do already :)) We want to define things in a way that is consistent with a variety of approaches. At least for now, that's not going to include, say, a That still leaves the obvious question unanswered: "So, if you don't initialize a field during the constructor, does it have to be There are some related questions that we might conceivably make stronger recommendations on, but even those are up in the air. For example, is it "wrong" to write |
I will just add to what @cpovirk wrote is that I suspect (assuming JSpecify adoption is good) overtime many frameworks will adapt to the idea that say One of the overwhelming usages of classic mutable POJOs where you have this problem is JPA aka Hibernate and I would argue it is largely one of the last libraries that still embraces it pretty heavily. Newer frameworks have been for better or worse focused on generating code at compile time and because they do that they can make it so these problems are treated automatically and safely (e.g. some accessor method that checks if a value hasn't been set or perhaps some builder like Immutables). Combine this with newer For example you see a lot less of this code today: if (getSomeNullable() != null) {
getSomeNullable().doSomething()
} The only way to support the above is in similar vain to So people will start writing the better safe version (both in terms of nullness and thread safety): var x;
if ((x = getSomeNullable()) != null) {
x.doSomething()
} So I hope overtime the few places where people kind of disregard JSpecify rules will be just a handful similar to how there should only be a handful of places of where "unsafe" code is used. And JSpecify timing albeit I wish was early is great because Consequently it is important to get JSpecify released with as minimal annotations to see and measure how it will change developers and code. |
I think #225 is probably the relevant past discussion. In summary, I think that API elements should be annotated appropriately to their "alive" state only, and that this and other initialization-gap issues would be best addressed separately. EDIT: to be clear, this is a stronger stance than what @cpovirk is saying above, and might still be debatable, although there is a lot of background behind the stance already. |
no, if you don't provide a solution then jspecify won't work correctly, there are too many edge cases to have real null safety at that point. Some kind of beans are, in the web backend world, which might be the most common use for java, are the most common thing. Other tools have provided other solutions, but it's not acceptable to provide a spec and annotation that doesn't include this. It simply means that you're not providing a complete solution and this can't really be a solution for things like the JDK to later adopt. I'm going to need to figure out a way to make this work, today. Going to go remind myself how nullaway does it. Which sometimes relies on external annotations to do things. To many libraries on my classpath to accomplish 1 thing. I know, for example that JPA is going to call that setter early and make it not null... and nothing external can call it. // © Copyright 2024 Caleb Cushing
// SPDX-License-Identifier: AGPL-3.0-or-later
package com.xenoterracide.jpa;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.validation.constraints.NotNull;
import java.util.UUID;
import org.jspecify.annotations.NonNull;
@Entity
public class TestEntity extends AbstractEntityBase<@NonNull UUID> {
protected TestEntity() {}
public TestEntity(@NonNull UUID id, @NonNull String name) {
super(id);
this.name = name;
}
private @NonNull String name;
@NonNull
@NotNull
@Column(nullable = false)
public String getName() {
return name;
}
protected void setName(@NonNull String name) {
this.name = name;
}
@Override
protected boolean canEqual(@NonNull AbstractEntityBase<?> that) {
return that instanceof TestEntity;
}
} |
I do find the current state of affairs to be unsatisfying, but I think it makes sense for us to start without a solution to initialization and then revisit it later:
|
If you wait until later, then code doesn't compile in the meantime. At least not if you're using a solution like nullaway. You're also giving yourself a very false sense of security. Valhalla, apparently another thing that don't know what it is. I guess. Seems like number four is something we're going to wait a long time on. At a glance it also sounds exactly what Jspecify is supposed to be doing, or at least leading the way on. https://openjdk.org/projects/valhalla/ You are suggesting that users don't want to annotate their own code in any way shape or form? And that they aren't library writers that are writing these kinds of things in the first place. I mean I'm commenting on this right now because I'm writing a library that has this problem; a library that will then be consumed by things that will continue to have this problem. I don't feel like it's really worth arguing, because Jspecify is either going to provide a complete specification instead of annotations or it's not which makes yet another solution that really doesn't solve the problem. |
How about we first make sure we all share the same understanding of the status quo -- of how things will look if this issue is either closed or tagged post-1.0. (Next we can try to agree on what this issue is asking for: exactly what changes would resolve it? Only after all that comes the deciding what to do part.) Here's what I think is the status quo: Your best move for a late-initialized field is to mark it non-null (e.g., if inside null-marked context, don't mark it nullable). This has some good effects:
And some bad effects:
So far so good? So we're currently not requiring tools to have any solution for that "bad effect". But they still can, and some do/will. The important thing is that you don't have to stop using that feature, even if you switch to JSpecify annotations for the rest. It should all work together just fine. In this case, I get that importing from two places isn't happymaking, but would JSpecify really have made anything worse? Please let us know what I've missed or misunderstood here. |
so, what do you suggest should be done here? if I want to use the EE validator I'm going to have to find a way to allow the null to return even though it should never be null in practice. If I use Note: I decided to make can anything give perfect verification? probably not with all of the runtime magic. Does that mean as a Developer I shouldn't have "Standardized" way of saying "magic here"? no, I don't think that works either. // © Copyright 2024 Caleb Cushing
// SPDX-License-Identifier: AGPL-3.0-or-later
package com.xenoterracide.jpa;
import jakarta.persistence.Column;
import jakarta.persistence.Id;
import jakarta.persistence.MappedSuperclass;
import jakarta.persistence.PostLoad;
import jakarta.persistence.PrePersist;
import jakarta.persistence.Transient;
import jakarta.validation.constraints.NotNull;
import java.io.Serial;
import java.io.Serializable;
import java.util.Objects;
import java.util.UUID;
import org.jspecify.annotations.NonNull;
import org.jspecify.annotations.Nullable;
/**
* The type Abstract uuid entity base.
*
* @param <ID> the type parameter
*/
@MappedSuperclass
public abstract class AbstractUuidEntityBase<ID extends AbstractUuidEntityBase.AbstractIdentity>
implements Identifiable<@NonNull ID> {
/**
* Surrogate Identifier.
*/
private @Nullable ID id;
/**
* NO-OP parent constuctor for JPA only.
*/
protected AbstractUuidEntityBase() {}
/**
* Instantiates a new Abstract uuid entity base.
*
* @param id the id
*/
protected AbstractUuidEntityBase(@NonNull ID id) {
this.id = id;
}
@Id
@NotNull
@Override
@SuppressWarnings("NullAway") // shouldn't be null at runtime, makes validator error better
public @NonNull ID getId() {
return this.id;
}
/**
* Sets id.
*
* @apiNote for JPA use only
* @param id the id
*/
void setId(@NonNull ID id) {
this.id = id;
}
@PrePersist
@PostLoad
void ensureId() {
Objects.requireNonNull(this.id, "use static factory method to create new id");
Objects.requireNonNull(this.id.getId(), "use static factory method to create new id");
}
@Override
public final int hashCode() {
return Objects.hashCode(this.id);
}
/**
* That is an {@code instanceof} this concrete class.
*
* @param that the other object
* @return the boolean
* @see <a href="https://www.artima.com/articles/how-to-write-an-equality-method-in-java">
* How to Write an Equality Method in Java
* </a>
*/
protected abstract boolean canEqual(@NonNull AbstractUuidEntityBase<?> that);
@Override
public final boolean equals(@Nullable Object other) {
if (other instanceof AbstractUuidEntityBase<?> that) {
return that.canEqual(this) && Objects.equals(this.id, that.id);
}
return false;
}
/**
* Abstract domain identifier.
*/
@MappedSuperclass
public abstract static class AbstractIdentity implements Serializable {
@Serial
@Transient
private static final long serialVersionUID = 1L;
/**
* The actual database UUID for id.
*/
private @Nullable UUID id;
/**
* NO-OP parent constuctor for JPA only.
*/
protected AbstractIdentity() {}
/**
* Instantiates a new Abstract identity.
*
* @param id the id
*/
protected AbstractIdentity(@NonNull UUID id) {
this.id = id;
}
/**
* Gets id.
*
* @apiNote for JPA use only
* @return the id
*/
@NotNull
@Column(name = "id", updatable = false, nullable = false, unique = true, columnDefinition = "uuid")
@SuppressWarnings("NullAway") // shouldn't be null at runtime, makes validator error better
public @NonNull UUID getId() {
return this.id;
}
/**
* Sets id.
*
* @apiNote for JPA use only
* @param id the id
*/
void setId(@NonNull UUID id) {
this.id = id;
}
@Override
public final int hashCode() {
return Objects.hashCode(this.id);
}
/**
* That is an {@code instanceof} this concrete class.
*
* @param that the other object
* @return the boolean
* @see <a href="https://www.artima.com/articles/how-to-write-an-equality-method-in-java">
* How to Write an Equality Method in Java
* </a>
*/
protected abstract boolean canEqual(@NonNull AbstractIdentity that);
@Override
public final boolean equals(@Nullable Object other) {
if (other instanceof AbstractIdentity that) {
return that.canEqual(this) && Objects.equals(this.id, that.id);
}
return false;
}
@Override
public final String toString() {
return String.valueOf(this.id);
}
}
} |
@xenoterracide You put all your JPA entities in a separate jar/project/module and do not run null analysis on it. Keep the JPA code with packages annotated with Make the no-arg constructors protected or package friendly so that JPA still can construct but you cannot. Then make developer friendly public constructors or static methods on the entities that will initialize the object correctly. As far as I know most null analysis tools like checker only check the code that is being compiled. Sorry for the edits! |
To be honest, since this is something that seems to be something that is quite fundamental/necessary for many things but also possibly a bit controversial in the details, I think there would be significant benefit in finding a common denominator between different nullness annotations/frameworks and clearly specifying the semantics of that. However, I agree that including that in the 1.0 release of jspecify is probably too much to ask for and would probably do more damage than good. |
It would be nice but I consider JSpecify at least for its first version more about libraries communicating nullness and less about correct static analysis. What @xenoterracide can largely be done with Checkerframework but I don't think they will like the sheer amount of complicated annotations they will need to do it (aka JPA as I stated in my comment is the largest offender. In fact I don't really know of many others and given how even Brian Goetz is saying mutable getters/setter POJOs are a dying programming pattern/practice I'm not sure how much effort should really be put into it. That is to support zero arg constructors that don't initialize correctly. What I think would be interesting is roping in the Hibernate maintainers like Gavin King to provide input on where they see the future of JPA like libraries. |
I think there may also be some work on the side of the Eclipse Foundation in allowing records to be used in a future version of Jakarta Persistence. |
Oops, I just realized I may have missed many comments. Rereading will change if necessary. See here's the problem with the better tools. For JPA all of your fields have to be nullable. However you might want your getters to be non-null. Like my example here. The ID is really a bad example, but you could replace just about any other property and it would behave the same way. As far as I can tell if you don't make the fields nullable and you don't instantiate them your static analysis tool is still going to complain that they're nullable, since you aren't setting them until later as far as it's concerned. So unless you can get Jakarta on board with making some serious improvements to EE such that beans don't have to have a no arg constructor... Then we need to have some way of saying hey this initialized. Yes I know I don't have to put in non-null on everything with the jspecify spec but tools don't work like that right now, and I do have nullmarked on my module. I mean I could give you a link to the code if you want to show me how you would make it compile, and run the tests successfully... Without suppress warnings. Unless you're saying that nullaway is so different from the spec as defined that none of the tooling that exist currently matters. |
I can tell you that in hibernate 6 there is some ability to do records for certain things already. At least you can find a tutorial for doing it for an embeddable. However it basically requires that you use their static factory which is... Well that's not the nicest thing in the world. Yes JPA is obviously one of the most pervasive thing in regards to this. Although I really think it's almost all object mapper technologies. Yeah checker framework has some nice annotations it's too bad that they don't do a semantic version of their API because I stopped using it because it breaks nullaway in Gradle. The reason it breaks is because they aren't shading it, and so if you mismatch the versions at all something breaks. I'm concerned about waiting until after 1.0 because what happens if you get into it and you realize that you have to break something in backwards compatibility... Not saying that will happen but this seems like such a common use case (unfortunately) that it doesn't feel like a good idea to postpone it. I'll be all happy if you can convince the EE guys to stop with the mutable bean requirements. I am not confident that JPA is the only violator there, as far as EE specs go. It just so happens that JPA might be the only EE spec that doesn't have an implementer that allows you to do something else. |
I can try to help if you put a link. I don't have as much experience with nullaway but I highly recommend Checkerframework or the Eclipse headless. Both of those two are closer to the spec than nullaway and intellij last time I checked. Regardless the trick that I would do is what I recommended earlier. Separate out the JPA code and have your build not run nullaway (or whatever else) on that code. You may even be able to exclude your JPA code by package but I'm not familiar with how to do that with Checker or Eclipse. |
yeah, I mean not validating code is an option. Mine are already/always separated. In this case I think I decided that As far as checkerframework, nullaway uses it under the hood, and because of that it likes to break nullaway since it doesn't have a stable API, and then classpath hell happens if both are included. I could micro manage both of them, but really someone else needs to persuade them to switch to a semantic versioning scheme (I tried a few years back). Error Prone as a whole does far more than checkerframework. I'm not familiar with eclipse headless... |
Regardless if JSpecify fixes this it will require a special annotation or a compiler flag. Its best to think of the current null annotations as types. For example in the new world order Let us pretend for a second that Java does not have It would probably be a solution of using public class Something {
private Optional<String> field1 = Optional.empty();
private String field2 = "";
private SomeEnum someEnum = SomeEnum.DEFAULT;
// getters and setters omitted.
} For you to do public class Something {
// Some special opt-in annotation
private String field1 = null;
// getter returns field1 as nonnull
} Requires something special. Whether its Its important to understand that JPA and various other CGLIB like runtime altering code including Spring's Java Config and ASpectJ are not really Java. Like you cannot represent what those things do with normal Java code is what I mean. That is they are not only reflection based but byte code generating. |
CGLIB is dead, it doesn't work past LTS Java 11 (or was it 17). Java Config can just use proxies... and reflection, which are only magical up to a point of understanding. Not all of that magic manipulates byte code. One of the problems I've shown here is not actually JPA, but "Validation" spec. I'm not actually certain how |
Depends WYM! Assuming [EDIT: spelling should have been a clue; this did not mean our |
FYI, "ALL we are doing is adding a nullness axis to the type system" was a foundational decision for this project, though a pointer to that discussion would take me time to find. |
Yes but I think that is a lot harder to explain and understand especially for those coming from an That is I don't think most are going to be writing |
Its far from dead especially its parent library of ASM as even the JDK has to use a forked version IIRC (well soon they will have their own byte code generator). I don't know of the latest of Hibernate but I can assure you Spring Java Config still uses CGLIB albeit its own version. You can put a break point on any Spring Boot application ORMs need the above so that they can lazy load on demand and the proxies in the JDK are only for interfaces. |
The note on the cglib homepage says "cglib is unmaintained and does not work well (or possibly at all?) in newer JDKs, particularly JDK17+". cglib depends on ASM, which is a separately project that is actively maintained and supports the latest class file versions. Spring has a fork of cglib that has some patches to improve JDK 17+ support. |
yes I am behind on the current state. It appears byte-buddy is being used by hibernate. Regardless whatever the library it uses it still is generating byte code at runtime (and possibly compile time but I'm not aware of this) otherwise I am not sure how it would work. |
I'm referring to Jakarta Validation
spring too, I think just about everyone has moved to that. I played with it, pretty nice. I built my own magic proxy for something to try to get HTTP PATCH behavior when client DTO's were Asymetric. It generate's classes at runtime, so yeah, byte code. However, I don't see it as relevant one way or another because you're still writing Java, it's just wrapping your code. Technically your Pojo still compiles even if it does more at runtime. Lazy init can be done without black magic byte code manipulation or reflection. Now if we were talking Lombok, that's different, it's not java, javac cannot compile it without a compiler plugin. checker framework does have annotations for this even if the project itself is undesirable. So it's not like no one has reasoned about it before. It's not the only thing that has a solution already, nullaway recognizes any annotation called All I've been trying to point out is that I'm really concerned that at 1.0, which from a semver perspective would be considered a feature complete and api that can't be broken, not having a complete plan for JPA and Validation seems unwise and means that anyone trying to do this can't only use jspecify, they'll still have to do something else. This will likely mean lower adoption rates because, why? it's just as good as any other solution, and probably inferior because it's not a complete solution, the other ones already "work". Sure, their'll be some adoption, but the tooling around it will still be inconsistent and for reasons that can't be argued as a bug. Basically the spec is SQL which doesn't define how to do (database) user permissions, but absolutely ever database needs to do that, so it's different for each of them. I'm done ranting now, I've said my peace. I think I've gotten my point across in a way that people understand where I'm coming from. |
As an exercise I urge you go try to annotate your JPA entities without changing the code with Checker Framework. Then after perhaps propose a solution to solve the JPA issue. I think you will be surprised how complicated it gets.
Just because it is byte code does not mean it is valid Java. That is the JVM is different than the Java language. If you need any further proof there is a language called Kotlin and things like AspectJ. Unless I'm mistaken Hibernate does not decorate but alters the class or at least tricks the runtime that the class can be extended (I can't remember which way it does it). Regardless it is happening at runtime. How is a static analysis tool going to figure out that it is valid? That is you can do: public final class SomeEntity { // notice this is final
SomeEntity() { // notice this is package protected
}
} How would you wrap or proxy
Of course you write the code that does it directly in the class. Code that can be statically analyzed. Just like you can add As for jakarta validation That is the thing is JPA is combining two states. An unvalidated state that you might just fill the object without using any mapper and get an NPE and a in theory validated state because it was loaded from the database and thus the invariants are met (in theory). The above is inherently mutable and nullable. Like if I take your entity not your base entity but some sub type and Entity e = new Entity();
// pass e to some other function
e.getIdOrSomeOtherField().toString(); // without actually writing code to prevent null in the getter it will be an NPE What you want is for static analysis to keep track of your valid state. E.g. unvalid and valid. Perhaps a new annotation you would write in checker new annotations. List<@NotValid Entity> entitiesFromMVC;
List<@Valid Entity> validated = validate(entitiesFromMVC); While the above might be possible with Checker it is by far out of scope for JSpecify. Do you remember Java libraries before we had generics? JPA is like that but with nullability but just like generics you can just suppress the warnings or ignore.
I am not sure of nullaway but at some point it becomes analogous to |
I do hear you. I just think you have a significantly different concept of what our library should be. More of a "one-stop shop". But we're explicitly not attempting a complete solution; we're incrementally building some common ground between the disparate tools. We think most nullness use cases by far will get everything they need from our jar, but some appreciable number will need other things too and we're fine with that. And bear in mind that the crucial need for uniform nullness semantics is for public library APIs, because that's when the parties on either side might use different tools (within a project, at least the project can demand convergence on one tool). And where whole-program analysis isn't even possible. The features you're asking aren't really in support of that. Given the path of this thread I relabeled it as a discussion. I will make sure something is filed about the post-1.0 feature request, but let me try to make sense of the mess of issues that already exist related to this. |
Some situations/frameworks (e.g. JPA) require a no-args constructor for initialization/set the object after the constructor.
However, in many cases, these types are not nullable after initialization.
Is there a way to annotate fields that are null at initialization but where all fields should be initialized and it generally being assumed that these fields are not nullable outside of their initialization?
The text was updated successfully, but these errors were encountered: