Eng/pr getters setters #40842

bro4all · 2022-01-13T17:12:40Z

In this patch, we add the ability to import struct getters/setters

example:

struct X {
    int getX() const {}
    void setX(int) {}
};

// will be:
var Object = X()
Object.x = 1

bro4all · 2022-01-13T17:15:37Z

@swift-ci please smoke test.

zoecarver

Hey Omar! This patch looks great! I'm super excited to start importing these methods more ergonomically!

Don't be overwhelmed by the comments, the logic actually looks pretty good, so most of these are just nits. Please add some test cases as well.

Also, I think we may need a forum post for this patch at some point. But we can probably get to that later.

zoecarver · 2022-01-14T05:31:47Z

lib/ClangImporter/ImportDecl.cpp

+Expr *createSelfExpr(AccessorDecl *accessorDecl);
+DeclRefExpr *createParamRefExpr(AccessorDecl *accessorDecl, unsigned index);
+CallExpr * createAccessorImplCallExpr(FuncDecl *accessorImpl,
+                           Expr *selfExpr,
+                           DeclRefExpr *keyRefExpr = nullptr);


Remove these forward declarations.

lib/ClangImporter/ImportDecl.cpp

zoecarver · 2022-01-14T05:32:48Z

lib/ClangImporter/ImporterImpl.h

@@ -499,7 +499,7 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
  /// Keep track of subscript declarations based on getter/setter
  /// pairs.
  llvm::DenseMap<std::pair<FuncDecl *, FuncDecl *>, SubscriptDecl *> Subscripts;
-
+  llvm::DenseMap<Identifier, std::tuple<VarDecl *, AccessorDecl *, AccessorDecl *>> GetterSetterMap;


Please add a comment to this like the one for Subscripts.

Also, I wonder if there's a better way to hold the values than a tuple.

zoecarver · 2022-01-14T05:34:03Z

lib/ClangImporter/ImportDecl.cpp

