clang-format interpreting variable name interface as some kind of keyword and changes formatting in constructor delegation

[zvolin]

Hi,
as in title, I have clang in version 13.0, on 12.0.1 the issue does not exist.

Here is a minimal example:

class Foo {
  int interface;

  Foo::Foo(int iface) : interface { iface } {
  }
}

class Bar {
  int outerface;

  Bar::Bar(int oface) : outerface{oface} {}
}

int main() {
  auto foo = Foo(1);
  auto bar = Bar(1);
}

I've included it here: https://github.com/Zwo1in/clang-format-interface-reproduction

[llvmbot]

@llvm/issue-subscribers-clang-format

[llvmbot]

@llvm/issue-subscribers-bug

[mydeveloperday]

This is exactly what you'd get for any keyword.

won't be treated any different from any other keyword (because its no longer a tok::identifier)

This is because interface is a keyword of at least Java & C#

i.e.

class Foo {
  int class;

  Foo::Foo(int iface) : class { iface } {
  }
}

1. calling a variable interface isn't the best idea! do yourself a favour and pull up!
2. its hard for us to revisit every rule to add

(!Style.isCSharp() && !Style.isJava() && Tok->is(tok::kw_interface)

Could we fix it, yes, would we with any priority......

patches welcome

[mydeveloperday]

Problem solved... ;-)

#define INTERFACE interface

class Foo {
  int INTERFACE;

  Foo::Foo(int iface) : INTERFACE{iface} {}
}

class Bar {
  int outerface;

  Bar::Bar(int oface) : outerface{oface} {}
}

[mydeveloperday]

That said, actually the following is all that's effectively needed

diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index a614d1bdee7a..29ffd46e0364 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -1589,7 +1589,7 @@ void UnwrappedLineParser::parseStructuralElement(bool IsTopLevel) {
         return;
       }

-      if (FormatTok->is(Keywords.kw_interface)) {
+      if (!Style.isCpp() && FormatTok->is(Keywords.kw_interface)) {
         if (parseStructLike()) {
           return;
         }

class Foo {
  Foo::Foo(int iface) : interface{iface} {}
}

class Bar {
  Bar::Bar(int oface) : outerface{oface} {}
}

let me make the review and we can discuss it. (I'd have to look at whats not covered by isCpp() i.e. that might have an impact on ObjectiveC/ObjectiveC++)

[zvolin]

As far as this indeed works for this case, I'd rather look for a more generic solution rather than specialized one like this. I was thinking about maintaining the list of keywords for given language and check whether given word is a keyword in present context. This doesn't sound like tons of effort however I'm not familiar with the code base. Gonna give this a try as well

[zvolin]

Okay, I pass on this one. I wanted to go with something like:

// clang/include/clang/Lex/Token.h
 bool is(tok::TokenKind K, llvm::clang::format::FormatStyle::LanguageKind L) const {
    /*... here I'd check if K is in extraTokens and L is in langs that have extraTokens and return is(TokenKind K) only if both or none of those are true ...*/
 }

however I'm kinda not enjoying writing this and going through not PR process, so I leave this as is. It's been already fixed by renaming on our side also and is not important from what I understand

[zvolin]

This is not really an exhaustive solution, as it still can confuse C# with Javascript, however the meat of the code for this is already written.

[mydeveloperday]

I don't believe a generalised solution is required here, this is a corner case and one that in my view should be dealt with as a corner case

The reality is if we presented code like this using interface as a variable name i personally would reject it during code review. Whilst we want clang-format to be 100% correct it cannot be without semantic information and I for one don't want clang format to need compiler flags in order to understand what people using macros or common language keywords as variables really meant when they could easily change their ways and be good!

We do already have keyword lists but I think if we made a sudden change to is/isNot like this we'd break the world

Also remember objc uses interface and that code can be in a .h file so even this suggested change could in theory be wrong

[mydeveloperday]

If you are looking to make the simple change I suggested as a good first issue I am happy to lead you through the patch process of adding tests etc

[llvmbot]

@llvm/issue-subscribers-good-first-issue

[kleisauke]

For reference, the above mentioned patch is applied to:

llvm-project/clang/lib/Format/UnwrappedLineParser.cpp

Lines 2010 to 2014 in 1522a3b

	if (FormatTok->is(Keywords.kw_interface)) {
	if (parseStructLike())
	return;
	break;
	}

IIUC, this probably fine since other FormatTok->is(Keywords.kw_interface) checks are currently only guarded for either Java or JavaScript.

llvm-project/clang/lib/Format/UnwrappedLineParser.cpp

Lines 1756 to 1757 in 1522a3b

	} else if (Style.Language == FormatStyle::LK_Java &&
	FormatTok->is(Keywords.kw_interface)) {

llvm-project/clang/lib/Format/UnwrappedLineParser.cpp

Lines 1977 to 1978 in 1522a3b

	if ((Style.isJavaScript() || Style.Language == FormatStyle::LK_Java) &&
	FormatTok->is(Keywords.kw_interface)) {

[owenca]

https://reviews.llvm.org/D148437