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

feat: migrate to JSpecify #1019

Merged
merged 2 commits into from
Nov 16, 2024
Merged

Conversation

TheMrMilchmann
Copy link
Collaborator

See #1008 for a detailed description.

Key changes:

  • All occurrences of javax.annotation.Nullable have been replaced with org.jspecify.annotations.Nullable
    • This introduced a conceptual change as nullability effectively becomes a use-site attribute of the type. Where a method previously returned "String or null", it now returns a "nullable String". To reflect this, the location of the marker annotation was (an in a few cases had to be) changed.
    • @Nullable public static Something.Buffer ... becomes public static Something.@Nullable Buffer ...
    • The @Nullable marker now directly precedes the type before the @NativeType annotation where applicable.
  • Likewise, org.lwjgl.system.NonnullDefault has been removed and replaced with org.jspecify.annotations.NullMarked.

This is a draft PR for the following reasons:

  1. We need to decide whether we want to expose JSpecify as an API dependency. JSpecify recommends this, but using them as compile-only dependencies (just like we currently do with JSR305 annotations) would also work fine.
  2. I'm not fully happy with the changes to the generator. If you have a better idea, please let me know.

@@ -36,9 +35,7 @@ final class SharedLibraryLoader {

private static final Lock EXTRACT_PATH_LOCK = new ReentrantLock();

@GuardedBy("EXTRACT_PATH_LOCK")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only use of JSR305 annotations that can't be replaced with JSpecify. We could keep JSR305 just for that but I figured it's not worth it.

@TheMrMilchmann TheMrMilchmann marked this pull request as ready for review November 13, 2024 16:41
@TheMrMilchmann TheMrMilchmann marked this pull request as draft November 13, 2024 16:42
@Spasi Spasi force-pushed the tmm/feat/jspecify branch from 0ff0bd2 to 4fae14d Compare November 16, 2024 16:38
@Spasi Spasi marked this pull request as ready for review November 16, 2024 16:41
@Spasi Spasi merged commit 2607557 into LWJGL:master Nov 16, 2024
@Spasi
Copy link
Member

Spasi commented Nov 16, 2024

Great job @TheMrMilchmann, thanks!

@SWinxy
Copy link
Contributor

SWinxy commented Nov 18, 2024

Is it valid syntax to have a type, the annotation, and then .Buffer?

public static AIBone.@Nullable Buffer createSafe(long address, int capacity) {

@TheMrMilchmann
Copy link
Collaborator Author

Is it valid syntax to have a type, the annotation, and then .Buffer?

public static AIBone.@Nullable Buffer createSafe(long address, int capacity) {

@SWinxy This is not just valid, but the only correct syntax for the TYPE_USE syntax. In simple terms, these annotations must appear next to the concert type name (or []) they apply to. AIBone is just part of the qualifier for its nested Buffer. This is specified in JLS 9.7.4.

Some examples from JSpecify's docs (which apply to all TYPE_USE annotations):

  1. For a nested static type like Map.Entry, if you want to say that the value can be null then the syntax is Map.@Nullable Entry. You can often avoid dealing with this by importing the nested type directly, but in this case import java.util.Map.Entry might be undesirable because Entry is such a common type name.

  2. For an array type, if you want to say that the elements of the array can be null then the syntax is @Nullable String[]. If you want to say that the array itself can be null then the syntax is String @Nullable []. And if both the elements and the array itself can be null, the syntax is @Nullable String @Nullable [].

@SWinxy
Copy link
Contributor

SWinxy commented Nov 18, 2024

Wild! Thanks for the info.

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

Successfully merging this pull request may close these issues.

3 participants