Break loops when result can no longer change

[XN137]

No description provided.

[XN137]

general note:

please let me know in case you consider these unnecessary micro optimizations

for me, when reading the code i always expected the loop bodies to have "meaningful" side-effects but from reading the inner code i think it was always just doing redundant work, with no observable difference, but maybe i am missing something

[msridhar]

Overall these changes LGTM. I don't know if we're consistent about breaking out of loops whenever possible, but it seems good to do so. @lazaroclapp if you have time you can take a quick look before we merge

[msridhar]

Not exactly related to this PR, but in general, I feel like our use of CompositeHandler is set up for a potentially-bad future bug where multiple handlers implement a particular method and their interaction is unexpected or order-dependent. @lazaroclapp do you know if we have known cases where multiple handlers try to "handle" the same construct and we expect one to win? If not, we may want to implement some kind of "handler debug mode" (off by default) where we actually check in CompositeHandler that only one contained handler is returning a non-default value for each case. It will make the code messier but may save us some pain in the future.

[lazaroclapp]

This would not work:

check in CompositeHandler that only one contained handler is returning a non-default value for each case

Handlers are meant to override and use the computations of one another, which is why they execute in a chain with meaningful order between them. Each Handler API method defines how results from different handlers are combined, in the case of onDataflowVisitMethodInvocation (admittedly one of the most complex) it's based on the priority given by the NullnessHint enum. This definitely leads to weird corner cases, often, but it also gives us a lot of flexibility in deciding which "source" should win for any given nullness fact (e.g. between jar annotations and library models).

[627721565]

您好，邮件已经收到，查阅后会尽快给您回复！

[lazaroclapp]

There are a couple safe optimizations here (marked them with 👍 ), but in general breaking out early of handler chain iteration is not a safe optimization. The contract is that for a given event, the corresponding method will be called for all handlers in the chain, and these methods can have side effects beyond the value returned.

[lazaroclapp]

I don't know if this will currently break anything, but this is not a safe optimization.

It means that calls to .onDataflowVisitMethodInvocation(...) might be skipped when a handler early in the chain "wins" in terms of determining the NullnessHint. But keep in mind that handler methods are not meant to be pure, they are meant to be callbacks for events of the analysis. In fact, .onDataflowVisitMethodInvocation(...) specifically is often called for its effects on its AccessPathNullnessPropagation.Updates paramenters.

For example, AssertionHandler will always return NullnessHint.UNKNOWN, because it doesn't actually care about the nullability of the statement assertThat(foo).bar(), but will instead add whatever facts that assertion statement indicates about foo into bothUpdates.

[XN137]

good point - thanks for the explanation, not sure how i missed this 😓

so for assertThat(x).isNotNull() when the handlers examine the isNotNull() method invocation part, we dont really care about the nullness hint on the handlers return for isNotNull() but we want to give them the opportunity to examine the whole method-call-chain and realize that x is non-null afterwards (i.e. update bothUpdates)

[lazaroclapp]

Almost correct. It's not about the method call chain, but the chain of handlers. The way these handlers are used is, whenever you call a method like .isNotNull(), each handler in turn has the opportunity to process it. Some handlers, like LibraryModelsHandler will process it by i.e. looking up some information and deciding whether or not the return of method call should @Nullable or @NonNull. But that's not the only action to be taken on such call. As you know, AssertionHandler will instead look at the receiver of .isNotNull() and unpack x within a assertThat(x) receiver and then set the nullability of x (within dataflow after the current statement).

With your change, if a handler early in the chain computes a FORCE_NONNULL nullability for the method call's return, then no handler after that has a chance to process the call and perhaps set other state or any of the updates passed as arguments based on the call.

[lazaroclapp]

👍

[lazaroclapp]

👍

[lazaroclapp]

The remaining two optimizations LGTM