How do we review changes made with coccinelle?

How do we review changes made with coccinelle?

[Glen Choo]

Æ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/

Re: How do we review changes made with coccinelle?

[Junio C Hamano]

Glen Choo <chooglen@google.com> writes:

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

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.

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

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.

> - Do we care about new patches slowing down coccicheck?

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.

Re: How do we review changes made with coccinelle?

[Glen Choo]

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

> Glen Choo <chooglen@google.com> writes:
>
>> - Is it okay to give Reviewed-By on the basis of _just_ the in-tree
>>   changes and ignore the .cocci patch?
>
> 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.

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?

>> - What do we do with .cocci after they've been applied?
>
> 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.

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/

Re: How do we review changes made with coccinelle?

[Junio C Hamano]

Glen Choo <chooglen@google.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.

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.

Re: How do we review changes made with coccinelle?

[Taylor Blau]

On Thu, Mar 30, 2023 at 12:13:07PM -0700, Junio C Hamano wrote:
> Glen Choo <chooglen@google.com> writes:
>
> > - Is it okay to give Reviewed-By on the basis of _just_ the in-tree
> >   changes and ignore the .cocci patch?
>
> 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.

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.

> > - Do we care about new patches slowing down coccicheck?

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".

> Surely.

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

Re: How do we review changes made with coccinelle?

[Taylor Blau]

On Thu, Mar 30, 2023 at 11:23:40AM -0700, Glen Choo wrote:
> - When do we introduce .pending.cocci vs .cocci?

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