Use clang-style USRs for swift decls that are exposed to Objective C #7670

nathawes · 2017-02-21T18:10:22Z

We have an issue where the Swift symbols that are exported to ObjC via the generated ObjC header that the Swift compiler emits, have different USRs when indexed from swift & clang.
The swift side uses Swift-specific USRs while the clang side uses it’s own ObjC USRs, making these symbols and their references seem as if they are different symbols (e.g. for jump-to-definition, callers, etc).

This patch changes the USR generation for @objc swift symbols to use clang-style USRs, so there is a single USR cross-language.

This is a step towards resolving rdar://problem/16271632, but it doesn't yet 'namespace' the generated clang-style USRs with the swift module name (waiting on some clang changes). This is needed to avoid USR conflicts with symbols from different Swift frameworks.

nathawes · 2017-02-21T18:10:41Z

@swift-ci please smoke test

benlangmuir · 2017-02-21T18:21:44Z

lib/AST/Decl.cpp

+    return true;
+  } else if (checkParent) {
+    if (auto ctor = dyn_cast<ConstructorDecl>(VD)) {
+      // Check if we're overriding an initializer that is visible to obj-c


Why is this the only case that checks the parent?

This is the logic PrintAsObjC was using to decide whether to include a decl in the generated header – I just moved the function here to make sure USR generation and the header printer use the same logic. I assume 'checkParent' was added specifically for the case of still writing out < minRequiredAccess initializers that override a >= minRequiredAccess initializer.

benlangmuir · 2017-02-21T18:24:32Z

lib/AST/USRGeneration.cpp

+
+  auto Parent = D->getDeclContext()->getInnermostDeclarationDeclContext();
+  if (Parent && (!ShouldUseObjCName(Parent) || // parent should be visible too
+                    !D->getDeclContext()->isTypeContext() || // no local decls


Indentation is off.

benlangmuir · 2017-02-21T18:25:17Z

lib/AST/USRGeneration.cpp

+
+  if (const ValueDecl *VD = dyn_cast<ValueDecl>(D)) {
+    if (isa<EnumElementDecl>(VD))
+      return Parent;


Does an EnumElementDecl ever not have a parent?

Wasn't sure about invalid code, but it doesn't look like it – I'll change it to true ;-)

benlangmuir · 2017-02-21T18:27:20Z

test/IDE/print_usrs.swift

+  }
+
+  // CHECK: [[@LINE+1]]:14 c:objc(cs)ObjCClass1(cpy)typeComputed{{$}}
+  static var typeComputed: Int {


Does this actually show up as a class property in ObjC? I would have expected you'd need to say "class" instead of "static" (they're not the same).

It does at the moment, yeah:
SWIFT_CLASS_PROPERTY(@Property (nonatomic, class) NSInteger typeComputed;)

(NSInteger)typeComputed SWIFT_WARN_UNUSED_RESULT;

(void)setTypeComputed:(NSInteger)newValue;

nkcsgexi · 2017-02-21T18:48:21Z

include/swift/AST/SwiftNameTranslation.h

+  /// Returns true if the given value decl D is visible to ObjC of its
+  /// own accord (i.e. without considering its context)
+  bool isVisibleToObjC(const ValueDecl *VD, Accessibility minRequiredAccess,
+                     bool checkParent = true);


nitpick: bool should align with const in the previous line.

nkcsgexi · 2017-02-21T18:50:23Z

lib/AST/Decl.cpp

+  } else if (checkParent) {
+    if (auto ctor = dyn_cast<ConstructorDecl>(VD)) {
+      // Check if we're overriding an initializer that is visible to obj-c
+      if (auto parent = ctor->getOverriddenDecl())


do we care about getSatisfiedProtocolRequirements() here?

nkcsgexi · 2017-02-21T18:51:20Z

lib/AST/USRGeneration.cpp

+    OS << "@" << ObjCName;
+  } else {
+    llvm_unreachable("Unexpected value decl");
+    return true;


nitpick: redundant return here

nkcsgexi · 2017-02-21T18:54:49Z

lib/AST/USRGeneration.cpp

+  OS << clang::index::getUSRSpacePrefix();
+
+  if (D->getDeclContext()->isTypeContext()) {
+    auto *Parent = D->getDeclContext()->getAsNominalTypeOrNominalTypeExtensionContext();


these above two lines can be combined as:
if (auto *Parent = D->getDeclContext()->getAsNominalTypeOrNominalTypeExtensionContext())

nkcsgexi · 2017-02-21T18:57:30Z

lib/AST/USRGeneration.cpp

+
+  if (!Ident.empty())
+    return printObjCNameFragment(D, Ident.str(), OS);
+  if (Selector) {


assert(Selector) here. one and only one of Ident and Selector is valid.

Are there no cases (e.g. invalid code) where they'll both come back invalid?

For invalid code, we may get nameless value decls. Can we have an early exist in this function like if (VD->getName().empty) return false like the one in ide::printDeclUSR so that we know what's going on without ignoring the cases?

Sounds good!

I realize we do have similar forgiving check in SwiftSourceDocInfo.cpp. The reason there is that we may have preferred names that are non-translatable to ObjC names, for instance, when the argument counts do not match. However, these cases are unlikely to happen here because we don't have preferred names.

slavapestov · 2017-02-21T20:33:57Z

lib/AST/Decl.cpp

 }
+
+bool swift::objc_translation::
+isVisibleToObjC(const ValueDecl *VD, Accessibility minRequiredAccess,


Is there a better place for this than Decl.cpp?

We want the utilities to be inside libAST. Which file do you suggest?

I would suggest either moving it to some other file, in lib/IDE/, or making it a method on ValueDecl instead of a top-level function. Just feels weird to see 'swift::objc_translation' in this file.

Also how about 'isVisibleInBridgingHeader' instead of 'isVisibleToObjC'?

Bridging header is the other direction (ObjC code visible in Swift).

It's generally preferable for headers to have an associated implementation file, so SwiftNameTranslation.h can have SwiftNameTranslation.cpp for its code, it doesn't have to go to Decl.cpp

I like @akyrtzi's suggest the best.

… own file. Also fix code formatting issues and simplify the code in USRGeneration.cpp based on review comments in PR swiftlang#7670.

nathawes · 2017-02-21T23:16:06Z

@swift-ci please smoke test

nathawes · 2017-02-21T23:22:27Z

@swift-ci please smoke test

nkcsgexi · 2017-02-21T23:38:15Z

lib/AST/SwiftNameTranslation.cpp

+                bool checkParent) {
+  if (!(VD->isObjC() || VD->getAttrs().hasAttribute<CDeclAttr>()))
+    return false;
+  if (VD->getFormalAccess() >= minRequiredAccess) {


can we check VD->hasAccessibility() before getting it? invalid code usually has this trap.

You mean hasAccessibility()?

…hasAccessibility() check.

nathawes · 2017-02-22T00:10:52Z

@swift-ci please smoke test

Use clang-style USRs for swift decls that are exposed to Objective C

c9758c2

nathawes requested review from akyrtzi and benlangmuir February 21, 2017 18:10

benlangmuir reviewed Feb 21, 2017

View reviewed changes

nkcsgexi reviewed Feb 21, 2017

View reviewed changes

slavapestov reviewed Feb 21, 2017

View reviewed changes

Move SwiftNameTranslation implementation out of Decl.cpp and into its…

0dc0985

… own file. Also fix code formatting issues and simplify the code in USRGeneration.cpp based on review comments in PR swiftlang#7670.

Fix bad indentation in SwiftNameTranslation.h

be75764

nkcsgexi reviewed Feb 21, 2017

View reviewed changes

SwiftNameTranslation.cpp: Guard call to VD->getFormalAccess() with a …

0223be2

…hasAccessibility() check.

nathawes merged commit 444775f into swiftlang:master Feb 22, 2017

Use clang-style USRs for swift decls that are exposed to Objective C #7670

Use clang-style USRs for swift decls that are exposed to Objective C #7670

Uh oh!

Conversation

nathawes commented Feb 21, 2017

Uh oh!

nathawes commented Feb 21, 2017

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!

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!

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!

nathawes commented Feb 21, 2017

Uh oh!

nathawes commented Feb 21, 2017

Uh oh!

nkcsgexi Feb 21, 2017 • 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!

nathawes commented Feb 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nkcsgexi Feb 21, 2017 •

edited

Loading