Fix @escaping printing #4905

milseman · 2016-09-21T20:49:06Z

[Cleanup] Drop needless TupleTypeElt constructor calls

Now that TupleTypeElts are simpler in Swift 3 (though they're about to
become more complicated for other reasons), most of the cases where we
are explicitly constructing ones are really just plain copies or can
otherwise use existing helper functions.

[AST] Create ParameterTypeFlags and put them on function params

Long term, we want to refactor the AST to reflect the current
programming model in Swift. This would include refactoring
FunctionType to take a list of ParameterTypeElt, or something with a
better name, that can contain both the type and flags/bits that are
only specific to types in parameter position, such as @autoclosure and
@escaping. At the same time, noescape-by-default has severely hurt our
ability to print types without significant context, as we either have
to choose to too aggressively print @escaping or not print it in every
situation it occurs, or both.

As a gentle step towards the final solution, without uprooting our
overall AST structure, and as a way towards fixing the @escaping
printing ails, put these bits on the TupleTypeElt and ParenType, which
will serve as a model for what ParameterTypeElt will be like in the
future. Re-use these flags on CallArgParam, to leverage shared
knowledge in the type system. It is a little painful to tack onto
these types, but it's minor and will be overhauled soon, which will
eventually result in size savings and less complexity overall.

This includes all the constraint system adjustments to make these
types work and influence type equality and overload resolution as
desired. They are encoded in the module format. Additional tests
added.

[ASTPrinter] Switch to new ParameterTypeFlags

Switch printing off of using Function's ExtInfo for autoclosure and
escaping, and onto the ParameterTypeFlags, which let us do precise and
accurate context-sensitive printing of these parameter type
attributes. This fixes a huge list of issues where we were printing
@escaping for things like optional ObjC completion handlers, among
many others. We now correctly print @escaping in more places, and
don't print it when it's not correct.

Also updates the dumper to be consistent and give a good view of the
AST as represented in memory. Tests updated, more involved testing
coming soon.

Resolves SR-2324, and potentially many other duplicates. I will be re-evaluating each of the SRs to see which have probably been fixed, as well as expanding corner case test coverage in lit.

I want to get this process started so that improvements can start landing.

milseman · 2016-09-21T21:20:59Z

@swift-ci please test

jrose-apple

Nice work. It still feels kind of big for a 3.0.1, but on the other hand it's definitely the direction we want to move in, and it makes some things a lot better.

jrose-apple · 2016-09-21T21:22:47Z

include/swift/AST/Types.h

