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

Further Support Null Analysis #47

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Jan 8, 2024

  • all builder fields will generate with a @nullable annotation (will default to jspecify unless one of the components has a different nullable annotation or nullableAnnotation is set)
  • if any form of @NonNull is detected on a component, the generated build method will validate that the value isn't null
  • adds a flag to enforce the above behavior on all components (unless the component is annotated with @Nullable)
  • adds support for Jspecify @NullMarked and @NullUnmarked
  • generated builds can now implement interfaces via the builderInterfaces property
  • add another annotation to set global generation settings

Resolves #43 (At least for the checker framework)

also has changes from #45

@SentryMan SentryMan self-assigned this Jan 8, 2024
@SentryMan SentryMan added the enhancement New feature or request label Jan 8, 2024
@SentryMan SentryMan added this to the 1.1 milestone Jan 8, 2024
@SentryMan
Copy link
Collaborator Author

Alright, we're now good with both checker framework and nullaway.

@commonquail
Copy link

Yep, Just Works™. Very cool, thank you. :)

@rob-bygrave
Copy link
Contributor

rob-bygrave commented Jan 11, 2024

So, I'm thinking this maybe could work better / more accurately in that with the current enforceNullSafety it pretty much assuming that enforceNullSafety = true the record is in a @NullMarked scope [with the scope based on class then package then module etc annotated with @NullMarked].

That is, if code is using jspecify then there is @NullMarked and @NullUnmarked on the module/package/class to control the scope of "it should be NonNull or Null unspecified by default". This PR is currently working / passing with the assumption that enforceNullSafety = true matches when the record is in a @NullMarked scope.

That is, currently there isn't code in the annotation processor to traverse up the enclosed types (class -> package -> module) looking for a @NullMarked or @NullUnmarked and using that to determine the NonNull'ness of a component of a record (and instead the code just uses the enforceNullSafety flag and assumes that matches what the jspecify scope has defined).

I'm pondering if it would be better if the enforceNullSafety was removed completely and instead if a record component isn't explicitly defined as NonNull or Nullable then:

  • Traverse up to the record type and look for @NullMarked or @NullUnmarked, if neither are present then ...
  • If the record is enclosed in another class traverse up to that class and look for @NullMarked or @NullUnmarked
  • Traverse up to the package and look for @NullMarked or @NullUnmarked, if neither are present then ...
  • Traverse to the module and look for @NullMarked or @NullUnmarked

Nb: Packages are not enclosed by "parent" packages.

Class members are enclosed by classes, which may be enclosed by other class members or classes. and top-level classes are enclosed by packages, which may be enclosed by modules.

@SentryMan
Copy link
Collaborator Author

I'm pondering if it would be better

why not both?

@SentryMan
Copy link
Collaborator Author

We can probably also try to annotation process those nullmark annotations

in any case we can add that as a fast followup, this PR is getting pretty big.

@commonquail
Copy link

commonquail commented Jan 11, 2024

So, I'm thinking this maybe could work better / more accurately [...]

Yeah, that sounds right. Checker Framework presently supports neither annotation and NullAway has only experimental support for them (and the default happens to be acceptable for my use cases) so I have not experimented with those combinations.

why not both?

I suppose because enforceNullSafety runs somewhat counter to @NullMarked and @NullUnmarked, needlessly expanding the surface area and complexifying both the implementation and the usage, compared to an alternate with strict adherence to the annotations (presumably with some default -- does JSpecify suggest a default?). But either way, it is indeed something that can be built on top.

@SentryMan
Copy link
Collaborator Author

SentryMan commented Jan 11, 2024

why not both?

I suppose because...

I'm thinking more along the lines of somebody who doesn't use static null analysis and wants the builders to enforce non-null at runtime anyway. this option would be useful for them.

@SentryMan
Copy link
Collaborator Author

Traverse up to the record type and look for @NullMarked or @NullUnmarked, if neither are present then .

hmm, yeah wasn't that hard to add

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSpecify 0.3.0, Checker Framework nullness, and avaje-record-builder in Maven
3 participants