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

Annotate Java Beans property listeners for nullness #123

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

grv87
Copy link

@grv87 grv87 commented Jan 26, 2022

I added @Nullable for several classes in java.beans package.

I'm not sure about propertyName property in IndexedPropertyChangeEvent.
According to the class' javadoc, it can be null.
But:

  • This paragraph is copied verbatim from ancestor PropertyChangeEvent, maybe a typo in JDK
  • I'm not sure how much sense would make an event without propertyName, but with index

Note that this property corresponds to propertyName arg in PropertyChangeSupport#fireIndexedPropertyChange. I added missing @Nullables in firePropertyChange methods, but not in fireIndexedPropertyChange.

@mernst
Copy link
Member

mernst commented Jan 29, 2022

Thank you for these annotations! We appreciate it. Overall they look great.

Your comment about IndexedPropertyChangeEvent seems right to me. You could open an issue in the JDK bug tracker to get Oracle's judgment on the correctness of the documentation.

I have a couple questions:

  • In 3 overloads of VetoableChangeSupport.fireVetoableChange(), what is the justification for @Nullable String propertyName? I don't see that mentioned in the documentation, and I don't see any calls within the JDK that pass null. (I don't have the same question about PropertyChangeSupport.firePropertyChange(), because of its class documentation.)
  • In ChangeListenerMap.set(), the type @Nullable L[] means: "a non-null array of possibly-null L objects". Is that what you meant? I think you meant L @Nullable [], which means: "a possibly-null array of non-null L objects". Please also make the element type of private @MonotonicNonNull Map<@Nullable String, L[]> map; consistent with that.

Finally, a few minor notes:

  • @AnnotatedFor({"nullable"}) should be @AnnotatedFor({"nullness"}). The argument to @AnnotatedFor is a checker name, not an annotation name. I fixed this.
  • It is usually easier for you in the long run to make pull requests from a branch other than master.

@mernst
Copy link
Member

mernst commented Feb 4, 2022

@grv87 A ping regarding the two comments I had on this pull request. Thanks again for the annotations.

@mernst
Copy link
Member

mernst commented Jul 12, 2022

@grv87 Another ping. I would like to get this pull request merged.

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