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

Split "paradox" into 3 categories. #135

Merged
merged 5 commits into from
Oct 2, 2020
Merged

Conversation

cpovirk
Copy link
Collaborator

@cpovirk cpovirk commented Sep 29, 2020

No description provided.

@googlebot googlebot added the cla: yes See https://cla.developers.google.com/ label Sep 29, 2020
@cpovirk
Copy link
Collaborator Author

cpovirk commented Sep 29, 2020

I had a realization about the new categories: Only one is purely "about" nullness:

  • @Nullable int i

The others are more general:

Based on that, I have tentatively named those 2 categories "jspecify_multiple_annotations" and "jspecify_unrecognized_location," without putting "nullness" in the name as I've been doing for the others (including new "jspecify_nullness_intrinsically_not_nullable"). @kevin1e100 , I think you may have been the one to suggest including "nullness" in the name of the initial tokens. Does omitting it from those 2 categories make sense to you? (Not that I want to overthink this :))

Once there is agreement on the names, I can update the sample inputs themselves accordingly.


- `jspecify_nullness_paradox`: for case 1 above
- `jspecify_multiple_annotations`: for cases like `@Nullable
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be "conflicting". When Jspecify expands, it may permit multiple annotations at a single location, such as int @NonNull @NonEmpty [].

Copy link
Collaborator Author

@cpovirk cpovirk Sep 30, 2020

Choose a reason for hiding this comment

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

Makes sense. Done.

(Mike and I had talked elsewhere about using the term "inconsistent." We set it aside because readers might also conclude that an annotation can be "inconsistent" with a corresponding annotation on a supermethod, for example. While we could have the same concern about "conflicting," I feel that it's clearer somehow. Perhaps it's more natural to assume that a "conflict" occurs between two things that are sitting right next to one another in the source file, whereas "inconsistency" is the kind of thing that you might see even between 2 completely separate codebases.)

I'll leave this PR open for a couple days to collect feedback from @kevin1e100 and other interested parties. After that, I'll make the changes to the sample inputs themselves and merge the PR. As always, we can revisit as needed.

Copy link
Contributor

@mernst mernst left a comment

Choose a reason for hiding this comment

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

This looks good, thanks.

@cpovirk cpovirk merged commit 2a6f618 into jspecify:main Oct 2, 2020
@cpovirk cpovirk deleted the resolve-paradox branch October 2, 2020 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes See https://cla.developers.google.com/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants