[llvm-dev] "[NFC]" Abuse

[Luke Drummond via llvm-dev]

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

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.

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