Page MenuHomePhabricator

Add -Wno-error=unknown flag to clang-format.
ClosedPublic

Authored by fodinabor on Aug 18 2020, 5:57 AM.

Details

Summary

Currently newer clang-format options cannot be included in .clang-format files, if not all users can be forced to use an updated version.
This patch tries to solve this by adding an option to clang-format, enabling to ignore unknown (newer) options.

Diff Detail

Event Timeline

fodinabor created this revision.Aug 18 2020, 5:57 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 18 2020, 5:57 AM
fodinabor requested review of this revision.Aug 18 2020, 5:57 AM
bkramer edited reviewers, added: krasimir; removed: bkramer.Aug 20 2020, 8:13 AM
MyDeveloperDay added a project: Restricted Project.
MyDeveloperDay removed a project: Restricted Project.
MyDeveloperDay removed a subscriber: llvm-commits.
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2020, 7:42 AM

This has caught me out from time to time, but the presence of such an option can lead to users mixing clang-format versions which could lead to flip/flopping of changes, so I'm not 100% sure it won't drive bad behaviour.

However breaking based on the option can be a pain if the code you are formatting isn't even using the new option

I wonder is it possible to do this without impacting the LLVM support libraries?

I see the possible issue with the possible version mismatches and that is why I'd make people opt-in for this option, to still being able to format their files (e.g. if using out-dated built-in versions like in Visual Studio - I know you can specify your own binary) but the real formatting can be done with a pre-commit script or similar, where the new options are actually supported.

Regarding not touching the LLVM support library: I'd love to find a way, but as clang-format uses the >> operator to read the YAML and this operator automatically emits the errors, I don't see any other obvious way.. (maybe there's a LLVM trick to suppress those errors that I haven't seen, yet :))

Regarding not touching the LLVM support library: I'd love to find a way, but as clang-format uses the >> operator

We need to find some reviewers who look after a wider area of LLVM, if you want to make a change out there in LLVM/support, but it above my pay grade to approve ;-) maybe one of the more experienced devs could help identify the correct person @aaron.ballman @sammccall or alternatively take a look via git log as to who has been maintaining that file @JDevlieghere, they may have an opinion about how this should be done.

clang/tools/clang-format/ClangFormat.cpp
108

feels like a mouthful is there nothing shorter we could use? -Wignore (or something)

llvm/include/llvm/Support/YAMLTraits.h
792

I'm not a massive fan of functions with out parameter names, but I see your following the local style.

1523

is this clang-formatted?

llvm/lib/Support/YAMLTraits.cpp
203

do we want to flat out ignore or just report but not fatally. (just a thought) silent failures are hard to diagnose

Thank you so far for the feedback!
maybe you can give further guidance on the comments on the comments :)

As of the git history it seems that @grimar did some work on YAML error handling..

clang/tools/clang-format/ClangFormat.cpp
108

hmm... -Wunknown
but the -W does not really make it clear that the default "errors" should now be treated as warnings instead. From compiler conventions, I'd expect the -W to enable a warning ...

and something like -Wno-error=unknown is not really shorter...

llvm/include/llvm/Support/YAMLTraits.h
1523

the patch is... the original alignment of the members is not. (also see the CurrentNode's formatting).
not sure what to do in this case, as the whole file seems rather unformatted?

llvm/lib/Support/YAMLTraits.cpp
203

true.. don't know what's the best option?

keep it as a printed out error and just don't return an error code on exit? This option would make it a clang-format only change, but feels really dirty.

Otherwise I'd have to dig my way through to llvm::yaml::Stream::printError (or maybe rather add a printWarning) to conditionally change the message type for the ignore case.

grimar added a comment.Sep 7 2020, 7:26 AM

I am not familar with clang-format, but have a few comments inlined about the rest.
I think the new setIgnoreUnknown YAMLlib API is probably OK generally.
I'd perhaps call it differently, e.g. setAllowUnknownKeys though.

Also, I think you need to add a unit testing for the new functionality.
It seems appropriate places are: clang\unittests\Format\FormatTest.cpp (to test clang-format)
and llvm\unittests\ObjectYAML\YAMLTest.cpp (to test new YAML Input::setIgnoreUnknown API)

I'll add people who might have something useful to say too.

llvm/include/llvm/Support/YAMLTraits.h
1511

I'd add a parameter name here. Seems the style is mixed in this file,
but it is a public member, and having no names for arguments actually
reads bad I think. Not sure why it was done initially.

Probably the same applies for setIgnoreUnknown in the base class.

llvm/lib/Support/YAMLTraits.cpp
203

Yes, I think we might want to introduce a method, like Input::reportWarning
which will call Strm->printError(node, message);, but will not set a EC.
Also, it should print "warning: ..." instead of "error: ..." prefix somehow.

753

You don't actually need it for Output, right?
I think instead you can have a default implementation in the base class, which should call llvm_unreachable.

I can see the use of this, but I am also wary that ignoring style options will lead to people producing different results on different versions of clang-format. This is both because having set-or-unset an option will naturally lead to different code and also that newer options are a de-facto check that clang-format is at least a certain version (we have minor differences between major versions as bugs are fixed). In any case, I see this being *very* easy to misuse and the documentation should have a warning reflecting that.

As far as docs go, the bulk are in clang/docs/ClangFormat.rst and require no additional 'publish' step but definitely should be updated. The command line options are implicitly generated for the cli tool's --help flag, but the docs for them are all in that rst file and manually maintained.

clang/tools/clang-format/ClangFormat.cpp
108

I personally like -Wno-error=unknown if the behavior is to emit warnings and -Wno-unknown if the behavior is to be silent, for consistency with clang/gcc.

I would also put a note in the description saying that ignoring options can lead to dramatically different output between versions that do and don't support a given option, so it should be used with care.

llvm/lib/Support/YAMLTraits.cpp
203

Something like how clang/gcc only report unknown -Wno-foo options if there's another error is an idea, but clang-format almost never fails unless there's a bad config or input file so that's not too useful.

I'm fine with either warning or being silent. If the user has opted-into ignoring missing options, we can assume they're willing to accept the consequences of such.

fodinabor updated this revision to Diff 290460.Sep 8 2020, 5:24 AM
fodinabor marked 9 inline comments as done.

Incorporating review comments:

  • renaming option to -Wno-error=unknown and adding warning in description
  • emit warnings instead of fully ignoring the issues

Documentation and unit tests will follow

fodinabor updated this revision to Diff 290461.Sep 8 2020, 5:25 AM

Remove test entry form .clang-format :)

