Added unimplemented expression cases in the ASTPrinter #64667

Rajveer100 · 2023-03-28T10:49:18Z

Part of #61601

Filled out the remaining expression cases for ASTPrinter, which were left out after in the PR #40691.

Rajveer100 · 2023-03-28T10:50:53Z

@hamishknight I have implemented one of the functions for you to review if I have done it the right way.

hamishknight

Thanks for taking this on!

lib/AST/ASTPrinter.cpp

Rajveer100 · 2023-03-31T10:05:46Z

How would visitTypeJoinExpr, visitClosureExpr, ... be handled?
(Many more which I am figuring...)

hamishknight · 2023-04-03T13:15:13Z

How would visitTypeJoinExpr, visitClosureExpr, ... be handled?

Sorry, I shouldn't have included TypeJoinExpr and OneWayExpr in the list, they only exist during constraint solving, so I don't think we need any printing logic for them (TypeJoinExpr isn't even really representable as source code anyway).

ClosureExpr should print a closure e.g { <body> } if there are no params, { <param>, ... in <body> } if there are params, { (<param>, ...) -> <Result> in <body>} if there is an explicit result type, { (<param>: <Type>, ...) in } if there is an explicit param type. Note you'll also need to handle CaptureListExpr, which also prints a closure, but with a capture list e.g { [<capture>, ...] <param>, ... in }.

lib/AST/ASTPrinter.cpp

Rajveer100 · 2023-04-04T10:49:36Z

How would visitTypeJoinExpr, visitClosureExpr, ... be handled?

Sorry, I shouldn't have included TypeJoinExpr and OneWayExpr in the list, they only exist during constraint solving, so I don't think we need any printing logic for them (TypeJoinExpr isn't even really representable as source code anyway).

ClosureExpr should print a closure e.g { <body> } if there are no params, { <param>, ... in <body> } if there are params, { (<param>, ...) -> <Result> in <body>} if there is an explicit result type, { (<param>: <Type>, ...) in } if there is an explicit param type. Note you'll also need to handle CaptureListExpr, which also prints a closure, but with a capture list e.g { [<capture>, ...] <param>, ... in }.

Alright! If I correctly understand, the capture in closures is similar to lambda capture in C++, just that in Swift it's handled by weak self?

Also after the recent update in Swift, I presume we no longer have to use self keyword inside the body (to reference a capture variable) once it's guarded.

hamishknight · 2023-04-05T14:10:41Z

Alright! If I correctly understand, the capture in closures is similar to lambda capture in C++, just that in Swift it's handled by weak self?

Yes they are similar to lambda captures (specifically captures by value). weak and unowned can be used as ownership modifiers in a capture list, yes, but they can be applied to any capture, e.g:

class C {}
var c = C()
let fn = { [weak c] in }

And without them, the capture is by a strong reference:

class C {}
var c = C()
let fn = { [c] in }

hamishknight · 2024-01-24T21:10:11Z

Note you will also need to fixup test/expr/print/protocol.swift to drop the access modifiers from the local vars:

  // CHECK: internal func cast(_ a: Any) {
- // CHECK:   @_hasInitialValue private let conditional: (any Archivable)? = a as? any Archivable
- // CHECK:   @_hasInitialValue private let forced: any Archivable = a as! any Archivable
+ // CHECK:   @_hasInitialValue let conditional: (any Archivable)? = a as? any Archivable
+ // CHECK:   @_hasInitialValue let forced: any Archivable = a as! any Archivable
  // CHECK: }

Rajveer100 · 2024-01-25T17:08:22Z

KeyPathExpr is complete right, or are there some significant changes to do still?

hamishknight · 2024-01-25T17:10:20Z

KeyPathExpr is complete right, or are there some significant changes to do still?

See #64667 (comment), the components still need printing

Rajveer100 · 2024-01-25T17:16:39Z

KeyPathExpr is complete right, or are there some significant changes to do still?

See #64667 (comment), the components still need printing

Under printKeyPathComponents, which components are missing?

hamishknight · 2024-01-25T17:19:48Z

Under printKeyPathComponents, which components are missing?

The individual components are good, but you're not calling into printKeyPathComponents for regular key paths, and you're missing the dots between components.

Rajveer100 · 2024-01-25T19:56:16Z

The individual components are good, but you're not calling into printKeyPathComponents for regular key paths, and you're missing the dots between components.

IIUC, we need to call this irrespective of any of the conditions (like ObjC or if root exists or not)?

hamishknight · 2024-01-25T20:36:08Z

IIUC, we need to call this irrespective of any of the conditions (like ObjC or if root exists or not)?

Yes, that's correct. FWIW it looks like the getParsedRoot() path is also missing the leading \. I would do the expr->isObjC() check as an early return, then print the \, then have the if statement where you print the root, then print the components. Happy for this to be done in a follow-up though if you leave it as a FIXME.

hamishknight · 2024-01-29T14:12:16Z

@swift-ci please test

Rajveer100 · 2024-01-29T18:48:02Z

Tests pass locally, I have pushed again, maybe a rebase should solve this, as there are few other tests not part of my changes that also fail?

hamishknight

Looks like you need to make a couple of other changes (the change to not print access modifiers for local declarations has affected a couple of other tests):

In test/attr/accessibility_print.swift:

@@ -211,14 +211,14 @@ public extension FE_PublicClass {
 
 // CHECK-LABEL: internal func GA_localTypes()
 func GA_localTypes() {
-  // CHECK-SRC: private struct Local {
+  // CHECK-SRC: struct Local {
   struct Local {
     // CHECK-SRC: internal let x
     let x = 0
   }
   _ = Local()
 
-  // CHECK-SRC: private enum LocalEnum {
+  // CHECK-SRC: enum LocalEnum {
   enum LocalEnum {
     // CHECK-SRC: {{^}} case A
     case A, B

In decl/enum/derived_hashable_equatable.swift:

@@ -8,14 +8,14 @@ enum Simple: Hashable {
   case b
 
   // CHECK:        @_implements(Equatable, ==(_:_:)) internal static func __derived_enum_equals(_ a: Simple, _ b: Simple) -> Bool {
-  // CHECK-NEXT:     private var index_a: Int
+  // CHECK-NEXT:     var index_a: Int
   // CHECK-NEXT:     switch a {
   // CHECK-NEXT:     case .a:
   // CHECK-NEXT:       index_a = 0
   // CHECK-NEXT:     case .b:
   // CHECK-NEXT:       index_a = 1
   // CHECK-NEXT:     }
-  // CHECK-NEXT:     private var index_b: Int
+  // CHECK-NEXT:     var index_b: Int
   // CHECK-NEXT:     switch b {
   // CHECK-NEXT:     case .a:
   // CHECK-NEXT:       index_b = 0
@@ -26,7 +26,7 @@ enum Simple: Hashable {
   // CHECK-NEXT:   }
 
   // CHECK:        internal func hash(into hasher: inout Hasher) {
-  // CHECK-NEXT:     private var discriminator: Int
+  // CHECK-NEXT:     var discriminator: Int
   // CHECK-NEXT:     switch self {
   // CHECK-NEXT:     case .a:
   // CHECK-NEXT:       discriminator = 0
@@ -101,14 +101,14 @@ enum HasUnavailableElement: Hashable {
   case b
 
   // CHECK:       @_implements(Equatable, ==(_:_:)) internal static func __derived_enum_equals(_ a: HasUnavailableElement, _ b: HasUnavailableElement) -> Bool {
-  // CHECK-NEXT:    private var index_a: Int
+  // CHECK-NEXT:    var index_a: Int
   // CHECK-NEXT:    switch a {
   // CHECK-NEXT:    case .a:
   // CHECK-NEXT:      index_a = 0
   // CHECK-NEXT:    case .b:
   // CHECK-NEXT:      _diagnoseUnavailableCodeReached{{.*}}()
   // CHECK-NEXT:    }
-  // CHECK-NEXT:    private var index_b: Int
+  // CHECK-NEXT:    var index_b: Int
   // CHECK-NEXT:    switch b {
   // CHECK-NEXT:    case .a:
   // CHECK-NEXT:      index_b = 0
@@ -119,7 +119,7 @@ enum HasUnavailableElement: Hashable {
   // CHECK-NEXT:  }
 
   // CHECK:       internal func hash(into hasher: inout Hasher) {
-  // CHECK-NEXT:    private var discriminator: Int
+  // CHECK-NEXT:    var discriminator: Int
   // CHECK-NEXT:    switch self {
   // CHECK-NEXT:    case .a:
   // CHECK-NEXT:      discriminator = 0
@@ -185,14 +185,14 @@ enum UnavailableEnum: Hashable {
   case b
 
   // CHECK:        @_implements(Equatable, ==(_:_:)) internal static func __derived_enum_equals(_ a: UnavailableEnum, _ b: UnavailableEnum) -> Bool {
-  // CHECK-NEXT:     private var index_a: Int
+  // CHECK-NEXT:     var index_a: Int
   // CHECK-NEXT:     switch a {
   // CHECK-NEXT:     case .a:
   // CHECK-NEXT:       index_a = 0
   // CHECK-NEXT:     case .b:
   // CHECK-NEXT:       index_a = 1
   // CHECK-NEXT:     }
-  // CHECK-NEXT:     private var index_b: Int
+  // CHECK-NEXT:     var index_b: Int
   // CHECK-NEXT:     switch b {
   // CHECK-NEXT:     case .a:
   // CHECK-NEXT:       index_b = 0
@@ -203,7 +203,7 @@ enum UnavailableEnum: Hashable {
   // CHECK-NEXT:   }
 
   // CHECK:        internal func hash(into hasher: inout Hasher) {
-  // CHECK-NEXT:     private var discriminator: Int
+  // CHECK-NEXT:     var discriminator: Int
   // CHECK-NEXT:     switch self {
   // CHECK-NEXT:     case .a:
   // CHECK-NEXT:       discriminator = 0

test/expr/print/misc_expr.swift

hamishknight · 2024-01-30T20:06:56Z

@swift-ci please test

Rajveer100 · 2024-01-31T19:35:05Z

Regressions macOS Platform:

Failed Tests (2):
  Swift(macosx-x86_64) :: decl/enum/derived_hashable_equatable_macos.swift
  Swift(macosx-x86_64) :: expr/print/func_closures.swift

CI

hamishknight · 2024-02-01T19:57:26Z

@swift-ci please test macOS

hamishknight · 2024-02-01T23:10:51Z

Looks like a couple more test tweaks are needed:

test/decl/enum/derived_hashable_equatable_macos.swift:

@@ -25,7 +25,7 @@ enum HasElementsWithAvailability: Hashable {
   case introduced10_50
 
   // CHECK:       @_implements(Equatable, ==(_:_:)) internal static func __derived_enum_equals(_ a: HasElementsWithAvailability, _ b: HasElementsWithAvailability) -> Bool {
-  // CHECK-NEXT:    private var index_a: Int
+  // CHECK-NEXT:    var index_a: Int
   // CHECK-NEXT:    switch a {
   // CHECK-NEXT:    case .alwaysAvailable:
   // CHECK-NEXT:      index_a = 0
@@ -40,7 +40,7 @@ enum HasElementsWithAvailability: Hashable {
   // CHECK-NEXT:    case .introduced10_50:
   // CHECK-NEXT:      index_a = 2
   // CHECK-NEXT:    }
-  // CHECK-NEXT:    private var index_b: Int
+  // CHECK-NEXT:    var index_b: Int
   // CHECK-NEXT:    switch b {
   // CHECK-NEXT:    case .alwaysAvailable:
   // CHECK-NEXT:      index_b = 0
@@ -59,7 +59,7 @@ enum HasElementsWithAvailability: Hashable {
   // CHECK-NEXT:  }
 
   // CHECK:       internal func hash(into hasher: inout Hasher) {
-  // CHECK-NEXT:    private var discriminator: Int
+  // CHECK-NEXT:    var discriminator: Int
   // CHECK-NEXT:    switch self {
   // CHECK-NEXT:    case .alwaysAvailable:
   // CHECK-NEXT:      discriminator = 0

test/expr/print/func_closures.swift:

@@ -1,4 +1,4 @@
-// RUN: %target-swift-frontend -print-ast %s 2>&1 | %FileCheck %s
+// RUN: %target-swift-frontend -print-ast -disable-availability-checking %s 2>&1 | %FileCheck %s
 
 func fetch() async throws -> String {
 }

Part of Issue swiftlang#61601

hamishknight · 2024-02-02T10:45:51Z

@swift-ci please test

Rajveer100 · 2024-02-03T05:52:03Z

CI looks good!

hamishknight

Thank you!

Rajveer100 · 2024-02-05T11:42:42Z

Welcome!

Rajveer100 requested review from hborla, slavapestov and xedin as code owners March 28, 2023 10:49

hamishknight self-requested a review March 28, 2023 10:56

hamishknight requested changes Mar 28, 2023

View reviewed changes

lib/AST/ASTPrinter.cpp Outdated Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-61601 branch from 7673bfa to 8231914 Compare March 28, 2023 19:00

Rajveer100 requested review from hamishknight and removed request for xedin, slavapestov and hborla March 28, 2023 19:04

hamishknight reviewed Mar 29, 2023

View reviewed changes

lib/AST/ASTPrinter.cpp Outdated Show resolved Hide resolved

hamishknight reviewed Mar 29, 2023

View reviewed changes

lib/AST/ASTPrinter.cpp Outdated Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-61601 branch from 8231914 to 58e0722 Compare March 29, 2023 18:34

hamishknight reviewed Mar 29, 2023

View reviewed changes

lib/AST/ASTPrinter.cpp Outdated Show resolved Hide resolved

hamishknight reviewed Mar 29, 2023

View reviewed changes

lib/AST/ASTPrinter.cpp Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-61601 branch from 58e0722 to ca14635 Compare March 30, 2023 11:36

Rajveer100 force-pushed the branch-for-issue-61601 branch from ca14635 to b15b531 Compare March 31, 2023 11:08

Rajveer100 requested a review from hamishknight March 31, 2023 11:09

hamishknight reviewed Apr 3, 2023

View reviewed changes

lib/AST/ASTPrinter.cpp Outdated Show resolved Hide resolved

hamishknight reviewed Apr 3, 2023

View reviewed changes

lib/AST/ASTPrinter.cpp Outdated Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-61601 branch from b15b531 to 1b5c7cf Compare April 8, 2023 10:01

Rajveer100 requested a review from tshortli as a code owner April 8, 2023 10:01

Rajveer100 force-pushed the branch-for-issue-61601 branch 2 times, most recently from 6bb3422 to 8da1c75 Compare April 13, 2023 11:30

Rajveer100 requested a review from hamishknight April 13, 2023 11:30

Rajveer100 force-pushed the branch-for-issue-61601 branch 2 times, most recently from d494003 to 135333f Compare January 29, 2024 09:14

Rajveer100 requested a review from hamishknight January 29, 2024 09:15

Rajveer100 force-pushed the branch-for-issue-61601 branch from 135333f to dac72b2 Compare January 29, 2024 18:46

hamishknight reviewed Jan 29, 2024

View reviewed changes

test/expr/print/misc_expr.swift Outdated Show resolved Hide resolved

Rajveer100 force-pushed the branch-for-issue-61601 branch 3 times, most recently from dac72b2 to ffc7673 Compare January 30, 2024 18:54

Rajveer100 requested a review from hamishknight January 30, 2024 18:55

Added the expression cases of the form 'visit...Expr()'

48847da

Part of Issue swiftlang#61601

Rajveer100 force-pushed the branch-for-issue-61601 branch from ffc7673 to 48847da Compare February 2, 2024 08:21

hamishknight approved these changes Feb 5, 2024

View reviewed changes

hamishknight merged commit 658be5b into swiftlang:main Feb 5, 2024
5 checks passed

hamishknight mentioned this pull request Feb 5, 2024

Fill out unimplemented expression cases in the ASTPrinter #61601

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added unimplemented expression cases in the ASTPrinter #64667

Added unimplemented expression cases in the ASTPrinter #64667

Rajveer100 commented Mar 28, 2023 •

edited by hamishknight

Loading

Rajveer100 commented Mar 28, 2023

hamishknight left a comment

Rajveer100 commented Mar 31, 2023 •

edited

Loading

hamishknight commented Apr 3, 2023

Rajveer100 commented Apr 4, 2023

hamishknight commented Apr 5, 2023

hamishknight commented Jan 24, 2024

Rajveer100 commented Jan 25, 2024 •

edited

Loading

hamishknight commented Jan 25, 2024

Rajveer100 commented Jan 25, 2024

hamishknight commented Jan 25, 2024

Rajveer100 commented Jan 25, 2024

hamishknight commented Jan 25, 2024

hamishknight commented Jan 29, 2024

Rajveer100 commented Jan 29, 2024 •

edited

Loading

hamishknight left a comment

hamishknight commented Jan 30, 2024

Rajveer100 commented Jan 31, 2024 •

edited

Loading

hamishknight commented Feb 1, 2024

hamishknight commented Feb 1, 2024

hamishknight commented Feb 2, 2024

Rajveer100 commented Feb 3, 2024

hamishknight left a comment

Rajveer100 commented Feb 5, 2024

Added unimplemented expression cases in the ASTPrinter #64667

Added unimplemented expression cases in the ASTPrinter #64667

Conversation

Rajveer100 commented Mar 28, 2023 • edited by hamishknight Loading

Rajveer100 commented Mar 28, 2023

hamishknight left a comment

Choose a reason for hiding this comment

Rajveer100 commented Mar 31, 2023 • edited Loading

hamishknight commented Apr 3, 2023

Rajveer100 commented Apr 4, 2023

hamishknight commented Apr 5, 2023

hamishknight commented Jan 24, 2024

Rajveer100 commented Jan 25, 2024 • edited Loading

hamishknight commented Jan 25, 2024

Rajveer100 commented Jan 25, 2024

hamishknight commented Jan 25, 2024

Rajveer100 commented Jan 25, 2024

hamishknight commented Jan 25, 2024

hamishknight commented Jan 29, 2024

Rajveer100 commented Jan 29, 2024 • edited Loading

hamishknight left a comment

Choose a reason for hiding this comment

hamishknight commented Jan 30, 2024

Rajveer100 commented Jan 31, 2024 • edited Loading

hamishknight commented Feb 1, 2024

hamishknight commented Feb 1, 2024

hamishknight commented Feb 2, 2024

Rajveer100 commented Feb 3, 2024

hamishknight left a comment

Choose a reason for hiding this comment

Rajveer100 commented Feb 5, 2024

Rajveer100 commented Mar 28, 2023 •

edited by hamishknight

Loading

Rajveer100 commented Mar 31, 2023 •

edited

Loading

Rajveer100 commented Jan 25, 2024 •

edited

Loading

Rajveer100 commented Jan 29, 2024 •

edited

Loading

Rajveer100 commented Jan 31, 2024 •

edited

Loading