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

What about lazy initialization/setters #431

Open
danthe1st opened this issue Dec 15, 2023 · 27 comments
Open

What about lazy initialization/setters #431

danthe1st opened this issue Dec 15, 2023 · 27 comments
Labels
discussion Something that did or will resolve itself without any actual change needed nullness For issues specific to nullness analysis.

Comments

@danthe1st
Copy link

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?

@cpovirk
Copy link
Collaborator

cpovirk commented Dec 15, 2023

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 "null is not assignable to a field of type @NonNull String," but we don't define rules like "If a non-@Nullable field is not definitely assigned at the end of a constructor, that's an error."

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 @InitializedAfterConstruction annotation. But we'd be happy to see tools layer on their own such annotations if they find them useful. Maybe someday we'd even look into adding one to JSpecify.

That still leaves the obvious question unanswered: "So, if you don't initialize a field during the constructor, does it have to be @Nullable?" I think the expectation of many of us is that many people will declare these fields without @Nullable. Some other people will use tools that don't like that, so they will use @Nullable and then, for convenience, maybe declare a private getter method with a non-nullable return type and a runtime null check. Typically, you can get by with either approach because the stakes are typically low: Fields are normally private and, in many cases, they're initialized immediately after construction or at least before anyone "does anything" with the object.

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 null to a non-nullable field (like when your object supports a "close" operation)? Is it "wrong" to read null from a non-nullable field? More generally, how much "cheating" is advisable when declaring types? There is a lot of wiggle room here, but it mostly doesn't interfere with the core JSpecify mission of providing a way to declare types at all.

@agentgt
Copy link

agentgt commented Dec 18, 2023

Some situations/frameworks (e.g. JPA) require a no-args constructor for initialization/set the object after the constructor.

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 @Nullable String is different from String and as a corollary embrace immutable objects more.

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 record (and pattern matching) and actually using something that analyzes null I think will change how a lot of people code. When I added null checking to my code it made me code differently and I suspect the same will happen to library authors.

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 @InitializeAfterConstruction is @MonotonicNonNull (I think).

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 null has a strong relationship to pattern matching. Null is not evil nor should be avoided because of NPE. null is bad because people are not semi-forced to dispatch on it. I think when more people start using pattern matching they will see this more.

Consequently it is important to get JSpecify released with as minimal annotations to see and measure how it will change developers and code.

@kevinb9n
Copy link
Collaborator

kevinb9n commented Jan 2, 2024

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.

@kevinb9n kevinb9n added the design An issue that is resolved by making a decision, about whether and how something should work. label Jan 31, 2024
@xenoterracide
Copy link

xenoterracide commented Mar 7, 2024

At least for now, that's not going to include, say, a @InitializedAfterConstruction annotation. But we'd be happy to see tools layer on their own such annotations if they find them useful. Maybe someday we'd even look into adding one to JSpecify.

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;
  }
}

@cpovirk
Copy link
Collaborator

cpovirk commented Mar 7, 2024

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:

  • A big goal of ours is to let owners of common libraries annotate those libraries for the sake of their users. For the most part, users of a library don't have to worry about the initialization story of the library's classes. So those users can still get what they need from pure JSpecify annotations. (The library's owners may want to use additional annotations specific to any checkers that that those owners use. But those owners need only serve those tools, not every tool that their users might use. In this respect, we can accept not being a complete solution.)
  • Handling of initialization differs significantly across tools. (Notably, Kotlin, NullAway, and the Checker Framework all handle it differently.) While JSpecify does make some attempts to break new ground in limited ways (e.g., @NullMarked, which is basically "things work like Kotlin" but which doesn't have a precise existing Java equivalent), we are mostly trying to standardize a core that various tools can agree on, at least for now.
  • I also expect us to benefit from waiting as we see how Valhalla continues to play out. It does seem that Valhalla's value types will provide features like "This field can never be null, and if you read from it uninitialized, you'll get a default value instead." As we learn more about what Valhalla will do, we'll have a better idea of how to make JSpecify and Valhalla integrate well.

@xenoterracide
Copy link

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.

@kevinb9n
Copy link
Collaborator

kevinb9n commented Mar 8, 2024

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:

  • When the field is read properly (post-init), it does the right thing (it's a non-null expression)
  • If you try to initialize the field to a nullable expression, you get a warning, which is probably also what you want

And some bad effects:

  • If the field is read improperly (pre-init), nothing is helping you catch that bug

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.

@xenoterracide
Copy link

xenoterracide commented Mar 12, 2024

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 Objects.requireNonNull validator itself throws an error. I might of course suggest that validator catch an NPE on a NotNull and do the same thing as if it had gotten null; I'm not certain I'm the right person to bring that to them. Although, I might also suggest to myself that validator is never going to be called on this... I need to figure that out.

Note: I decided to make id @Nullable because someone, ;) , decided use getId() in equals/hashcode and at the time I was using requireNonNull and so that was blowing up using equalsverifier.

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);
    }
  }
}

