Better documentation and tests for initializer support

[xenoterracide]

not sure how to fix this

> Task :cli:config:compileJava FAILED
/home/xeno/IdeaProjects/brix/modules/cli/config/src/main/java/com/xenoterracide/cli/config/CliCommand.java:32: error: [NullAway] initializer method does not guarantee @NonNull fields language (line 22), moduleType (line 24), project (line 26) are initialized along all control-flow paths (remember to check for exceptions or early returns).
  CliCommand( Dispatcher dispatcher ) {
  ^
    (see http://t.uber.com/nullaway )
1 error

I've tried adding this via the ep plugin

    option("NullAway:AnnotatedPackages", "com.xenoterracide")
    option("NullAway:ExternalInitAnnotations", "picocli.CommandLine.Option")
    option("NullAway:CustomInitializerAnnotations", "picocli.CommandLine.Option")

package com.xenoterracide.cli.config;

import com.xenoterracide.brix.cli.api.CliConfiguration;
import com.xenoterracide.brix.dispatch.Dispatcher;
import org.apache.commons.lang3.builder.ToStringBuilder;
import org.apache.commons.lang3.builder.ToStringStyle;
import org.checkerframework.checker.nullness.qual.MonotonicNonNull;
import org.springframework.stereotype.Component;
import picocli.CommandLine;

import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Optional;

@Component
public class CliCommand implements CliConfiguration, Runnable {

  private final Dispatcher dispatcher;

  private Path workdir = Paths.get( "" );

  private String language;

  private String moduleType;

  private String project;

  private @MonotonicNonNull String name;

  private @MonotonicNonNull Path repo;

  CliCommand( Dispatcher dispatcher ) {
    this.dispatcher = dispatcher;
  }

  @Override
  public String toString() {
    return ToStringBuilder.reflectionToString( this, ToStringStyle.MULTI_LINE_STYLE );
  }

  @Override
  public Optional<Path> getRepo() {
    return Optional.ofNullable( repo );
  }

  @CommandLine.Option(
    names = {"--repo"},
    description = "Repository path from the current working directory. " +
      "Templates and configs are looked up relative to here. If the config " +
      "isn't found here, then we will search ~/.config/brix"
  )
  public void setRepo( Path repo ) {
    this.repo = repo;
  }

  @Override
  public Path getWorkdir() {
    return workdir;
  }

  @CommandLine.Option(
    names = {"--workdir"},
    defaultValue = "",
    showDefaultValue = CommandLine.Help.Visibility.ALWAYS,
    description = "The working directory you want your destination paths to be relative to." +
      " Defaults to current working directory"
  )
  public void setWorkdir( Path workdir ) {
    this.workdir = workdir;
  }

  @Override
  public String getProject() {
    return project;
  }

  @CommandLine.Parameters(
    index = "2",
    description = "The name of the project you're generating code for."
  )
  public void setProject( String project ) {
    this.project = project;
  }

  @Override
  public String getLanguage() {
    return language;
  }

  @Override
  public String getModuleType() {
    return moduleType;
  }

  @CommandLine.Parameters(
    index = "1",
    description = "The type of code you're generating e.g controller, also the name of the config" +
      " file without the extension."
  )
  public void setModuleType( String moduleType ) {
    this.moduleType = moduleType;
  }

  @Override
  public @MonotonicNonNull String getName() {
    return name;
  }

  @CommandLine.Parameters(
    index = "3",
    description = "The name of the module to be created within the project.",
    arity = "0"
  )
  public void setName( String name ) {
    this.name = name;
  }

  @CommandLine.Parameters(
    index = "0",
    description = "The programming language you're generating code for. Directory under --dir"
  )
  public void setLanguage( String language ) {
    this.language = language;
  }

  @Override
  public void run() {
    dispatcher.run();
  }
}

[lazaroclapp]

NullAway is saying that the class constructor (the only initializer I see there) is not setting these three fields to anything:

language (line 22), moduleType (line 24), project (line 26)

Since the fields are not marked @Nullable, they should be set during object construction, otherwise they are null at that point, which is not safe.

[xenoterracide]

yes, I get that, but since they're set (more or less) by framework construction before use.. I'm trying to figure out how best to tell nullaway that they're being initialized. I would have thought that those options would have fixed it.

[lazaroclapp]

Oh, right, sorry. I saw this fairly late last night and I missed the fact that you were setting picocli.CommandLine.Option to be an @Initializer annotation.

Note that, for the three fields you are being asked about initialization, none of their setters are annotated with picocli.CommandLine.Option, but are instead annotated with @CommandLine.Parameters. That said, even fixing that, I am not sure those options will help.

This one almost certainly won't help:

option("NullAway:ExternalInitAnnotations", "picocli.CommandLine.Parameters")

Because external initialization annotations are only really expected at the class level and are used to indicate that the entire class is being initialized by something outside the code. I don't believe the annotations in this list are even looked at for methods.

As for this one:

option("NullAway:CustomInitializerAnnotations", "picocli.CommandLine.Parameters")

I mean, I'd give it a try, but I wouldn't expect it to work. The problem here is that, as far as I know, NullAway assumes that there is a single initializer. It expects that calling the constructor plus the initializer will fully initialize every field in the class. I am not exactly sure right now what happens at the implementation level when there are multiple @Initializer methods, but I doubt the logic is built with that case in mind.

It's also not obvious what multiple initializers means. Does it mean that all of them should be called before the class is used? (As in this case) Or does it mean that any of them could be called? (Similar to constructors, and to me it seems like a more likely case, where the framework picks one version of the initializer method)

This feels like a case where, to handle this in a fully precise manner, we would need to build:

• Some flexible hooks into the initialization logic that could be overridden by a Handler
• A Handler specific to picocli.CommandLine that encodes the logic those annotations use to initialize the class

That said, a simpler option, in my opinion, is to make the fields of that class @Nullable (no need to make the getters return @Nullable, you could stick a Preconditions.checkNotNull(...) right before the return there and defer checking to that point). This might not feel entirely satisfactory, but if you were writing your own driver class that called each of those setters in turn and then returned the object, that's what you would need to mark the fields as anyways, since they aren't initialized "before other methods can be called on the object" (namely all the setters).

I'd consider an external driver that is calling multiple methods of the object to be beyond the scope of initialization checking. For an example of why: what happens right now if you dereference name within setLanguage(...) or language within setName(...)? For which methods exactly do you expect NullAway to consider those fields "already initialized" vs "potentially uninitialized"?

Alternatively, instead of marking the fields @Nullable, you can stick @SuppressWarnings("NullAway.Init") on the class level here, and just forego initialization checking altogether. You would still get errors if you tried to assign null directly to any of those fields or return null, but nothing about missing a setter or an annotation, unfortunately.

[msridhar]

I mean, I'd give it a try, but I wouldn't expect it to work. The problem here is that, as far as I know, NullAway assumes that there is a single initializer. It expects that calling the constructor plus the initializer will fully initialize every field in the class. I am not exactly sure right now what happens at the implementation level when there are multiple @Initializer methods, but I doubt the logic is built with that case in mind.

It's also not obvious what multiple initializers means. Does it mean that all of them should be called before the class is used? (As in this case) Or does it mean that any of them could be called? (Similar to constructors, and to me it seems like a more likely case, where the framework picks one version of the initializer method)

My memory was fuzzy, but it turns out that based on this code, we do support multiple @Initializer methods:

https://github.com/uber/NullAway/blob/f8e9cbe4f6471330873c4c27e240c12fef92ddd1/nullaway/src/main/java/com/uber/nullaway/NullAway.java#L1493

Basically, the meaning of having multiple @Initializer methods is that all of them should be called before the class is used. I honestly don't remember why we chose to do things this way, but I presume there was a good reason 🙂

So, this suggestion from @lazaroclapp should work I think:

option("NullAway:CustomInitializerAnnotations", "picocli.CommandLine.Parameters")

@xenoterracide can you try and let us know?

[lazaroclapp]

Basically, the meaning of having multiple @Initializer methods is that all of them should be called before the class is used. I honestly don't remember why we chose to do things this way, but I presume there was a good reason 🙂

Are we documenting and/or testing this somewhere?

Because I also see a lot of checks for instanceInitializerMethods.size() == 1, so there are definitely things we are handling differently when there is more than one instance initializer (checking of uses before/during initialization?).

[msridhar]

I don't think this is properly documented (since I had to look at the source code to figure out what we do). These are the best docs we have but they don't talk about multiple @Initializer methods:

https://github.com/uber/NullAway/wiki/Error-Messages#initializer-method-does-not-guarantee-nonnull-field-is-initialized--nonnull-field--not-initialized https://github.com/uber/NullAway/wiki/Error-Messages#initializer-method-does-not-guarantee-nonnull-field-is-initialized--nonnull-field--not-initialized

We should probably add some docs and tests around this.

On May 26, 2021, at 11:16 AM, Lázaro Clapp @.***> wrote:

Basically, the meaning of having multiple @Initializer methods is that all of them should be called before the class is used. I honestly don't remember why we chose to do things this way, but I presume there was a good reason 🙂

Are we documenting and/or testing this somewhere?

Because I also see a lot of checks for instanceInitializerMethods.size() == 1, so there are definitely things we are handling differently when there is more than one instance initializer (checking of uses before/during initialization?).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/uber/NullAway/issues/482#issuecomment-849011083, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABPEUKS4IMAKMGUBTXZ5CLTPU3HNANCNFSM45QNFPBA.

[xenoterracide]

So, this suggestion from @lazaroclapp should work I think:

option("NullAway:CustomInitializerAnnotations", "picocli.CommandLine.Parameters")

yes it works, I misread, I do that, sorry. I missed that I was using Options and not parameters.

I honestly don't remember why we chose to do things this way, but I presume there was a good reason

probably because frameworks like spring support setter injection (could be interesting to try supporting a check that checks these aren't called explicitly, not sure that would/could work)

to be honest the documentation for the cli options for initialization could be better, it's hard to tell when to use which. also, @Initializer keeps being mentioned, but haven't figured out what provides that annotation.

[msridhar]

Thanks for the update @xenoterracide. I'm re-purposing this issue to focus on improving documentation and testing of initializer support.

to be honest the documentation for the cli options for initialization could be better, it's hard to tell when to use which. also, @Initializer keeps being mentioned, but haven't figured out what provides that annotation.

There is one provided by FB's infer-annotation package, but in general you can use any annotation whose simple name is Initializer.

[xenoterracide]

well... this is awkward, reading this in 2024 trying to solve the problem for @TempDir from junit.

which none of these seem to work (tried outside of the conditional too)

      option("NullAway:HandleTestAssertionLibraries", true)
      option("NullAway:ExternalInitAnnotations", "org.junit.jupiter.api.io.TempDir")
      option("NullAway:CustomInitializerAnnotations", "org.junit.jupiter.api.io.TempDir")

class SemVerPluginTest {

  @TempDir
  File testProjectDir;

  File buildFile;

  @BeforeEach
  public void setup() throws IOException {
    Files.writeString(testProjectDir.toPath().resolve("settings.gradle"), "rootProject.name = 'hello-world'");
    buildFile = new File(testProjectDir, "build.gradle");
  }

[msridhar]

Sorry for the trouble @xenoterracide. The flag you want is ExcludedFieldAnnotations, e.g.:

option("NullAway:ExcludedFieldAnnotations", "org.junit.jupiter.api.io.TempDir")

Docs are here.