This logic won't handle cases where @NullMarked appears on some nested classes but not others. But, I think for now, we can just say we only support @NullMarked on the top-level class. Can we add a comment to this effect?

It also needs @NullMarked on the same file, correct? We don't have a way to add this to the package somewhere and have the rest of the traversal be aware of that (that seems more likely than mixed marked/unmarked code in the models, but I am not sure)

Yes, you're correct, @lazaroclapp, we are assuming an explicit @NullMarked on the top-level class in a source file, and that there is only one top-level class per source file. We won't find anything on package-info.java or module-info.java for now. Both these assumptions hold for jspecify/jdk

@akulk022 can you update the comment in light of the above?

Are we otherwise using @AutoValue for something or what's the advantage of it here?

Sorry this is my fault. I thought this was new code and thought that @AutoValue made it a bit cleaner. I didn't realize it had been moved from somewhere else. @lazaroclapp would you like us to undo the @AutoValue-ization of this file?

Never mind, seems we have been using @AutoValue in at least some parts of the codebase, since the beginning, so I am fine having it here too, at least until we can do JDK 17 records.

Not that I care about this anymore, but I suspect Uber would want these copyright headers to be updated for the right years 😉 (e.g. 2024 here and 20[...]-2024 for the files that were altered, I think)

Check the number of args and print a basic usage message? (Not sure if we do that for JarInfer, and I don't think we need a full argument parser here yet, a check on the length of args would suffice for now)

I agree with @yuxincs that overloading the meaning of the JarInfer[...] CLI options for NullAway for this is bad practice, specially because other than the Android SDK, JarInfer no longer uses .astubx but actually modifies the bytecode of jars to add annotations (so this options aren't used with JarInfer except for that Android SDK jar).

It can certainly be a follow up PR, but I'd be in favor of rethinking these CLI flags. Maybe -XepOpt:NullAway:JarInferEnabled is actually something like -XepOpt:NullAway:ScanClassPathForAstubXModels? Is there a better name? A nicer way to specify which .astubx files we want to load without breaking things in our build internally? (I think there is! We only really care about that Android SDK astubx file right now, so it's not like this changes for each target like it used to, cc: @yuxincs )

I am in favor of changing these, but maybe in a follow-up PR. I don't think we need to go as far as keeping the old flag but deprecating it, as long as we document properly in release notes. But we should probably check as to what this breaks (if anything) internally at Uber.

@akulk022 can you open an issue on renaming these flags?

I've created the issue #940

Do we care about star imports or other nonsense or is it fine here to only count either FQN or an import of the FQN?

I think this logic is fine but we should add a comment that we don't consider star imports @akulk022

Why are we using the javax annotations here and not JSpecify? Some limitation with StubxWriter?

No limitation with StubxWriter 🙂, just used it because the check here considers the javax annotation and I probably didn't consider adding an OR and including the jspecify annotation in the condition. I've updated it.

It also needs @NullMarked on the same file, correct? We don't have a way to add this to the package somewhere and have the rest of the traversal be aware of that (that seems more likely than mixed marked/unmarked code in the models, but I am not sure)

This is just the simple name, right? So currently this breaks when dealing with nested/inner classes? i.e. the concatenation below:

            packageName
                + "."
                + parentClassName
                + ":"
                + getMethodReturnTypeString(md)
                [...]

Ends up producing com.example.Bar:String baz() for:

@NullMarked
class Foo {
   public class Bar {
      @Nullable public String baz() {...}
   }
}

correct?

Yes, that is correct. I'll look into making this work.

@akulk022 is this easy to fix? Or should it wait for a follow up?

@msridhar I'm working on a fix since it seems like it should be easy enough.

@akulk022 be sure to add a test for it as well

@msridhar @lazaroclapp I've updated the logic to handle this scenario, I've also added a test for it. Does this work and are there more scenarios I should test?

Note that using simple names is something we inherited from the CF stub format + us not explicitly handling imports, I think. So we might want to change these to FQNs at some point (but probably not in this PR)

Probably better to just make this a String? I think making it a StringBuilder is an unnecessary optimization and makes the code more brittle. You can just overwrite it with new String values and I think it'll be fine