@@ -3577,7 +3587,9 @@ namespace {
        if (auto structResult = dyn_cast<StructDecl>(result))
          structResult->setIsCxxNonTrivial(
              !cxxRecordDecl->isTriviallyCopyable());
-
+        for (auto &getterAndSetter : Impl.GetterSetterMap){


This should be called computedProperties or something.

zoecarver · 2022-01-14T05:34:24Z

lib/ClangImporter/ImportDecl.cpp

@@ -3577,7 +3587,9 @@ namespace {
        if (auto structResult = dyn_cast<StructDecl>(result))
          structResult->setIsCxxNonTrivial(
              !cxxRecordDecl->isTriviallyCopyable());
-
+        for (auto &getterAndSetter : Impl.GetterSetterMap){
+            result->addMember(get<0>(getterAndSetter.second));


If we can't get rid of the tuple, please add a comment explaining that this is the vardecl/property itself.

zoecarver · 2022-01-14T05:36:54Z

lib/ClangImporter/ImportDecl.cpp

+
+
+
+      Decl *VisitCXXMethodDecl(const clang::CXXMethodDecl *decl) {


I think you can clean up and refactor this whole function.

lib/ClangImporter/ImportDecl.cpp

zoecarver · 2022-01-14T05:37:55Z

lib/ClangImporter/ImportDecl.cpp

@@ -7568,7 +7741,14 @@ createAccessorImplCallExpr(FuncDecl *accessorImpl,
  accessorImplDotCallExpr->setType(accessorImpl->getMethodInterfaceType());
  accessorImplDotCallExpr->setThrows(false);

-  auto *argList = ArgumentList::forImplicitUnlabeled(ctx, {keyRefExpr});
+  ArgumentList *argList;
+  if (keyRefExpr) {


zoecarver · 2022-01-14T05:38:18Z

lib/ClangImporter/ImportDecl.cpp

@@ -7568,7 +7741,14 @@ createAccessorImplCallExpr(FuncDecl *accessorImpl,
  accessorImplDotCallExpr->setType(accessorImpl->getMethodInterfaceType());
  accessorImplDotCallExpr->setThrows(false);

-  auto *argList = ArgumentList::forImplicitUnlabeled(ctx, {keyRefExpr});
+  ArgumentList *argList;


Nit: please initialize this to nullptr.

zoecarver · 2022-01-14T05:38:48Z

lib/ClangImporter/ImportDecl.cpp

@@ -8930,6 +9110,18 @@ ClangImporter::Implementation::importDeclImpl(const clang::NamedDecl *ClangDecl,
    Result = importDecl(UnderlyingDecl, version);
    SkippedOverTypedef = true;
  }
+  // TODO extend Swift classes to it's equivalents from it's ClanDecled attributes


I don't understand this TODO.

zoecarver · 2022-01-14T05:43:28Z

I'd specifically like to see some tests that check for the following cases:

void getX();
void get();
int getX();
int get();
int getfoo();
int getFOO();
int GetFoo();
int Getter();

And

void setX();
void set();
int setX(int);
void setX(int, int);
void set(int);
...

And at least one test when we have a setter but no getter.

egorzhdan

This looks great! Other than what Zoe already mentioned, I have a few ideas for test cases:

int& getInt() const;
void setInt(int);
// I don't know if these should be transformed into a computed property, I think, for now we should just make sure it doesn't crash :)

int& getInt2() const;
void setInt2(int*);
// Same as above.

void settings(int);
void setUp(int);
// These should not be transformed into a computed property.

int getOrCreateSomething() const;
// This would probably be transformed into something awkward (`var orCreateSomething: Int`), and I think it's fine.
// It would be nice to still be able to call the getters or setters explicitly
// (in this case, `let x = object.getOrCreateSomething()` instead of `let x = object.orCreateSomething`)
// I think it's worth having a test for this.

bro4all · 2022-01-16T06:17:30Z

Another test case: make sure that it doesn't matter which order getters/setters are defined in (i.e., you can define the setter first).

bro4all · 2022-01-16T06:18:41Z

It would be nice to still be able to call the getters or setters explicitly

Yes, I think we should always continue to import the original method. Maybe we want to add a deprecation, though. I think this happens for some Objective-C enums. Maybe something for the forums to decide.

bro4all · 2022-01-16T06:20:02Z

lib/ClangImporter/ImportDecl.cpp

@@ -3577,7 +3587,9 @@ namespace {
        if (auto structResult = dyn_cast<StructDecl>(result))
          structResult->setIsCxxNonTrivial(
              !cxxRecordDecl->isTriviallyCopyable());
-
+        for (auto &getterAndSetter : Impl.GetterSetterMap){


These VarDecls will be added to every record.

bro4all · 2022-02-10T17:15:08Z

@swift-ci please smoke test

bro4all · 2022-02-10T17:38:09Z

@swift-ci please smoke test

hyp · 2022-02-11T04:07:55Z

@swift-ci please smoke test

lib/ClangImporter/ImportDecl.cpp

bro4all · 2022-02-16T21:58:30Z

@swift-ci please smoke test

zoecarver · 2022-02-25T16:42:18Z

lib/ClangImporter/ImportDecl.cpp

@@ -3507,6 +3571,9 @@ namespace {
          continue;
        }

+        if(nd->getDeclName().isIdentifier())


zoecarver · 2022-02-25T16:43:02Z

lib/ClangImporter/ImportDecl.cpp

@@ -4308,9 +4404,53 @@ namespace {

      recordObjCOverride(result);
    }


Nit: new line.

zoecarver · 2022-02-25T16:45:02Z

lib/ClangImporter/ImportDecl.cpp

+      setterDecl->setSelfAccessKind(SelfAccessKind::Mutating);
+      result->setIsSetterMutating(true);
+    } else {
+        setterDecl->setSelfAccessKind(SelfAccessKind::NonMutating);


nit: indentation.

zoecarver · 2022-02-25T16:45:21Z

lib/ClangImporter/ImportDecl.cpp

@@ -9027,6 +9184,25 @@ ClangImporter::Implementation::importDeclImpl(const clang::NamedDecl *ClangDecl,
    Result = importDecl(UnderlyingDecl, version);
    SkippedOverTypedef = true;
  }
+  // TODO extend Swift classes to it's equivalents from it's ClanDecled


What does this mean?

zoecarver · 2022-02-25T16:45:36Z

lib/ClangImporter/ImportDecl.cpp

+  // TODO extend Swift classes to it's equivalents from it's ClanDecled
+  // attributes
+
+  if (isa<clang::FunctionDecl>(ClangDecl)) {
+    if (ClangDecl->getDeclName().isIdentifier() &&
+        ClangDecl->getName().startswith_insensitive("get") &&
+        ClangDecl->getName().lower() != "getter") {
+      const_cast<clang::NamedDecl *>(ClangDecl)->addAttr(
+          clang::SwiftAttrAttr::CreateImplicit(ClangDecl->getASTContext(),
+                                               "import_as_getter"));
+    }
+    if (ClangDecl->getDeclName().isIdentifier() &&
+        ClangDecl->getName().startswith_insensitive("set") &&
+        ClangDecl->getName().lower() != "setter") {
+      const_cast<clang::NamedDecl *>(ClangDecl)->addAttr(
+          clang::SwiftAttrAttr::CreateImplicit(ClangDecl->getASTContext(),
+                                               "import_as_setter"));
+    }
+  }


This can all be removed.

test/Interop/Cxx/ergonomics/Inputs/implicit-computed-properties.h

zoecarver · 2022-02-25T16:48:55Z

test/Interop/Cxx/ergonomics/implicit-computed-properties.swift

+    expectEqual(a.foo, 42)
+    a.foo = 32
+    expectEqual(a.foo, 32)
+


test/Interop/Cxx/ergonomics/implicit-computed-properties.swift

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift

zoecarver · 2022-02-25T16:52:54Z

include/swift/ClangImporter/CXXMethodBridging.h

+            return Kind::getter;
+        }
+
+        rdar://89453187 (Add subscripts clarification to CXXMethod Bridging to clean up importDecl)


zoecarver · 2022-02-25T16:54:10Z

include/swift/ClangImporter/CXXMethodBridging.h

+        // this should be handled as snake case. See: rdar://89453010
+        // case. In the future we could
+        //  import these too, though.


Nit:

Suggested change

// this should be handled as snake case. See: rdar://89453010

// case. In the future we could

// import these too, though.

// TODO: this should be handle snake case too (rdar://89453010).

zoecarver · 2022-02-25T16:55:25Z

test/Interop/Cxx/ergonomics/Inputs/implicit-computed-properties.h

+  void setX(int);
+};
+
+struct VoidGetterNoName {


VoidSetterNoName

zoecarver · 2022-02-25T16:55:48Z

test/Interop/Cxx/ergonomics/Inputs/implicit-computed-properties.h

+struct VoidGetterNoName {
+  void set();
+};
+
+struct IllegalIntReturnSetter {
+  int setX(int);
+};
+
+struct TwoParameterSetter {
+  void setX(int, int);
+};
+
+struct NoNameSetter {
+  void set(int);
+};


These all need correct getters. Otherwise they're all testing the same thing.

test/Interop/Cxx/ergonomics/Inputs/implicit-computed-properties.h

zoecarver · 2022-02-25T17:00:59Z

test/Interop/Cxx/ergonomics/Inputs/implicit-computed-properties.h

-  int setFOO(int v) const {}
+    mutable int value = 42;
+  int getFOO() const { return value; }
+  void setFOO(int v) const {value = v ;}
 };

 struct LongNameMix {


LongNameMix doesn't really make sense here. This shows the need for two different tests:

struct UpperCaseMix { int getFoo() const; void SetFoo(int); }

And:

struct UpperCaseGetterSetter { ... }

zoecarver · 2022-02-25T17:02:47Z

include/swift/ClangImporter/CXXMethodBridging.h

+            return Kind::getter;
+        }
+
+        rdar://89453187 (Add subscripts clarification to CXXMethod Bridging to clean up importDecl)


Suggested change

rdar://89453187 (Add subscripts clarification to CXXMethod Bridging to clean up importDecl)

// TODO: classify subscripts here (rdar://89453187).

zoecarver · 2022-02-25T17:03:36Z

It's worrying to see any tests passing... this shouldn't compile.

zoecarver · 2022-02-25T17:06:07Z

@egorzhdan want to take another look over this?

General formatting.

zoecarver · 2022-02-28T23:42:19Z

@swift-ci please test.

zoecarver · 2022-02-28T23:42:51Z

include/swift/ClangImporter/CXXMethodBridging.h

+//===----------------------------------------------------------------------===//
+//
+//  This file implements IR generation for value witnesses in Swift.
+//
+//  Value witnesses are (predominantly) functions that implement the basic
+//  operations for copying and destroying values.
+//
+//  In the comments throughout this file, three type names are used:
+//    'B' is the type of a fixed-size buffer
+//    'T' is the type which implements a protocol
+//
+//===----------------------------------------------------------------------===//


Sorry can you delete this or change it?

zoecarver · 2022-02-28T23:51:45Z

@swift-ci please test.

bro4all force-pushed the eng/PR-getters-setters branch from 2c99890 to 4c36d52 Compare January 13, 2022 17:14

bro4all requested a review from zoecarver January 13, 2022 17:14

zoecarver reviewed Jan 14, 2022

View reviewed changes

zoecarver requested review from egorzhdan, hyp and guitard0g January 14, 2022 05:40

zoecarver added the c++ interop Feature: Interoperability with C++ label Jan 14, 2022

egorzhdan reviewed Jan 14, 2022

View reviewed changes

bro4all commented Jan 16, 2022

View reviewed changes

bro4all force-pushed the eng/PR-getters-setters branch from 4c36d52 to b8b9478 Compare January 28, 2022 07:45

bro4all force-pushed the eng/PR-getters-setters branch from 28c3a07 to fa5ca21 Compare February 10, 2022 17:37

bro4all requested review from zoecarver and egorzhdan February 10, 2022 17:39

egorzhdan reviewed Feb 14, 2022

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Show resolved Hide resolved

bro4all force-pushed the eng/PR-getters-setters branch from fa5ca21 to 6ae41ad Compare February 15, 2022 21:27

Omar Habra added 3 commits February 15, 2022 13:36

fixing setters

21cc043

adding tests

ed168d9

Removing extras

659d4d5

bro4all force-pushed the eng/PR-getters-setters branch from 6ae41ad to 659d4d5 Compare February 15, 2022 21:37

fixing tests

d57c548

bro4all requested a review from egorzhdan February 17, 2022 18:49

Better naming for properties

4554ad0

zoecarver reviewed Feb 25, 2022

View reviewed changes

lib/ClangImporter/ImportDecl.cpp

@@ -4308,9 +4404,53 @@ namespace {

recordObjCOverride(result);

}

Copy link

Contributor

zoecarver Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: new line.

zoecarver reviewed Feb 25, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/Inputs/implicit-computed-properties.h Show resolved Hide resolved

zoecarver reviewed Feb 25, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties.swift Show resolved Hide resolved

zoecarver reviewed Feb 25, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties.swift Show resolved Hide resolved

zoecarver reviewed Feb 25, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties.swift Show resolved Hide resolved

zoecarver reviewed Feb 25, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/implicit-computed-properties-module-interface.swift Show resolved Hide resolved

zoecarver reviewed Feb 25, 2022

View reviewed changes

test/Interop/Cxx/ergonomics/Inputs/implicit-computed-properties.h Show resolved Hide resolved

zoecarver reviewed Feb 25, 2022

View reviewed changes

Omar Habra added 2 commits February 25, 2022 13:17

Fixing Comments left on the PR:

1e1bbb0

General formatting.

Fixing tests, and general updates for formatting

8ac79bf

zoecarver reviewed Feb 28, 2022

View reviewed changes

zoecarver approved these changes Feb 28, 2022

View reviewed changes

removing extras and passing this on swift.

6ef20b8

bro4all merged commit 4167e2f into swiftlang:main Mar 1, 2022

cabmeurer mentioned this pull request Mar 2, 2022

[cxx-interop] Update CxxMethodBridging with snake_case #41617

Closed

cabmeurer mentioned this pull request Mar 19, 2022

[cxx-interop] Add snake_case to CXXMethodBridging #41905

Merged




		Decl VisitCXXMethodDecl(const clang::CXXMethodDecl decl) {

		@@ -4308,9 +4404,53 @@ namespace {

		recordObjCOverride(result);
		}

	rdar://89453187 (Add subscripts clarification to CXXMethod Bridging to clean up importDecl)
	// TODO: classify subscripts here (rdar://89453187).

Eng/pr getters setters #40842

Eng/pr getters setters #40842

Uh oh!

Conversation

bro4all commented Jan 13, 2022

Uh oh!

bro4all commented Jan 13, 2022

Uh oh!

zoecarver left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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 Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

egorzhdan left a comment

Choose a reason for hiding this comment

Uh oh!

bro4all commented Jan 16, 2022

Uh oh!

bro4all commented Jan 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bro4all commented Feb 10, 2022

Uh oh!

bro4all commented Feb 10, 2022

Uh oh!

hyp commented Feb 11, 2022

Uh oh!

Uh oh!

bro4all commented Feb 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoecarver commented Jan 14, 2022 •

edited

Loading