-
Notifications
You must be signed in to change notification settings - Fork 27
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
Decide retention for our annotations #28
Comments
I'm not really aware of a reason why we would ever regret using runtime retention. And whenever we don't, we will sooner or later be asked to. |
For the Checker Framework, we provided a separate By default, the annotations are RUNTIME retention https://checkerframework.org/manual/#faq-runtime-retention |
I think the only thing that makes sense is for us to make everything runtime retention. When users have performance-based reasons for wanting to turn that off, they can deal with that in their toolchain; I don't know if it's even any of our concern how they do. |
I believe we consider this settled for now. No-runtime-retention versions of our annotations can be hacked up as a secondary matter. |
(We now have #96 to consider a special case for nullness. It hasn't seen much activity, but I wanted to cross-link these 2 issues.) |
(Just to pile on: I was just reminded that Guice is an example of a library that requires runtime retention for nullness annotations. But that said: It also ignores type annotations entirely! So that's something that will affect us inside Google, at least.) |
I saw a concern internally about runtime retention on Android: Under older versions of Android, a runtime-visible annotation must appear in the same dex as all classes that use it. For widely-used nullness annotations, this would force a huge number of classes to appear in a single dex, exceeding the dex limit.
It may be that the solution to this is going to be to still use |
Hmm, or does the problem arise only with annotations that have an |
Ah: I think the problem with old versions of Android was indeed specific to annotations with an Perhaps the workaround could be improved, but we'd need to look into that. |
But wait, no? ...except... our "in the club" annotation will be a declaration annotation. It will cause the same problem. (I was starting to wonder if the "in the club" annotation could mostly be placed in |
Note the combination of type and runtime-retention declaration annotations is somewhat scary for Android also (regardless of Android version): |
Thanks. I recall your pointing that out in #96, and we should definitely have all these points in front of us. I'm finding myself rapidly convinced that we need a type-use, runtime-retention Nullable (for Guice) and a declaration, class-retention InTheClub (for multidex). That handily solves the problem you've raised, too. So at the very least, I'm inclined to try that internally to see if it shakes out any other fun surprises. We can presumably still define aliases with different retentions (or users can do it themselves), but those are feeling like the least dangerous defaults. |
(Granted, this creates the reverse problem in the JRE: You can see Nullable but not InTheClub. But I'm guessing that much less code cares about InTheClub, aside from maybe a testing library or bytecode rewriter that adds runtime null checks or something. (And those could reasonably extract InTheClub from bytecode.)) |
See #28 (comment) and nearby comments. This is not a final decision, either. However, since we've already discovered a problem with `RUNTIME` in practice, I'd like to take the opportunity to try to identify any problems with `CLASS` in practice. That should help us make an informed final decision.
I'm not really sure I believe that, though. Maybe I just believe that looking for An example other concern around runtime use of Again, though, I'm not putting much stock in any of this. [edit: Part of the reason I'm not too worried about this: It's not too different from the normal confusion that could arise when |
We need |
Thanks. As discussed in today's meeting, we will need to do at least one of the following before our 1.0 release:
|
FWIW I don't know that legacy multidex is at this point a big enough
problem to worry about, not least considering that (1) it does work, (2) a
tighter main dex list algorithm would seemingly avoid the issue, and
meanwhile (3) using the annotation on entire packages should minimize the
issue (assuming that works on Android).
This brings me to a different concern, however: annotations on modules may
not be runtime-visible on Android (since d8 assumably just ignores
module-info files). Conceivably that's something the build system could
warn about (since it's potentially a concern for any runtime-visible
annotation in a module-info file) or even automatically handle (by spraying
the annotation into affected packages). And here too I'm not sure it's a
big enough concern for us considering that this would only come up when
someone tries runtime reflection for this annotation on Android. But maybe
worth filing a bug for.
|
If we put a runtime-retention annotation on every generated protobuf class (which I think we'll have to do, since those classes may share a package with other code), that's enough to hit the dex limit in at least one Google app. So I fear we need to do "something." (For my reference: Native multidex arrived in API Level 21. Android devices with a lower API Level likely represent <5% of devices now, though I haven't found up-to-date numbers.) Among the possibilities:
(RE: annotations on modules: Given that our type annotations are going to be entirely invisible, as you've pointed out, I suspect that we don't need to worry too much about how visible our declaration annotations are. And in fact we may prefer them not to be visible, as we've noted :) I grant that a mixture of visible and invisible (depending on whether they appear on modules or not) is worse than either option, but it feels like a small concern, relatively speaking.) |
I filed https://issuetracker.google.com/issues/179271759 to ask if Android could be less aggressive in putting things in the main dex. I could imagine that this is not a high priority for them, but we'll see. |
We converted the issue I filed into a Google-internal one so that we could share information about the specific failures I was seeing. But I have an update. I've been informed that our internal tools (and probably external tools, too) are nowadays building the main-dex list with R8 instead of the dx code I was looking at. And the R8 code is smart enough to apply its workaround only for runtime-retention annotations that actually have enum elements. That sounds like it should mean that we can safely use runtime retention (again, as long as we avoid enum elements, which we want to do, anyway). However, there's still the mystery of why I saw an actual "Too many classes listed in main dex list file" failure when I attempted a widely used runtime-retention annotation. The R8 code's enum check is clearly working in general, but either it's failing in this specific case, or something else is going wrong. |
Motivation: 1. sdeleuze points out that Spring needs `RUNTIME` retention: #28 (comment) 2. The problem I'd seen with `RUNTIME` retention is more limited than I'd initially feared. The downside of that is that it's still not clear when the problem actually does arise. Given that, we're probably best served by using `RUNTIME` retention for a while and trying to identify any commonalities from failures that we see: #28 (comment) This reverts commit 4fc89fd.
It feels like the only good solution here would have to involve runtime-retention-stripping out of our official jar as some sort of proguard-like build step. Is that feasible? Is there really an alternative? And, would we feel responsible for providing that solution? But one way or the other, I just can't see justifying making the core artifact class-retention-only. |
At this point I'm exceedingly comfortable with the decision that annotations in our core artifact have runtime retention. If the need for an altered form of the core artifact with the retention reduced (or tool that does that munging in a build) becomes compelling it can be filed separately. |
One thing that's convenient (which we've already said a bit about above): One of the platforms most likely to care about code size, Android, already leaves type-use annotations out of the final binary. So, if we can make it practical for most people to use type-use annotations, then there's no need for Android apps to fear runtime retention for those annotations, in contrast to what some do now:
That still leaves the possibility that |
The current, working decision, as of 2021-12-08, is that JSpecify's annotations will have runtime retention, since some reflection-based tools, like Guice, require runtime retention for annotations. The argument for changing this decision is that some platforms, like Android, are sensitive to code size and don't require runtime retention, so we should not impose those costs. However, platforms or applications that do not want the code size cost of runtime-retained annotations can use tools to strip them out of binary code before deployment. We shouldn't preclude reflection-based tools from using nullness information. This must be decided before version 1.0 of the jar. I propose the current decision be finalized for 1.0. If you agree, please add a thumbs-up emoji (👍) to this comment. If you disagree, please add a thumbs-down emoji (👎) to this comment and briefly explain your disagreement. Please only add a thumbs-down if you feel you can make a strong case why this decision will be materially worse for users or tool providers than an alternative. Results: Eight 👍; no other votemoji. |
Decision: JSpecify's annotations will have runtime retention. |
No description provided.
The text was updated successfully, but these errors were encountered: