Update cxx operator implementation by Huddie · Pull Request #58910 · swiftlang/swift

Huddie · 2022-05-13T22:33:12Z

This PR aims to improve the CXX class member operator implementation by making it a more true mapping from the CXX world.

Previous Impl

Previously if a class in CPP has an operator member function

class Foo {
  Foo operator+(Foo a, Foo b);
}

it would get mapped into static method inside the generated Foo class

struct Foo {
 static func +(lhs: inout Foo, rhs: Foo) -> Foo
}

This achieves the correct result but the operator is a static function and therefore the Foo class doesn't really have the same methods on it that it did in CPP

New Impl

This change maps the member operator function to a member function in the swift class & also create a separate swift operator static function which calls the member function

struct Foo {
 func __operatorPlus(rhs: Foo) -> Foo { ... }
 static func +(lhs: inout Foo, rhs: Foo) -> Foo  { 
   return lhs.__operatorPlus(rhs)
 }
}

A few windows tests need tor enabled in a follow up

Huge thanks to @zoecarver !

zoecarver

This looks great! Thank you so much Ehud! I only have a few minor comments.

Also, either in this PR or another (maybe better to do another one), would you mind re-enabling these tests on Windows (they should work now) and adding the test from this PR: https://github.com/apple/swift/pull/32869/files ?

And as we've discussed offline, I think this will allow us to get rid of a lot of special-case code throughout silgen! Thanks again :)

zoecarver · 2022-05-16T15:51:47Z

lib/ClangImporter/ClangImporter.cpp

nit:

Suggested change

? DeclBaseName(SwiftContext.getIdentifier("__operator" + getOperatorNameForToken(std::string{name.getBaseName().getIdentifier()})))

? DeclBaseName(SwiftContext.getIdentifier("__operator" + getOperatorNameForToken(name.getBaseName().getIdentifier().str())))

I can do .str().str() to go from identifier to StringRef to std::string

Or should I make getOperatorNameForToken take take StringRef ?

zoecarver · 2022-05-16T15:53:10Z

lib/ClangImporter/ImportDecl.cpp

Can we update this to use getOperatorNameForToken?

zoecarver · 2022-05-16T15:56:23Z

lib/ClangImporter/ImportName.cpp

Here std::string is a temporary, so you can't get a "ref" to it (StringRef is basically a pointer to the temporary memory). Beyond that, I think we'll need to make baseName a std::string or allocate an identifier (like you did above), because it's declared/used out of this scope.

What is happening in the other cases here? Like:

baseName = "callAsFunction";

Is this not equivalent to:

baseName = StringRef{"callAsFunction"};

?

Is this an rValue that gets moved into baseName?

String literals are sort of like globals, they last for the duration of the program, so you don't have to worry about their lifetime. std::strings on the other hand are destroyed at the end of the scope if they're locals, otherwise at the end of the expression of they're temporaries. A StringRef doesn't store anything, it's a reference to a char *. If you take the reference of a "global" that's always fine. If you take the reference of a local/temporary, that's where we run into problems, because the string may have already been destroyed.

zoecarver · 2022-05-16T16:01:29Z

Also, would you mind running this whole PR through git-clang-format. You can just run git-clang-format main.

zoecarver · 2022-05-16T16:01:37Z

@swift-ci please test.

zoecarver · 2022-05-16T16:39:32Z

test/Interop/Cxx/operators/member-inline-silgen.swift

There's an XFAIL at the top of this file and in a few of these operator tests. I suspect you didn't see these locally because your repo is out of date. Anyway, you'll also need to re-enable operators once you pull from main. Let me know if you have any questions about this.

LucianoPAlmeida · 2022-05-18T02:18:23Z

lib/ClangImporter/ClangImporter.cpp

Also nit: Seems to be missing clang-format
You can use either git-clang-format tool that is on ${PATH_TO_SWIFT}/llvm-project/clang/tools/clang-format and you can add it to your path, and run on the git directory after git add .
Or clang-format -lines 1630:1660 lib/Sema/CSFix.h -i for example to run at lines in specific.

Thanks! Will do!

LucianoPAlmeida · 2022-05-18T02:22:43Z

lib/ClangImporter/ClangImporter.cpp

It seems like those two loops are exact the same code(just over different lists), could this be abstracted into a function/lambda in some way?

Yes I think so! Just gotta rebase and fix some tests first.

…names __operator(Minus, Plus,...) and fix test cases

Huddie · 2022-05-22T01:46:13Z

@swift-ci please smoke test

Huddie · 2022-05-24T17:14:14Z

@swift-ci please smoke test

zoecarver · 2022-05-25T18:05:56Z

Is this ready for re-review?

Huddie · 2022-05-25T19:15:13Z

Is this ready for re-review?

Yes please :)

Huddie · 2022-05-26T15:08:00Z

@swift-ci please smoke test

zoecarver · 2022-05-26T17:58:51Z

lib/ClangImporter/SwiftLookupTable.cpp


  // Find the lookup table entry for this base name.
-  auto known = findOrCreate(LookupTable, baseName,
+    auto known = findOrCreate(LookupTable, baseName,


Nit: spacing

zoecarver · 2022-05-26T17:59:51Z

lib/ClangImporter/ImportDecl.cpp

-      // Support for importing operators is temporarily disabled: rdar://91070109
-      if (decl->getDeclName().getNameKind() == clang::DeclarationName::CXXOperatorName &&
-          decl->getDeclName().getCXXOverloadedOperator() != clang::OO_Subscript)
-        return nullptr;


The one thing we will probably want to keep disabled is templated operators. Those cause issues with ambiguity. But we can probably disable those in a follow up patch, given this patch is pretty big already.

zoecarver

I want to do a final review pass but this generally looks good to me. Thanks again, Ehud!

LucianoPAlmeida · 2022-05-27T00:38:52Z

lib/AST/DeclContext.cpp

  }
 }

+


Were those changes intended?

Nope lol fixing

LucianoPAlmeida · 2022-05-27T00:40:16Z

lib/ClangImporter/ClangImporter.cpp

-    if (isVisibleFromModule(ModuleFilter, VD))
-      NextConsumer.foundDecl(VD, Reason, dynamicLookupInfo);
+    if (!VD->hasClangNode() || isVisibleFromModule(ModuleFilter, VD))
+        NextConsumer.foundDecl(VD, Reason, dynamicLookupInfo);


Probably just run git-clang-formater will fix all formatting issues =]

LucianoPAlmeida · 2022-05-27T00:43:38Z

lib/ClangImporter/ImportDecl.cpp

+      );
+
+  topLevelStaticFuncDecl->setAccess(AccessLevel::Public);
+  topLevelStaticFuncDecl->setImplicit();


Woudn't createImplicit already set topLevelStaticFuncDecl as implicit already making this redundant?

Yes! Just checked

LucianoPAlmeida · 2022-05-27T00:46:31Z

lib/ClangImporter/ImportDecl.cpp

+      ctx,
+      StaticSpellingKind::None,
+      opDeclName, SourceLoc(),
+      false, false,


Suggested change

false, false,

/*Async=*/false, /*Throws=*/ false,

LucianoPAlmeida · 2022-05-27T01:02:31Z

lib/ClangImporter/ImportName.cpp

    case clang::OverloadedOperatorKind::OO_AmpAmp:
    case clang::OverloadedOperatorKind::OO_PipePipe:
-      baseName = clang::getOperatorSpelling(op);
+      baseName = isa<clang::CXXMethodDecl>(functionDecl) ? swiftCtx.getIdentifier("__operator" + std::string{getOperatorName(op)}).str() : swiftCtx.getIdentifier(clang::getOperatorSpelling(op)).str();


Nit: just a suggestion for simplification :)

Suggested change

baseName = isa<clang::CXXMethodDecl>(functionDecl) ? swiftCtx.getIdentifier("__operator" + std::string{getOperatorName(op)}).str() : swiftCtx.getIdentifier(clang::getOperatorSpelling(op)).str();

