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

consider making all packages considered annotated by default #1081

Open
xenoterracide opened this issue Dec 5, 2024 · 9 comments
Open

consider making all packages considered annotated by default #1081

xenoterracide opened this issue Dec 5, 2024 · 9 comments

Comments

@xenoterracide
Copy link

xenoterracide commented Dec 5, 2024

I just realized that I wasn't getting checks from 3rd party jars that are annotated. Then I realized that it's going to be a massive pain to develop a comprehensive list of annotated packages from 3rd party jars.

Whilst using UnannotatedSubPackages should work, I think it would be better if all packages were considered "null marked" by default, unless they are annotated by something that says they aren't. I might be wrong in this attitude and maybe I should bug "jgit" to add a recognized (are there any)? annotation to say its packages are marked.

@xenoterracide
Copy link
Author

I'm dumb and should read the wiki

@xenoterracide xenoterracide closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2024
@msridhar
Copy link
Collaborator

msridhar commented Dec 5, 2024

Actually it's not fully obvious to me the best way to do this. Are you making your annotated packages like com, org, etc? And then excluding sub-packages?

@xenoterracide xenoterracide changed the title Add an UnannotatedPackages instead consider making all packages considered annotated by default Dec 5, 2024
@xenoterracide xenoterracide reopened this Dec 5, 2024
@xenoterracide
Copy link
Author

hahah, you're fast.

I am now. I guess the best way is to use the UnannotatedSubPackages so, for example I'd make it those, but now, for example while I have io I'd add UnannotatedSubPackages io.vavr testing to see if this works.

I'm changing the description of the original ticket to consider making "all packages" considered NullMarked default, perhaps with a flag to turn that off, if it's too much. Or perhaps simply setting AnnotatedPackages would. I'd rather have the negative list and go bug people.

@msridhar
Copy link
Collaborator

msridhar commented Dec 5, 2024

Let me think through the implications of this and how best to do it. Optimistically, if more and more jars start becoming annotated, this might be a desired default more and more often.

Another option, if you're willing to bug people 🙂 If there is a third party library that is annotated, they should be able to add a package-info.java to each of their packages with a @NullMarked annotation, and NullAway should pick that up. Note that this is not hierarchical; if your library has packages com.foo.bar1 and com.foo.bar2, you can't just add a package-info.java in com.foo; you need to add one in each individual package.

@xenoterracide
Copy link
Author

xenoterracide commented Dec 5, 2024

does it pick up a module-info with NullMarked yet? which would be hierarchical ("all packages in module"). Also, is the presence of the spring annotations like NonNullApi/Fields (I forget if they have a nullable) do similar as nullmarked? meaning the package is then considered NullMarked

@xenoterracide
Copy link
Author

I think I like the idea that if no AnnotatedPackages is set, then all packages that aren't marked as unmarked either by the UnannotatedSubPackages or @NullUnmarked on the package, or module is treated as marked. This would be backwards compatible as of right now it should be set.

@msridhar
Copy link
Collaborator

msridhar commented Dec 5, 2024

does it pick up a module-info with NullMarked yet? which would be hierarchical ("all packages in module").

I want this to work and would add support for it on the NullAway side (it's not there yet). But it's a bit tricky to get it working exactly as users want, as discussed in jspecify/jspecify#496.

Also, is the presence of the spring annotations like NonNullApi/Fields (I forget if they have a nullable) do similar as nullmarked? meaning the package is then considered NullMarked

There is no support for this yet; I could look into it but not sure if we would support it. Should probably open a separate issue.

I think I like the idea that if no AnnotatedPackages is set, then all packages that aren't marked as unmarked either by the UnannotatedSubPackages or @NullUnmarked on the package, or module is treated as marked. This would be backwards compatible as of right now it should be set.

There is discussion related to this in #574. I am leery of NullAway just chugging ahead if no AnnotatedPackages option is set; I think for new users the behavior will be unexpected. Probably we'd want a new option to opt in to this behavior.

@xenoterracide
Copy link
Author

xenoterracide commented Dec 6, 2024

There is discussion related to this in #574. I am leery of NullAway just chugging ahead if no AnnotatedPackages option is set; I think for new users the behavior will be unexpected. Probably we'd want a new option to opt in to this behavior.

I'm kind of the opposite. I'm not a new user. I always assumed (for no logical reason) that it'd figure jars on my classpath out. I don't think new users (based on me) would ever consider that libraries aren't being picked up. This might be solved by improved documentation, or perhaps a more aggressive strategy in a different way, perhaps requiring packages to be explicitly configured in general. So if I set com.xenoterracide but org.jgit is not configured, it would complain that org.jgit has no configuration detected.

chugging ahead if no AnnotatedPackages option is set

thinking about it as above, that potentially solves another issue that I hadn't thought of. What if all packages are already accounted for. My code is all Nullmarked via module-info. I have a small library that has no 3rd party dependencies. Should I have to set AnnotatedPackages, doesn't feel like I should (at least once that support is there). note #1083

There is no support for this yet; I could look into it but not sure if we would support it. Should probably open a separate issue.

will do, this pattern is also becoming increasingly common, due to spring doing it. Gradle uses this pattern too, IIRC. #1084

Maybe the right way forward for this ticket is if you can't be "smarter" be "louder" :p

@msridhar
Copy link
Collaborator

Sorry for the slow response. I think what you're suggesting is a mode where every method / class / package / module must be either explicitly @NullMarked or @NullUnmarked, via annotations or AnnotatedPackages. Does that sound right? It seems like that could be useful, though I think I have some higher-priority tasks to work on before this one, and also I'd want to implement it carefully to not impact performance (we check for markedness of code very frequently). I am open to a PR here, but fair warning it may be rather subtle to get it right.

Thanks for opening the other issues!

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

No branches or pull requests

2 participants