@agentgt
Copy link

agentgt commented Mar 12, 2024

@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 @NullMarked and continue to annotate where you expect things to be actually nullable.

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!

@danthe1st
Copy link
Author

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.
Also, I think this is a topic where people from different nullness frameworks should be invited to collaborate/discuss/find a said common denominator that can be specified.

@agentgt
Copy link

agentgt commented Mar 12, 2024

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 @MontonicNonNull).

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.

@danthe1st
Copy link
Author

danthe1st commented Mar 12, 2024

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.

@xenoterracide
Copy link

xenoterracide commented Mar 12, 2024

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.

@xenoterracide
Copy link

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.

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.

@agentgt
Copy link

agentgt commented Mar 13, 2024

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 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.

@xenoterracide
Copy link

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 SuppressWarnings("NullAway") on the getter's is the right solution. However, and the reason I'm commenting on this issue, is I'd prefer a better plan exist before 1.0.

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...

@agentgt
Copy link

agentgt commented Mar 13, 2024

yeah, I mean not validating code is an option. Mine are already/always separated. In this case I think I decided that SuppressWarnings("NullAway") on the getter's is the right solution. However, and the reason I'm commenting on this issue, is I'd prefer a better plan exist before 1.0.

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 @Nullable String is a different type than String.

Let us pretend for a second that Java does not have null how would you represent your uninitialized POJO using types?

It would probably be a solution of using Optional and or sensible defaults.

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 @SuppressWarnings @NonNullAfterInitialization or a new JSpecify annotation you have to do something different. Something not really canonical in a nonnull world. Something that will unlikely be added if the JDK offers null checking other than compiler flags (which is the same as separating out your code and not running null analysis on it).

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.

@xenoterracide
Copy link

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 @NotNull Optional<String> is handled, does it actually DWIM? using Optional here might actually be a better solution, I don't know why I'm not thinking of that.

@kevinb9n
Copy link
Collaborator

kevinb9n commented Mar 13, 2024

I'm not actually certain how @NotNull Optional<String> is handled, does it actually DWIM?

Depends WYM! Assuming @NullMarked context, both Optional<String> and String are treated as non-null (which is good in the latter case, since it's required to be). @NonNull in that location would be superfluous.

[EDIT: spelling should have been a clue; this did not mean our @NonNull annotation, sorry]

@kevinb9n
Copy link
Collaborator

Its best to think of the current null annotations as types.

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.

@agentgt
Copy link

agentgt commented Mar 13, 2024

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 Optional<...> mindset. The only real difference I think for many (over optional) will be understanding that NonNull extends Nullable.

That is I don't think most are going to be writing <T extends @Nullable Object> on their first pass of trying to do null analysis.

@agentgt
Copy link

agentgt commented Mar 13, 2024

@xenoterracide

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.

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 @Configurable / @Bean on startup and clearly see it. Although not Hibernate I'm fairly sure EBeans (a hibernate competitor that is not JPA) does as well.

ORMs need the above so that they can lazy load on demand and the proxies in the JDK are only for interfaces.

@cushon
Copy link
Collaborator

cushon commented Mar 13, 2024

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.

@agentgt
Copy link

agentgt commented Mar 13, 2024

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.

@xenoterracide
Copy link

I'm not actually certain how @NotNull Optional is handled, does it actually DWIM?

Depends WYM! Assuming @NullMarked context, both Optional and String are treated as non-null (which is good in the latter case, since it's required to be). @nonnull in that location would be superfluous.

I'm referring to Jakarta Validation NotNull on an Optional... I plan on testing it out, but I have to build something else first.

yes I am behind on the current state. It appears byte-buddy is being used by hibernate.

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 Initializer

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.

@agentgt
Copy link

agentgt commented Mar 13, 2024

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.

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.

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 SomeEntity without violating the Java language? Sure you can produce the bytecode but how would you do it with Java?

Lazy init can be done without black magic byte code manipulation or reflection.

Of course you write the code that does it directly in the class. Code that can be statically analyzed. Just like you can add Objects.requireNonNull on ever getter.

As for jakarta validation @NotNull it is inherently @Nullable. Otherwise how would the field ever be null?

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.

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 Initializer

I am not sure of nullaway but at some point it becomes analogous to @SuppressWarnings. You have taken off the safety net just like casting or removing generic parameters.

@kevinb9n kevinb9n added nullness For issues specific to nullness analysis. discussion Something that did or will resolve itself without any actual change needed and removed design An issue that is resolved by making a decision, about whether and how something should work. labels Mar 14, 2024
@kevinb9n
Copy link
Collaborator

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Something that did or will resolve itself without any actual change needed nullness For issues specific to nullness analysis.
Projects
None yet
Development

No branches or pull requests

6 participants