auto operatorName = isa<clang::CXXMethodDecl>(functionDecl) ? "__operator" + std::string{getOperatorName(op)} ? getOperatorSpelling(op);

baseName = swiftCtx.getIdentifier(operatorName).str();

baseName is defined before the switch (both the nested operator switch and outer switch) and used for all cases (not just operators)

Not sure I understand, the implication suggestion was just to do ternary on the argument of swiftCtx.getIdentifier, but it was just a nit...

Oh I see! I misread (on GH phone) I thought you were just renaming baseName to operatorName

Ya this looks neat! Thanks

Huddie · 2022-05-27T19:15:29Z

@swift-ci please test

Huddie · 2022-05-27T19:16:06Z

@zoecarver Ready for another pass!

egorzhdan · 2022-05-27T19:17:38Z

lib/ClangImporter/ClangImporter.cpp

  void foundDecl(ValueDecl *VD, DeclVisibilityKind Reason,
                 DynamicLookupInfo dynamicLookupInfo) override {
-    if (isVisibleFromModule(ModuleFilter, VD))
+    if (!VD->hasClangNode() || isVisibleFromModule(ModuleFilter, VD))


Why do we need !VD->hasClangNode() here?

Good question! Basically it can happen that we try looking up the pure swift function that we synthesize which doesn't have a clang node (since we add it as an altDecl to the original clang operator that we import). And if that happens it will call isVisibleFromModule which assume it has clangNode which our pure swift function does not have. So this guards against that.

It's when VD isn't something we imported, but something we found via alternativeDecls. I think it would be a good idea to add a comment saying that. Maybe also add a comment to the top of the consumer that says it only filters imported decls/decls that have an associated clang node.

Hmm, does that mean that operators are effectively visible from Swift even if their module isn't imported? E.g.

// Foo.h static MyClass operator+(MyClass a, MyClass b) { /* ... */ };

// A.swift @_implementationOnly import Foo

// B.swift import A // + shouldn't be visible here

Could we instead add a clang decl to the synthesized functions to avoid a special case here? That's how this is handled for synthesized subscripts.

@egorzhdan is that done in subscripts result->addMember(subscript) ?

This happens in SwiftDeclConverter::makeSubscript, SubscriptDecl *subscript is created with a getterImpl->getClangNode() parameter.

hmm.. I was using createImplicit which doesn't have that option. But maybe createImported which does would be better here? And pass in the clangNode? And then call ->setImplicit() after to make it implicit?@zoecarver ?

feels a bit weird just cause this function technically wasn't imported.

zoecarver · 2022-05-27T19:19:44Z

lib/ClangImporter/ImportDecl.cpp

+
+              Impl.addAlternateDecl(MD, opFuncDecl);
+
+              Impl.markUnavailable(


This formatting seems really weird.

zoecarver

There are still some formatting issues but please land this once those are fixed. The patch looks good to me.

Huddie · 2022-05-27T19:30:32Z

@swift-ci please test

zoecarver · 2022-05-31T16:59:04Z

@swift-ci please test

zoecarver · 2022-05-31T17:11:34Z

@swift-ci please test

egorzhdan

This looks great, other than a couple of minor code formatting issues. Thanks!

There is a potential issue when an operator is declared in a different module from the struct, however, that never worked even with the previous implementation and can be fixed later.

Huddie · 2022-06-01T18:41:16Z

@swift-ci please test

egorzhdan · 2022-06-01T18:41:55Z

@swift-ci please smoke test

egorzhdan · 2022-06-01T18:58:11Z

@swift-ci please smoke test

egorzhdan · 2022-06-01T19:02:17Z

@swift-ci please smoke test

zoecarver

🚀 it!

zoecarver · 2022-06-01T19:08:04Z

@swift-ci please test

Huddie · 2022-06-01T21:56:13Z

@swift-ci please test macos

Huddie added the c++ interop Feature: Interoperability with C++ label May 13, 2022

Huddie requested a review from zoecarver May 13, 2022 22:33

zoecarver reviewed May 16, 2022

View reviewed changes

LucianoPAlmeida reviewed May 18, 2022

View reviewed changes

Huddie and others added 9 commits May 18, 2022 10:29

start work on operators

88f7260

Add operators as class members

178b012

Add changes

b9f2196

Add changes

e14e2fb

Mapping decl base name to imported name

4267958

Remap class operator function names (-/+/*....) to imported operator …

11730e8

…names __operator(Minus, Plus,...) and fix test cases

Added synth member to result

bd4db51

Update Identifier.h

c36eeed

Update Identifier.h

540e52a

zoecarver requested a review from egorzhdan May 20, 2022 18:42

Fix tests and re-enable support for CXX operators

e7fe6f0

Huddie force-pushed the update-cxx-operator-impl branch from 33c7002 to e7fe6f0 Compare May 22, 2022 01:29

Refactor and format

3196ea9

Use Identifier instead of StringRef to avoid dangling pointer

65473f5

Huddie requested a review from LucianoPAlmeida May 26, 2022 16:26

zoecarver reviewed May 26, 2022

View reviewed changes

zoecarver approved these changes May 26, 2022

View reviewed changes

LucianoPAlmeida reviewed May 27, 2022

View reviewed changes

Huddie added 4 commits May 26, 2022 21:29

Format

16c1e1d

PR comment

238eec4

Test diff mangled name for windows

597668f

Keep the XFAIL on windows for now

2422c0c

egorzhdan reviewed May 27, 2022

View reviewed changes

zoecarver reviewed May 27, 2022

View reviewed changes

zoecarver approved these changes May 27, 2022

View reviewed changes

Manually fix some formatting

bb3a721

egorzhdan approved these changes Jun 1, 2022

View reviewed changes

Fix switch error + format

1d1df27

Huddie requested a review from LucianoPAlmeida June 1, 2022 18:44

Huddie added 2 commits June 1, 2022 14:54

Format

54ed81c

Add in newline

9aea433

Fix spacing

e7f8a99

zoecarver approved these changes Jun 1, 2022

View reviewed changes

zoecarver merged commit 4967cd5 into swiftlang:main Jun 2, 2022

egorzhdan mentioned this pull request Jun 8, 2022

[cxx-interop][SwiftCompilerSources] Use swift::SourceLoc instead of BridgedSourceLoc #58699

Merged

	? DeclBaseName(SwiftContext.getIdentifier("__operator" + getOperatorNameForToken(std::string{name.getBaseName().getIdentifier()})))
	? DeclBaseName(SwiftContext.getIdentifier("__operator" + getOperatorNameForToken(name.getBaseName().getIdentifier().str())))

	baseName = isa<clang::CXXMethodDecl>(functionDecl) ? swiftCtx.getIdentifier("__operator" + std::string{getOperatorName(op)}).str() : swiftCtx.getIdentifier(clang::getOperatorSpelling(op)).str();
	auto operatorName = isa<clang::CXXMethodDecl>(functionDecl) ? "__operator" + std::string{getOperatorName(op)} ? getOperatorSpelling(op);
	baseName = swiftCtx.getIdentifier(operatorName).str();

Conversation

Huddie commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Previous Impl

New Impl

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented May 16, 2022

Uh oh!

zoecarver commented May 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Huddie commented May 22, 2022

Uh oh!

Huddie commented May 24, 2022

Uh oh!

zoecarver commented May 25, 2022

Uh oh!

Huddie commented May 25, 2022

Uh oh!

Huddie commented May 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Huddie May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Huddie commented May 27, 2022

Uh oh!

Huddie commented May 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Huddie commented May 13, 2022 •

edited

Loading

Huddie May 27, 2022 •

edited

Loading

Huddie May 27, 2022 •

edited

Loading

Huddie commented Jun 1, 2022 •

edited

Loading