Skip to content

Annotate CodeSource.#131

Merged
cpovirk merged 1 commit intomainfrom
codesource
Feb 10, 2026
Merged

Annotate CodeSource.#131
cpovirk merged 1 commit intomainfrom
codesource

Conversation

@cpovirk
Copy link
Copy Markdown
Collaborator

@cpovirk cpovirk commented Jan 7, 2026

No description provided.

@cpovirk cpovirk requested a review from wmdietl January 7, 2026 19:21
Copy link
Copy Markdown
Collaborator

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

One point of discussion, all other additions are consistent with javadoc.

* codesource, {@code false} if not.
*/
public boolean implies(CodeSource codesource)
public boolean implies(@Nullable CodeSource codesource)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The first condition says <li> <i>codesource</i> must not be null. also the @param doesn't mention null.
The implementation clearly returns false if the parameter is null, which is consistent with the If any fail, it returns {@code false}. in the javadoc.

This looks like another instance of our discussions about how to annotate methods where null is forbidden, but no NPE is thrown.
I'm fine with making it Nullable, if that is more convenient for the uses of the method that you see.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks.

I found this Javadoc a bit vexing. Normally, I read "must" as expressing a precondition in the sense of "This method will throw (or at 'best' have undefined behavior) if the precondition is not met." But the way the Javadoc here is phrased, it seems to be consistently using the word "must" to express "The method returns true if all these 'musts' are met and false otherwise." Hence we have "This object's protocol... must be equal to codesource's protocol" in parallel to "codesource's location must not be null." But then, just when it looks like everything is phrased consistently, we have a bullet that suggests that the whole list should have been expressed in pseudocode: "If this object's location equals codesource's location, then return true."

Given that, I do feel OK with this annotation. I should record, though, that don't think it was prompted by a specific need in our code: I think that we needed CodeSource(URL url, Certificate[] certs) to have a @Nullable on that second parameter, and I decided to go for the whole file. (It's possible that I was also confused and thought that we needed a bit more because I was mixing up a null passed to CodeSource and a call passed to ProtectionDomain in the same statement :)) Let me punt things back to you in case my confession leaves you reluctant here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the detailed analysis. I'm fine with keeping this annotation. If anyone has an issue, they'll see this discussion and can argue against it.

@wmdietl wmdietl assigned cpovirk and unassigned wmdietl Feb 9, 2026
@cpovirk cpovirk requested a review from wmdietl February 9, 2026 22:04
@cpovirk cpovirk assigned wmdietl and unassigned cpovirk Feb 9, 2026
@wmdietl wmdietl assigned cpovirk and unassigned wmdietl Feb 10, 2026
@cpovirk cpovirk merged commit 2c0c130 into main Feb 10, 2026
21 checks passed
@cpovirk cpovirk deleted the codesource branch February 10, 2026 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants