How do we review changes made with coccinelle?

Ævar recently sent a series that made pretty extensive use of coccinelle to remove a lot of the_repository [1], but then we ended up in this weird spot where even though several reviewers thought the code changes looked good, we weren't sure about giving Reviewed-By because we didn't know coccinelle or how to review it.

I'm pretty sure that the series could have been merged without a hitch if reviewers knew what to do about coccinelle. I expect more of these to appear as a result of the libification work, so it's probably a good time for us to figure out some norms about coccinelle :)

Perhaps we could start the discussion by sharing thoughts on the following questions, which I'll summarize in a change to contrib/coccinelle/README (where we can do final bikeshedding):

- Is it okay to give Reviewed-By on the basis of _just_ the in-tree
  changes and ignore the .cocci patch?

  - If not, what should reviewers look for in .cocci? Do we have a
    style?

- When do we introduce .pending.cocci vs .cocci?

- What do we do with .cocci after they've been applied?

- Do we care about new patches slowing down coccicheck?

Relevant threads
- How to learn cocci: https://lore.kernel.org/git/230326.86edpcw0yh.gmgdl@evledraar.gmail.com/ 
- https://lore.kernel.org/git/230328.86a5zxw31u.gmgdl@evledraar.gmail.com/
- https://lore.kernel.org/git/230326.86ileow1fu.gmgdl@evledraar.gmail.com/

[1] https://lore.kernel.org/git/cover-v2-00.17-00000000000-20230328T110946Z-avarab@gmail.com/

Glen Choo <chooglen@google.com> writes:

If they were made in separate steps, sure. If not, not really. But we can still say "I've checked the changes the author made to the code and they looked good." But we haven't reviewed the patch in its entirety in such a case to give a Reviewed-by, I would thihk.

When we keep .cocci rules in tree, "make coccicheck" would complain on any new code that matches the preimage pattern of these rules and adjust them. Your use of memcpy() may be rewritten to COPY_ARRAY() when appropriate. At least that is the theory---an overly wide or ad-hoc rule that depends too much on heuristic may misconvert future code, which needs to be caught by reviewers when .cocci files are added to the tree.

Surely.

We may want to cull rules from time to time. For example, a rule that moves callers of an older API function to use a newer API function has to be kept while the older API function still exists in the tree to help topics that are still in flight, but eventually everybody stops using it and the implementation of the older API function gets removed. We should make sure we remove the rule that is now stale.

Junio C Hamano <gitster@pobox.com> writes:

I agree, but I wonder what this means in practice when .cocci fluency is so low. Maybe:

- We increase .cocci fluency, partly by creating our own learning
  materials, partly by being more conscious about spreading knowledge
  from the folks who are relatively fluent in it. We might not have to
  do a lot to get the ball rolling either; just checking in a
  "MyFirstCocci" would help a lot, I think.

- Define some guidelines for what it means for a ".cocci" to be 'good
  enough'. E.g.:

  - Do rules need to target just the right things or is some collateral
    damage okay? Does this change between .cocci and .pending.cocci?

  - Do we allow copy-pasting in .cocci rules when a more 'elegant'
    alternative exists?

Orthogonal to the in tree .cocci rules that enforce code style, there are .cocci meant for large scale refactoring.

IIUC at least one other opinion there is that we keep such refactoring rules in tree temporarily for the benefit of in-flight or out-of-tree conflicts [1], but since they are defunct by definition, we remove them eventually.

[1] https://lore.kernel.org/git/230326.86ileow1fu.gmgdl@evledraar.gmail.com/

Glen Choo <chooglen@google.com> writes:

I think it goes both ways. Readers of course must be willing to learn, but whoever proposes to add a .cocci patch file should be able to explain what the rewrite rules in there are designed to do and why. It is pretty much the same deal as any other patches.

I'll wait and prefer to hear from others, also on points that I did not even quote back in my message because I didn't have anything to add myself, before digging further on points I responded to.

Thanks for starting this topic.

On Thu, Mar 30, 2023 at 12:13:07PM -0700, Junio C Hamano wrote:

I think that while none of us would probably call ourselves "Coccinelle experts", we are probably reasonably capable of reviewing *.cocci files and their impact on the tree.

What I meant when we were talking about this off-list was that I don't consider myself an expert at writing idiomatic Coccinelle rules. But I feel competent enough that I could review Ævar's patches by roughly grokking the *.cocci changes, and then checking that the resulting tree state matched my understanding of those changes.

I was the one who asked this question off-list, and I did so in a leading way that implied that the answer was "no".

But I agree with Junio that we *do* care about slowing down the performance of 'make coccicheck'. When I originally asked, I was under the (false) impression that we didn't run 'make coccicheck' in CI. But we do (see ci/run-static-analysis.sh), so we do care about the performance there since we don't want to unnecessarily slow down CI.

Thanks, Taylor

On Thu, Mar 30, 2023 at 11:23:40AM -0700, Glen Choo wrote:

The documentation in contrib/coccinelle/README is authoritative. I'll refer you there for the full details, but the rough idea is that:

- *.cocci patches are used to guard against bad patterns in the code
  (e.g., that we should never write "if (thing != NULL)" and instead "if
  (thing)"). When applying these transformations leads to changes, they
  are designed to break the build to avoid letting these bad patterns
  enter our tree.

- *.pending.cocci patches are used to guide large scale refactorings,
  like Ævar demonstrated in his last series. They don't cause us to fail
  the build in the meantime while the refactoring is in progress.

The latter is useful when the refactoring is either (a) too large to fit in a single patch, or (b) would cause too many conflicts with in-flight series.

(FWIW, I think that I'm mixing up my terminology, in that *.cocci files are typically referred to as "transformations" and the result of running Coccinelle on them is used to produce a patch. Shows you how much I know about Coccinelle ;-).)

Thanks, Taylor