[lldb] Fix typealias not being emitted for weakly captured generic #7526

augusto2112 · 2023-09-27T18:43:15Z

The typealias to $__lldb_context (the self type) was not being emitted when self was weakly captured and the type of self was a generic class type. Fix this, and also make AddRequiredAliases return an error code in case it was unable to emit one of the required aliases.

rdar://104697539

augusto2112 · 2023-09-27T18:43:22Z

@swift-ci test

augusto2112 · 2023-09-27T20:27:41Z

macOS test failures seem to be unrelated.

augusto2112 · 2023-10-03T17:40:22Z

Ping :)

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

adrian-prantl · 2023-10-03T20:15:29Z

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

@@ -532,7 +532,7 @@ lldb::VariableSP SwiftExpressionParser::FindSelfVariable(Block *block) {
  return variable_list_sp->FindVariable(ConstString("self"));
 }

-static void AddRequiredAliases(Block *block, lldb::StackFrameSP &stack_frame_sp,
+static bool AddRequiredAliases(Block *block, lldb::StackFrameSP &stack_frame_sp,


Can you document what the return value means?

Should you instead return a Status that contains an error message about what failed?

I added the error messages, but I wonder if having them is better than a generic error. They aren't really actionable from the user's point of view.

@PortalPete what do you think?

I generally think more information is helpful to the community. The error messages certainly help me understand what the code is doing a bit better.

My answer hinges on what you mean by "actionable" and who could take action. Even if the message isn't about the developer's code that they're debugging, it could potentially inform them it's not responsible for the issue at hand.

PortalPete

Hi Augusto,

As my editor said to me on my first day as a tech writer, "You're going to see a lot of red, and that's ok!"
In this case it's actually green, but you get the idea. 🙂

I'd start with this one on line 535 (ish)
because it's the most interesting.
I know Stash sometimes mixes up the order of things in the Conversation tab.

The rest are just classic fixes for active voice and/or present tense, and you'll see the wording patterns right away.

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

PortalPete · 2023-10-03T23:37:59Z

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

-
-    if (!self_class_type)
-      return;
+    // Only class types can be weakly captured.


Suggested change

// Only class types can be weakly captured.

// The Swift expression parser can only weakly capture class types.

In this case it's not the Swift expression parser, it's in the rules of the Swift language that only a class type can be weakly captured. Also, "strongly captured" and "weakly captured" are already part of the Swift lingo.

I'll change it to:

// In Swift only class types can be weakly captured.

PortalPete · 2023-10-03T23:39:22Z

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

+    // Only class types can be weakly captured.
+    if (!llvm::isa<swift::ClassType>(first_arg_type) &&
+        !llvm::isa<swift::BoundGenericClassType>(first_arg_type))
+      return Status("Could not add required aliases: weakly captured type is "


Suggested change

return Status("Could not add required aliases: weakly captured type is "

return Status("Unable to add the aliases the expression needs because "

If the word "Unable" isn't a valid option, then I suggest the contraction "Couldn't".

Since "weakly captured" is already part of the Swift lingo I think we should keep it, maybe:

"Unable to add the aliases the expression needs because weakly captured type is not a class type"?

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

PortalPete

A found a few more things that I missed yesterday.

Other than that, I noticed there's a few cases of self inside backticks and some more without. I suggest choosing one way for consistency.

Personally, I prefer `self` with backticks because it refers to the language's keyword as opposed to the normal English word self.

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

lldb/test/API/lang/swift/expression/weak_generic_self/TestSwiftWeakGenericSelf.py

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

augusto2112 · 2023-10-05T16:36:38Z

A found a few more things that I missed yesterday.

Other than that, I noticed there's a few cases of self inside backticks and some more without. I suggest choosing one way for consistency.

Personally, I prefer `self` with backticks because it refers to the language's keyword as opposed to the normal English word self.

Hmm, I think we should do it without backticks because:

We don't use backticks on error messages anywhere else.
This will be printed in the console, backticks won't change the formatting of self.

The typealias to $__lldb_context (the self type) was not being emitted when self was weakly captured and the type of self was a generic class type. Fix this, and also make AddRequiredAliases return an error code in case it was unable to emit one of the required aliases. rdar://104697539

augusto2112 requested review from kastiglione and adrian-prantl September 27, 2023 18:43

kastiglione reviewed Oct 3, 2023

View reviewed changes

adrian-prantl reviewed Oct 3, 2023

View reviewed changes

augusto2112 force-pushed the weak-generic-class branch from 900b25b to 1923951 Compare October 3, 2023 21:33

PortalPete reviewed Oct 3, 2023

View reviewed changes

augusto2112 force-pushed the weak-generic-class branch from 1923951 to b617a1c Compare October 4, 2023 23:53

PortalPete approved these changes Oct 5, 2023

View reviewed changes

PortalPete reviewed Oct 5, 2023

View reviewed changes

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp Outdated Show resolved Hide resolved

PortalPete reviewed Oct 5, 2023

View reviewed changes

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp Outdated Show resolved Hide resolved

PortalPete approved these changes Oct 5, 2023

View reviewed changes

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp Outdated Show resolved Hide resolved

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp Outdated Show resolved Hide resolved

augusto2112 force-pushed the weak-generic-class branch from b617a1c to 6fb5c0f Compare October 5, 2023 16:46

augusto2112 merged commit 7f8a88f into swiftlang:stable/20230725 Oct 5, 2023

	// Only class types can be weakly captured.
	// The Swift expression parser can only weakly capture class types.

	return Status("Could not add required aliases: weakly captured type is "
	return Status("Unable to add the aliases the expression needs because "

[lldb] Fix typealias not being emitted for weakly captured generic #7526

[lldb] Fix typealias not being emitted for weakly captured generic #7526

Uh oh!

Conversation

augusto2112 commented Sep 27, 2023

Uh oh!

augusto2112 commented Sep 27, 2023

Uh oh!

augusto2112 commented Sep 27, 2023

Uh oh!

augusto2112 commented Oct 3, 2023

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!

Choose a reason for hiding this comment

Uh oh!

PortalPete left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

PortalPete left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

augusto2112 commented Oct 5, 2023

Uh oh!

Uh oh!