@@ -1353,6 +1353,11 @@ class TupleTypeElt {
  TupleTypeElt getWithType(Type T) const {
    return TupleTypeElt(T, getName(), isVararg());
  }
+
+  /// Retrieve a copy of this tuple type element with the name replaced.
+  TupleTypeElt getWithName(Identifier name = Identifier()) const {


Nitpick: It seems weird to be able to call getWithName() and not have to explicitly specify "no name".

jrose-apple · 2016-09-21T21:24:57Z

include/swift/AST/Types.h

+
+  ParameterTypeFlags() : value(None) {}
+
+  ParameterTypeFlags(uint8_t val) : value(val) {}


This probably shouldn't be public.

jrose-apple · 2016-09-21T21:25:21Z

include/swift/AST/Types.h

+  bool isAutoClosure() const { return 0 != (value & AutoClosure); }
+  bool isEscaping() const { return 0 != (value & Escaping); }
+
+  bool operator==(const ParameterTypeFlags &other) {


Nitpick: const.

jrose-apple · 2016-09-21T21:26:50Z

include/swift/AST/Types.h

@@ -1351,12 +1411,20 @@ class TupleTypeElt {

  /// Retrieve a copy of this tuple type element with the type replaced.
  TupleTypeElt getWithType(Type T) const {
-    return TupleTypeElt(T, getName(), isVararg());
+    return TupleTypeElt(T, getName(), isVararg(), isAutoClosure(),


Shouldn't these use the constructor that takes a ParameterTypeFlags?

jrose-apple · 2016-09-21T21:28:10Z

include/swift/AST/Types.h

+
+  /// Retrieve a copy of this tuple type element with the type and name
+  /// replaced.
+  TupleTypeElt getWithTypeAndName(Type T, Identifier name) const {


I feel like it's fine to use constructor syntax for this one rather than a getWith… method. TupleTypeElt(T, name, orig.getParameterFlags().

jrose-apple · 2016-09-21T21:35:20Z

lib/AST/ASTPrinter.cpp

+    printParameterFlags(Printer, Options, paramFlags);
+
+    // Special case, if we're not going to use the type repr printing, peek
+    // through the paren types so that we don't print excessive @escapings


Nitpick: period.

What? Please clarify?

Ah, sorry. A lot of your comments are missing final periods. Very tiny nitpick!

Ah, I didn't realize there was a convention there
(period in this comment intentionally absent just to drive you crazy :-)

jrose-apple · 2016-09-21T21:36:25Z

lib/AST/ASTPrinter.cpp


  for (unsigned CurrPattern = 0, NumPatterns = BodyParams.size();
       CurrPattern != NumPatterns; ++CurrPattern) {
-    printParameterList(BodyParams[CurrPattern], /*Curried=*/CurrPattern > 0,
+    // Be extra careful in the event of printing mal-formed ASTs
+    auto paramListType = parameterListTypes.size() > CurrPattern


Off-by-one error here.

jrose-apple · 2016-09-21T21:37:24Z

test/Constraints/diagnostics.swift

@@ -672,7 +672,7 @@ func overloadSetResultType(_ a : Int, b : Int) -> Int {
  // https://twitter.com/_jlfischer/status/712337382175952896
  // TODO: <rdar://problem/27391581> QoI: Nonsensical "binary operator '&&' cannot be applied to two 'Bool' operands"
  return a == b && 1 == 2  // expected-error {{binary operator '&&' cannot be applied to two 'Bool' operands}}
-  // expected-note @-1 {{expected an argument list of type '(Bool, @autoclosure () throws -> Bool)'}}
+  // expected-note @-1 {{expected an argument list of type '(Bool, () throws -> Bool)'}}


This seems like a problem; the autoclosure is important to the call site.

Oh, right, @autoclosure is irrelevant inside of the definition but totally relevant to the caller

Also, code might be a little different for operators. I'll look into this, and also expand test coverage for operators with e.g. escaping

Argh... This is CSDiag having it's own custom printer. I'll try to make it use the ASTPrinter...

jrose-apple · 2016-09-21T21:38:35Z

test/IDE/print_clang_bool_bridging.swift

-func testDarwinBooleanFnToBlockTypedef(_: DarwinBooleanFn) -> DarwinBooleanBlock
+func testCBoolFnToBlockTypedef(_: @escaping CBoolFn) -> CBoolBlock
+func testObjCBoolFnToBlockTypedef(_: @escaping ObjCBoolFn) -> ObjCBoolBlock
+func testDarwinBooleanFnToBlockTypedef(_: @escaping DarwinBooleanFn) -> DarwinBooleanBlock


jrose-apple · 2016-09-21T21:38:47Z

test/IDE/print_clang_bool_bridging.swift

-  func produceDarwinBooleanBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@escaping @convention(block) (DarwinBoolean) -> DarwinBoolean)?>)
+  func produceCBoolBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@convention(block) (Bool) -> Bool)?>)
+  func produceObjCBoolBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@convention(block) (ObjCBool) -> ObjCBool)?>)
+  func produceDarwinBooleanBlockTypedef(_ outBlock: AutoreleasingUnsafeMutablePointer<(@convention(block) (DarwinBoolean) -> DarwinBoolean)?>)


Also sweet!

swift-ci · 2016-09-21T21:51:09Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 7c4ac884227ddd568a3e993a58d9b66be531bfed
Test requested by - @milseman

milseman · 2016-09-22T05:10:59Z

(build failure was Foundation casting with Any)

milseman · 2016-09-22T17:10:29Z

@swift-ci please test

swift-ci · 2016-09-22T17:32:21Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 111b2074bc838b34ce53f5df4426006b5530ebb8
Test requested by - @milseman

jrose-apple · 2016-09-22T17:50:35Z

lib/AST/ASTPrinter.cpp

@@ -2742,7 +2742,7 @@ void PrintAST::printFunctionParameters(AbstractFunctionDecl *AFD) {
  for (unsigned CurrPattern = 0, NumPatterns = BodyParams.size();
       CurrPattern != NumPatterns; ++CurrPattern) {
    // Be extra careful in the event of printing mal-formed ASTs
-    auto paramListType = parameterListTypes.size() > CurrPattern
+    auto paramListType = CurrPattern < parameterListTypes.size()


I can't off-by-one. Thanks for putting this in the conventional order so that I don't get confused.

Now that TupleTypeElts are simpler in Swift 3 (though they're about to become more complicated for other reasons), most of the cases where we are explicitly constructing ones are really just plain copies or can otherwise use existing helper functions. NFC

@autoclosure

Long term, we want to refactor the AST to reflect the current programming model in Swift. This would include refactoring FunctionType to take a list of ParameterTypeElt, or something with a better name, that can contain both the type and flags/bits that are only specific to types in parameter position, such as @autoclosure and @escaping. At the same time, noescape-by-default has severely hurt our ability to print types without significant context, as we either have to choose to too aggressively print @escaping or not print it in every situation it occurs, or both. As a gentle step towards the final solution, without uprooting our overall AST structure, and as a way towards fixing the @escaping printing ails, put these bits on the TupleTypeElt and ParenType, which will serve as a model for what ParameterTypeElt will be like in the future. Re-use these flags on CallArgParam, to leverage shared knowledge in the type system. It is a little painful to tack onto these types, but it's minor and will be overhauled soon, which will eventually result in size savings and less complexity overall. This includes all the constraint system adjustments to make these types work and influence type equality and overload resolution as desired. They are encoded in the module format. Additional tests added.

Switch printing off of using Function's ExtInfo for autoclosure and escaping, and onto the ParameterTypeFlags, which let us do precise and accurate context-sensitive printing of these parameter type attributes. This fixes a huge list of issues where we were printing @escaping for things like optional ObjC completion handlers, among many others. We now correctly print @escaping in more places, and don't print it when it's not correct. Also updates the dumper to be consistent and give a good view of the AST as represented in memory. Tests updated, more involved testing coming soon.

milseman · 2016-09-22T19:25:36Z

I rebased, and did a minor version bump because I think the old modules were getting mixed in.

@swift-ci please test

swift-ci · 2016-09-22T19:34:19Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 111b2074bc838b34ce53f5df4426006b5530ebb8
Test requested by - @milseman

milseman · 2016-09-22T19:38:44Z

12:34:39 ERROR: Error cloning remote repo 'origin'
12:34:39 hudson.plugins.git.GitException: Could not init /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift

swift-ci · 2016-09-22T19:59:50Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 111b2074bc838b34ce53f5df4426006b5530ebb8
Test requested by - @milseman

milseman · 2016-09-22T20:05:23Z

@jrose-apple I believe I have addressed all of your feedback for this PR, can you confirm?

milseman · 2016-09-22T20:39:02Z

Passes for me locally, waiting on CI to come back up. Note the this PR doesn't fix one corner case where you have nested closures in closures, and at the same time are switching off between the Decl ASTPrinter and the TypeRepr printer. Unlikely to get the fix in for that for 3.0.1, but probably for later. Regardless, that's a niche case, and better to get a fix for the super common scenarios in quickly.

milseman · 2016-09-22T20:56:17Z

@swift-ci please test

swift-ci · 2016-09-22T21:20:36Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 0bb868a
Test requested by - @milseman

shahmishal · 2016-09-22T21:20:57Z

@swift-ci Please test OS X

swift-ci · 2016-09-22T21:28:00Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 4324d8f
Test requested by - @shahmishal

milseman · 2016-09-22T21:32:04Z

@shahmishal 14:28:52 Setting status of 4324d8f to FAILURE with url https://ci.swift.org/job/swift-PR-osx/3756/ and message: 'Build finished. No test results found.'

Any idea what failed?

shahmishal · 2016-09-22T21:37:35Z

This is the error it failed at: Do we need to do clean build?

<unknown>:0: error: module file was created by an older version of the compiler; rebuild 'CoreLocation' and try again: /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/./lib/swift/watchos/armv7k/CoreLocation.swiftmodule

jrose-apple · 2016-09-22T21:46:39Z

That usually means a missing dependency. @jckarter, did you get a chance to look through your new overlays and anything that might depend on them?

milseman · 2016-09-22T22:20:03Z

@shahmishal yeah, can you please do a clean build?

shahmishal · 2016-09-22T22:27:57Z

@swift-ci Please clean test OS X

DougGregor

This looks great.

DougGregor · 2016-09-22T22:48:34Z

include/swift/AST/Types.h

+/// escaping.
+class ParameterTypeFlags {
+  enum ParameterFlags : uint8_t {
+    None = 0,


None seems unnecessary, since we're working with option sets.

milseman · 2016-09-22T23:26:56Z

@swift-ci please smoke test Linux Platform

shahmishal · 2016-09-22T23:33:28Z

@swift-ci please smoke test Linux Platform

[3.0] Fix @escaping printing (from #4905)

milseman force-pushed the escaping_printing_fix_clean branch from a2afa24 to 7c4ac88 Compare September 21, 2016 20:59

jrose-apple suggested changes Sep 21, 2016

View reviewed changes

jrose-apple reviewed Sep 22, 2016

View reviewed changes

milseman added 3 commits September 22, 2016 12:24

milseman force-pushed the escaping_printing_fix_clean branch from 111b207 to 81acf49 Compare September 22, 2016 19:24

[ParameterTypeFlags] Incorporate review feedback

0bb868a

milseman force-pushed the escaping_printing_fix_clean branch from 81acf49 to 0bb868a Compare September 22, 2016 19:24

[CSDiag] Switch to using the ASPrinter rather than repeating logic

4324d8f

milseman force-pushed the escaping_printing_fix_clean branch from 16c72cb to 4324d8f Compare September 22, 2016 20:19

jrose-apple approved these changes Sep 22, 2016

View reviewed changes

milseman mentioned this pull request Sep 22, 2016

[3.0] Fix @escaping printing (from #4905) #4931

Merged

DougGregor approved these changes Sep 22, 2016

View reviewed changes

milseman merged commit 3619738 into swiftlang:master Sep 23, 2016

tkremenek added a commit that referenced this pull request Sep 23, 2016

Merge pull request #4931 from milseman/3_0_escaping_printing_fix_clean

239c5d3

[3.0] Fix @escaping printing (from #4905)

gonzalolarralde mentioned this pull request Sep 23, 2016

More changes for SE-0103 (@noescape by default). swiftlang/swift-corelibs-libdispatch#157

Closed

milseman deleted the escaping_printing_fix_clean branch September 29, 2016 18:12

bg-stripe mentioned this pull request Feb 10, 2017

Remove escaping note from docs stripe/stripe-ios#554

Merged

johnfairh mentioned this pull request Nov 7, 2017

[ASTPrinter] Print parameter type attributes more often #12793

Merged

swift-ci mentioned this pull request Apr 21, 2017

[SR-4637] Add fix-it for removing '@escaping' from optional closure parameter #47214

Closed


		ParameterTypeFlags() : value(None) {}

		ParameterTypeFlags(uint8_t val) : value(val) {}

Fix @escaping printing #4905

Fix @escaping printing #4905

Uh oh!

Conversation

milseman commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milseman commented Sep 21, 2016

Uh oh!

jrose-apple 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!

jrose-apple Sep 21, 2016 • 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!

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!

milseman Sep 22, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Sep 21, 2016

Uh oh!

milseman commented Sep 22, 2016

Uh oh!

milseman commented Sep 22, 2016

Uh oh!

swift-ci commented Sep 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milseman commented Sep 22, 2016

Uh oh!

swift-ci commented Sep 22, 2016

Uh oh!

milseman commented Sep 22, 2016

Uh oh!

swift-ci commented Sep 22, 2016

Uh oh!

milseman commented Sep 22, 2016

Uh oh!

milseman commented Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milseman commented Sep 22, 2016

Uh oh!

swift-ci commented Sep 22, 2016

Uh oh!

shahmishal commented Sep 22, 2016

Uh oh!

swift-ci commented Sep 22, 2016

Uh oh!

milseman commented Sep 22, 2016

milseman commented Sep 21, 2016 •

edited

Loading

jrose-apple Sep 21, 2016 •

edited

Loading

milseman Sep 22, 2016 •

edited

Loading

milseman commented Sep 22, 2016 •

edited

Loading