grimar added inline comments.Sep 9 2020, 12:41 AM
llvm/lib/Support/YAMLTraits.cpp
204–209

Please don't use auto when the variable type is not obvious.
Use the real type name instead.

205

The same here, but also the result type here is Twine, and you shouldn't use it like this. Documentation says:
"A Twine is not intended for use directly and should not be stored, its implementation relies on the ability to store pointers to temporary stack objects which may be deallocated at the end of a statement. Twines should only be used accepted as const references in arguments, when an API wishes to accept possibly-concatenated strings."
(https://llvm.org/doxygen/classllvm_1_1Twine.html#details)

You can probably inline it or use std::string, since this is a error path only code.

386

Why do you need both reportWarning methods? I think you can have only one for now.

Typz added a subscriber: Typz.Sep 10 2020, 8:02 AM

Address review comments, copy the help text into docs and add some basic unit tests.

grimar added inline comments.Sep 14 2020, 4:10 AM
clang/unittests/Format/FormatTest.cpp
16338

It looks like it is very similar to "Test 7:" and can be a part of it?
E.g:

...
llvm::consumeError(Style7a.takeError());

// <comment>
auto Style7b = getStyle("file", "/d/.clang-format", "LLVM", "", &FS, true);
ASSERT_TRUE((bool)Style8);
llvm/unittests/ObjectYAML/YAMLTest.cpp
39

UnknownOption

40

std::string->StringRef?

41

Will this work if we switch the order of InvalidKey and Binary?

std::string InputYAML = "InvalidKey: InvalidValue\n"
                        "Binary: AAAA";

I expect that yes and I think it is reasonable to do it to show that we don't stop parsing an YAML
when there is an unknown key.

44

No full stop at the end.

57

Do you need BH_GT? Can it be just:

// test 2: only warn about invalid key if actively set.
llvm::yaml::Input Input2(InputYAML);
BinaryHolder BH2;
Input2.setAllowUnknownKeys(true);
Input2 >> BH2;
EXPECT_EQ(Input2.error().value(), 0);
EXPECT_EQ(BH2.Binary, yaml::BinaryRef("AAAA"));
fodinabor updated this revision to Diff 291826.Sep 15 2020, 1:21 AM
fodinabor marked 9 inline comments as done.

Unit test cleanups.

All comments should be adressed now.

grimar accepted this revision.Sep 16 2020, 1:01 AM

LGTM. It worth wainting for a second approvement and/or other comments to verify that people are happy with doing this for clang-format.

clang/unittests/Format/FormatTest.cpp
16317–16318

nit: since you have Style7b, this perhaps should be Style7a.

This revision is now accepted and ready to land.Sep 16 2020, 1:01 AM
fodinabor updated this revision to Diff 292781.Sep 18 2020, 7:09 AM
fodinabor marked an inline comment as done.

Fix the nit :)
Anyone with access may commit this now (ideally ofc someone with a final opinion on the clang-format part).

fodinabor retitled this revision from Add ignore-unknown-options flag to clang-format. to Add -Wno-error=unknown flag to clang-format..Sep 18 2020, 7:11 AM
fodinabor edited the summary of this revision. (Show Details)

IMHO I feel that given that these changes are more wide reaching than just clang-format that it might be more correct for your to request commit access yourself.

You need to own these changes, because if the llvm/Support changes fail the build or unit tests the person who commits it is going to have to resolve those issues.

It doesn't take long to request commit access.

Okay, I requested commit access.
So the patch is fine with you from a clang-format perspective?

it certainly LGTM, like I said I've had met this issue before and I think this does get around that situation where perhaps your not even using the option it's complaining about (especially if its a C++ option and you are using clang-format for C#)

So yes I think it's good. Thank you for the patch and getting the commit access.

When you commit don't forget to keep an eye on the build_bots at http://lab.llvm.org:8011/ this was something I didn't learn about until after my first commit... (to my cost)

MyDeveloperDay accepted this revision.Sep 18 2020, 12:16 PM
This revision was landed with ongoing or failed builds.Sep 19 2020, 1:18 AM
This revision was automatically updated to reflect the committed changes.