[llvm-dev] "[NFC]" Abuse

[David Blaikie via llvm-dev]

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.

Thanks for the summary!

> 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?

I don't think it does that - to me the only difference with "NFC" when
reviewing a patch (pre or post-commit) is "Is this intended to be
testable/tested". If it says NFC I expect to see no tests updated and
review the code change to ensure/verify that it shouldn't/couldn't be
tested (in addition to whatever change-specific review to do).

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

I lost track of this bit a bit. We do often expect newcomers to do API
changes - one of the perennial "easy things for a newcomer to do" is
to change APIs that take other string types to take StringRef for
example - which is generally an NFC API change.

(aside: folks keep saying "public API" - LLVM has no clear distinction
of public (if we're using that in the sense of "outside the LLVM
project") and private API - there's some library internal APIs (those
where the header is in the lib directory rather than the include
directory) but everything else is "inter-library API" which is about
as "public" as it gets, I guess - if other parts of LLVM can use it,
so can people using LLVM's libraries from outside)

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

Generally we should be somewhat careful of such a refactor because we
might miss a call site inside LLVM too - but I wouldn't rule it out.
It's not hard inside LLVM to search for all the call sites and update
them. I think no matter the words used in the commit message I think
that's always going to be a reality in the LLVM project (that there
might be silently incompatible API changes).

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

Do you have references on broader/other/"usual" uses of the term NFC and NFCI?

At least my attempts to google the terms seem to only turn up LLVM and
LLVM-adjacent (eg: Swift) results.

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

This might be misquoted/out of context. It looks to me like John was
agreeing with my description - John's concept of "purely internal
implementation details" may include what you are describing as "Public
API" - the public interface John is probably talking about (I think,
given he opens with "yeah" in response to my description) is LLVM IR,
bitcode, and command line tools - not APIs LLVM uses internally to
implement that functionality.

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

Conflating the "there can be silent API breakage that causes semantic
changes without build breaks" and "there are changes tagged NFC that
change functionality" (due to developer mistakes/bugs) and "there are
changes tagged NFC that change APIs" together is probably confusing
things a bit. For instance I think the first of those is relatively
unavoidable - and how something's tagged isn't going to change that.

I don't think NFCI v NFC (I don't mind people choosing one or the
other, but I don't think as a community we should try to split hairs
and either say "you can't say NFC, you have to say NFCI always" or
"here's the level of complexity over which we aren't comfortable
saying it's for sure, so it's only 'intent'") really helps - all patch
descriptions are statements of intent, not proven fact - we all write
bugs, if we have to add defensive wording for the possibility of bugs
in any commit message, I don't think that's going to improve
clarity/communication. And that distinction still wouldn't address the
desire for API changes to not be flagged NFC[I] - which, again, I
think is a bad thing, annotating changes that don't/shouldn't be
tested is a core property of the use of the NFC[I] flag.

- Dave