[llvm-dev] "[NFC]" Abuse

Wed Jun 23 14:57:54 PDT 2021

On Wed, Jun 23, 2021 at 10:34 AM Luke Drummond via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> Hi all
>
> Thanks for all the responses.
>
> Since there have been quite a few separate alignments one way or another
> I'll
> try and summarize everything mentioned in one mail, being fairly liberal
> with
> the quoting and paraphrasing. Ed: please forgive.
>
>
> Basically it seems there are two distinct groups:
>
> Those who don't consider API changes as "functional" consider LLVM's
> in-tree
> command line tools as the "function", and consider changes to the API fair
> game
> for the NFC tag.
>
> I argued that API changes are, by definition, not NFC. It was fairly
> strongly
> rejected by several:
>
> David Blaikie:
> > I don't think that's how the LLVM project should/is going to classify
> > "functional change" - in part because there isn't a clear "public" API
> > boundary - there are wide interfaces across the whole project that
> external
> > users can call into.
>
> Nikita Popov:
> > For what it's worth, my understanding was that NFC can also include API
> > changes, as long as they are, well, non-functional. In LLVM pretty much
> > everything is part of the public API, so non-functional refactoring will
> > often touch the API.
>
> Mehdi Amini:
> > Yes I am ignoring API users, I am on the same line as Nikita here.
> > We don’t have stable APIs (other than the C one), so I for example I may
> > change an API that was taking 3 bools to take now a struct parameter
> > wrapping the 3 bools instead. I’ll tag it NFC.
>
> Chris Lattner mostly agrees with the above:
> > "NFC" is intended to talk about the behavior of the compiler, not its API
>
> and
>
> > [...] "NFC" *encourage people* to land large changes as a series of NFC
> > refactorings.
>
> I think the final point is pretty telling about the use of [NFC] among llvm
> developers historically.
>
> Historically I can see the attraction of minimizing the cognitive barrier
> to
> addressing poorly factored code, but perhaps now it's an opportunity to
> rubber-stamp your commits to avoid the slow review process? When we have a
> mature system, fairly robust test-suite, code-review, and CI, I don't
> think the
> minor benefit of softening the armour is necessary when the sorts of
> changes
> that actually do require changing public APIs are almost never done by
> newcomers
> [citation needed].
> To continue doing this when downstreams have to refactor their code every
> couple
> of days, and - crucially - find the commit to blame is frustrating.
>
> Then there are those more conservative devs whose wishes align more with
> my own.
>
> David Jones:
> > On some occasions I have dealt with API changes where compiling my old
> code
> > with the new API results in a correctly-compiling program. However, the
> > resulting application fails to run correctly. This issue is much harder
> to
> > track down.
>
> I think this distills my argument. Sometimes due to promotions things like
> this get past the compiler with no change of the user:
>
> - void maybeDoThing(unsigned Count, bool AreYouSure);
> + int maybeDoThing(bool AreYouSure, unsigned count);
>
> ...and then do something pretty nasty when you least expect it. If this
> commit
> is marked NFC, the committer has actively misdirected anyone who doesn't
> understand LLVM's unusual use of "NFC", so the barrier to using LLVM
> becomes
> higher again. Do this enough times and using LLVM's API becomes a cruel and
> unusual punishment ;)
>
> John McCall:
> > Yeah, I expect NFC changes to be purely internal implementation
> > details.  Changing the public interface (even to add a feature)
> > isn’t NFC; neither is anything that can affect output.
>
> Joerg Sonnenberger:
> > I would consider an even more restricted subset: NFC changes would be
> trivial proven to be just that
>
> David Blaikie in response to Joerg and John
> > I disagree pretty strongly there - intent is important and all any patch
> > comment can describe.
> >
> > If that change was adopted, I'd want another flag for the "I intend this
> > not to change the observable behavior, but it's not trivially proven" -
> > especially when reviewing a patch that is missing test coverage. If
> someone
> > hasn't made a claim that this doesn't change behavior, then I expect that
> > the inverse is true (it does change behavior) and a test should be
> present.
> > And once we have that flag, I'm not sure the difference between "I intend
> > this to not change behavior, but it passes some threshold of triviality
> > that makes it not 'guaranteed'" and things below that threshold is
> > sufficiently valuable to encode in the commit message (& haggle over
> which
> > things are over or under that threshold).
>
> I'd say that David is already describing a tag in popular use: "NFCI" is
> what's
> conventionally used outside LLVM (and even within LLVM by others) to tag
> changes
> that match exactly that description.
>
> David also sort of foreshadowed this opinion in another message:
>
> > We could introduce a separate term but I think most NFC patches would use
> > that term instead, so it probably wouldn't amount to much real change -
> > we'd end up using that new term ubiquitously instead.
>
> Fangrui Song mentioned the following in a separate message which - to me -
> largely aligns with David's intent:
> > Sometimes people use the term "NFCI" (no functional change intended).
> > I thought "intended" means that: the author is not 100% sure that no
> > functional change is caused (for some refactoring it is sometimes
> > difficult to guarantee without good test coverage)
> > but seems that many use "NFCI" to refer to obviously NFC things.
>
> I think that in the face of an extant "NFCI" tag, these 3 opinions actually
> *are* compatible with each other, but whether those that advocate for "NFC"
> where others have described "NFCI" are aware of the existence of this tag
> is
> unclear to me.
>
> Another point was raised about downstreams and pre-commit testing by
> Christian
> Kühnel. Christian suggested integration of upstream LLVM and a number of
> popular
> downstreams' CI systems:
> >  We could be running some *automatic integration testing with downstream
> >  users*: Have some *public* CI machine build the heads of LLVM and a
> >  downstream project (e.g. Rust, Tensorflow) and see if the builds still
> pass.
>
> I think this is noble but misses the essence of my original point: It's
> not that
> LLVM changes *per-se* that's troublesome as a downstream. It's that LLVM
> changes but the committer that made the change has said that it's not
> changed.
>
> LLVM *should* continue to change, but I still think the NFC tag is
> incorrect
> with respect to API changes, and any gentle push to make life easier to
> downstreams would be something I'm sure would be appreciated. Applying
> "[NFC]"
> to silent breakages goes somewhat beyond "we provide no API stability"
> (which
> is clear and well understood by the community) to "this is active
> misdirection
> that makes your life harder as a maintainer".
>

