[lldb] Implement TypeSystemSwiftTypeRef::IsPossibleDynamicType #2232

kastiglione · 2020-12-11T23:44:46Z

Implement TypeSystemSwiftTypeRef::IsPossibleDynamicType to match the SwiftASTContext implementation.

rdar://68170302

kastiglione · 2020-12-11T23:53:20Z

This is written to match SwiftASTContext. I haven't run tests yet, I am publishing a draft because I have some questions.

The SwiftASTContext implementation is here: https://github.com/apple/llvm-project/blob/db5bc1d58bf76a0ebe709275711317ef6c9b52bb/lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp#L5161-L5179

I'm not entirely sure how to translate swift::CanType predicates to swift::Demangle::Node. Hopefully the tests will identify gaps. But there are a couple of questions I have:

I don't know how to translate hasArchetype() and hasOpaqueArchetype()
Some node kinds might be relevant, but I am not sure how to know, for example AnyProtocolConformanceList.

I have other questions too, ex is ContainsGenericTypeParameter() equivalent to hasTypeParameter()? Hopefully tests will cover everything, but I'd also like to know how to know each of the two sides (swift types vs demangling/reflection) well enough to translate expressions between the two.

adrian-prantl

Very nice! I guess we could add unit tests for the cases in the switch if we want to.

adrian-prantl · 2020-12-11T23:52:31Z

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

-  return m_swift_ast_context->IsPossibleDynamicType(
-      ReconstructType(type), target_type, check_cplusplus, check_objc);
+  if (target_type)
+    target_type->Clear();


Side note: This is a good example of some of LLDB's less than great APIs. I'm sure we're not very consistent in whether this should get cleared or not in the failure case...

In SwiftASTContext it is cleared unconditionally.

adrian-prantl · 2020-12-11T23:54:23Z

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

+    case Node::Kind::BuiltinTypeName: {
+      if (!node->hasText())
+        return false;
+      auto name = node->getText();


Would you mind spelling the type out here, so it's obvious that it isn't a char*? Otherwise the comparison below looks scary :-)

adrian-prantl · 2020-12-11T23:54:51Z

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