I suspect there is an irreconcilable underlying motivation here in how
we're looking at it, in terms of who is the intended target of this
signaling. I feel like you're looking at getting a signal through NFC for a
downstream project looking at finding issues during their integration,
while I'm not using NFC for this purpose. Instead I'm using NFC as a
signaling tool internal to the project development itself.
 it is a signal for reviewers of my revision (no test change here, and also
"Please reviewer: validate my intent here, am I really not changing the
behavior of the compiler?") and vice-versa when I am the reviewer. It is
also a potential signal for narrowing down a blamelist sent by broken bot
post-commit (I've also used it to pre-filter a list of commits when
tracking performance regressions in the past). It isn't bulletproof: a `git
bisect` can lead to a NFC commit, but that's because we're human and we
make mistakes.

> In summary, I think there's still consensus to be made, and if us loud
> voices
> have equal standing, there's little majority, so I don't think I can
> formalize
> this for the documentation yet.

There are two things here, "should we change what we do to be more strict",
on which I don't see a consensus here, and "should we document better how
this tag has been widely used till now", which is a different thing :)

-- 
Mehdi

>
> Thanks for all the input so far. Apologies if I missed anyone.
>
> More voices welcome.
>
>
> All the Best
>
> Luke
> --
> Codeplay Software Ltd.
> Company registered in England and Wales, number: 04567874
> Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210623/345beba8/attachment.html>