+      return true;
+    case Node::Kind::BuiltinTypeName: {
+      if (!node->hasText())
+        return false;


Side note: And here we should have an error channel ...

by changing the return type of this function? or by logging?

This was just me rambling — you didn't design this API :-)

It would be best if all CompilerType operations had a way to signal an error case, so we aren't silently ignoring errors. Either by returning Optionals, Expected, and for the functions that return a new type, potentially even a special Error type in a special Error TypeSystem, so the operations still compose nicely.

I totally agree.

adrian-prantl · 2020-12-12T00:00:21Z

I don't know how to translate hasArchetype() and hasOpaqueArchetype()

Surprise! HasArchetype() is probably always false. We should only see generic type parameters, but no Archetypes any more. I hope there is a testcase that breaks for the opaque archetype case.

Some node kinds might be relevant, but I am not sure how to know, for example AnyProtocolConformanceList.

You could try and see what the type Proto1 & Proto2 mangles to.

I have other questions too, ex is ContainsGenericTypeParameter() equivalent to hasTypeParameter()? Hopefully tests will cover everything, but I'd also like to know how to know each of the two sides (swift types vs demangling/reflection) well enough to translate expressions between the two.

These two should be identical.

adrian-prantl · 2020-12-12T00:01:41Z

In func f<T>(_ t: T) {}

T: Archetype
tau_0_0: generic type parameter

kastiglione · 2020-12-12T00:46:06Z

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

+    case Node::Kind::TypeAlias: {
+      if (node->getNumChildren() == 2) {
+        auto *module = node->getFirstChild();
+        auto *identifier = node->getLastChild();
+        return module->getKind() == Node::Kind::Module &&
+               identifier->getKind() == Node::Kind::Identifier &&
+               module->getText() == "Swift" &&
+               identifier->getText() == "AnyObject";


I don't like this. $ss9AnyObjectaD caused a failure, and this block handles that specific case. I will investigate a better solution, probably a way to resolve the alias.

I have found AnyObject to be special before, too. Maybe this comes from SwiftASTContext but if it's in the compiler we probably just have to accept it.

I ended up replacing this with ResolveTypeAlias in 26fa282. Hopefully that's more reasonable.

adrian-prantl · 2020-12-12T00:48:27Z

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

+        auto *identifier = node->getLastChild();
+        return module->getKind() == Node::Kind::Module &&
+               identifier->getKind() == Node::Kind::Identifier &&
+               module->getText() == "Swift" &&


nitpick: theres a swift::something_STDLIB_NAME constant in swift/strings.h that we should use here

kastiglione · 2020-12-12T01:16:22Z

@swift-ci test

kastiglione · 2020-12-14T17:04:42Z

It looks like this will have to go back to an explicit check for Swift.AnyObject type aliases (by reverting 26fa282).

The test failure is: $sSo18NSNotificationNameaD:

kind=Global
  kind=TypeMangling
    kind=Type
      kind=TypeAlias
        kind=Module, text="__C"
        kind=Identifier, text="NSNotificationName"

When resolving this alias, the result is a class (typedef NSString *NSNotificationName), and class types make IsPossibleDynamicType return true. However the SwiftASTContext implementation returns false.

adrian-prantl · 2020-12-14T17:20:08Z

It looks like this will have to go back to an explicit check for Swift.AnyObject type aliases (by reverting 26fa282).

The test failure is: $sSo18NSNotificationNameaD:
kind=Global
  kind=TypeMangling
    kind=Type
      kind=TypeAlias
        kind=Module, text="__C"
        kind=Identifier, text="NSNotificationName"
When resolving this alias, the result is a class (typedef NSString *NSNotificationName), and class types make IsPossibleDynamicType return true. However the SwiftASTContext implementation returns false.

Side note: There are a few Foundation types that are special-cased in ClangImporter and thus keep creating inconsistencies like this. I believe that this is on of them. You'll find mentions of it elsewhere, too.

In the overlay it is:

public typealias Name = NSNotification.Name

and as you said it points to just an NSString.I think we should return true here (since it is an ObjC class) and ignore this particular type in the comparison.

kastiglione · 2020-12-14T18:32:13Z

I think we should return true here (since it is an ObjC class) and ignore this particular type in the comparison.

I wondered along these, whether this behavior is better.

It's an objc class (NSString), but a swift struct (String)
Similarly, in Swift Notification.Name is an alias for NSNotification.Name which in swift is a struct
Even when saying "this is a class", is there anything dynamic lldb needs to do? Intuitively I would say no, but maybe I'm wrong

These all lean toward saying the type is not dynamic, but I am just as fine doing what you said, and returning true.

adrian-prantl · 2020-12-14T18:37:57Z

I think we should return true here (since it is an ObjC class) and ignore this particular type in the comparison.

I wondered along these, whether this behavior is better.

It's an objc class (NSString), but a swift struct (String)

That's a good observation.

Similarly, in Swift Notification.Name is an alias for NSNotification.Name which in swift is a struct

Even when saying "this is a class", is there anything dynamic lldb needs to do? Intuitively I would say no, but maybe I'm wrong

These all lean toward saying the type is not dynamic, but I am just as fine doing what you said, and returning true.

Generally speaking, ObjC classes are dynamic, but NSString is special, because it is bridged to a Swift struct type.

kastiglione · 2020-12-14T18:49:16Z

@swift-ci test

kastiglione · 2020-12-14T18:50:41Z

I think we should return true here (since it is an ObjC class) and ignore this particular type in the comparison.

I went with this approach. The ignoring is done in 8552c56.

[lldb] Implement TypeSystemSwiftTypeRef::IsPossibleDynamicType

cd9a7fc

kastiglione marked this pull request as draft December 11, 2020 23:44

kastiglione requested a review from adrian-prantl December 11, 2020 23:45

adrian-prantl approved these changes Dec 11, 2020

View reviewed changes

kastiglione added 3 commits December 11, 2020 16:26

use precise/non-ambiguous type name instead of auto

6ef3f67

add ProtocolListWithAnyObject

4beccfb

handle TypeAlias to Swift.AnyObject

1122b69

kastiglione commented Dec 12, 2020

View reviewed changes

adrian-prantl reviewed Dec 12, 2020

View reviewed changes

kastiglione added 2 commits December 11, 2020 17:13

add ExistentialMetatype

e97417a

resolve type aliases

26fa282

kastiglione added 2 commits December 14, 2020 10:48

add special case handling of NSNotificationName

8552c56

use std::get<T>

f8d2fb2

kastiglione marked this pull request as ready for review December 14, 2020 18:50

kastiglione mentioned this pull request Dec 14, 2020

[lldb] Implement TypeSystemSwiftTypeRef::IsPossibleDynamicType #2238

Merged

kastiglione merged commit c37c368 into swift/main Dec 14, 2020

kastiglione deleted the lldb-Implement-TypeSystemSwiftTypeRef-IsPossibleDynamicType branch December 14, 2020 23:21

[lldb] Implement TypeSystemSwiftTypeRef::IsPossibleDynamicType #2232

[lldb] Implement TypeSystemSwiftTypeRef::IsPossibleDynamicType #2232

Uh oh!

Conversation

kastiglione commented Dec 11, 2020

Uh oh!

kastiglione commented Dec 11, 2020

Uh oh!

adrian-prantl 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!

adrian-prantl commented Dec 12, 2020

Uh oh!

adrian-prantl commented Dec 12, 2020

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!

kastiglione commented Dec 12, 2020

Uh oh!

kastiglione commented Dec 14, 2020

Uh oh!

adrian-prantl commented Dec 14, 2020

Uh oh!

kastiglione commented Dec 14, 2020

Uh oh!

adrian-prantl commented Dec 14, 2020

Uh oh!

kastiglione commented Dec 14, 2020

Uh oh!

kastiglione commented Dec 14, 2020

Uh oh!

Uh oh!