[Sema] Implement SE-0110 #4102

MnO2 · 2016-08-08T14:06:47Z

What's in this pull request?

[Sema] Implement SE-0010

This commit built upon the work of Pull Request 3895 by @dduan. Apart from the work to make the following work

let f: (Int, Int) -> Void = { x in  } // this is now an error

This patch also implement the part 2 mentioned in the #3895

let g: ((Int, Int)) -> Void = { y in  } // y should have type (Int, Int)

Resolved bug number: (SR-2008)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

Builds the compiler incrementally.
Builds the standard library only for macOS. Simulator standard libraries and
device standard libraries are not built.
lldb is not built.
The test and validation-test targets are run only for macOS. The optimized
version of these tests are not run.

A smoke test on Linux does the following:

Builds the compiler incrementally.
Builds the standard library incrementally.
lldb is built incrementally.
The swift test and validation-test targets are run. The optimized version of these
tests are not run.
lldb is tested.

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

This commit built upon the work of Pull Request 3895. Apart from the
work to make the following work

let f: (Int, Int) -> Void = { x in  } // this is now an error

This patch also implement the part 2 mentioned in the #3895

let g: ((Int, Int)) -> Void = { y in  } // y should have type (Int, Int)

dduan · 2016-08-08T15:41:27Z

@swift-ci test

CodaFi · 2016-08-08T16:08:17Z

include/swift/AST/DiagnosticsSema.def

@@ -2482,6 +2482,9 @@ ERROR(tuple_pattern_in_non_tuple_context,none,
 ERROR(closure_argument_list_tuple,none,
      "contextual closure type %0 expects %1 argument%s1, "
      "but %2 were used in closure body", (Type, unsigned, unsigned))
+ERROR(closure_argument_list_single_tuple,none,


This looks like it's going to need a singular and plural %select for were/was.

I tried to use %switch but couldn't find it in the code base, therefore I use %select instead.

My bad. You're correct.

CodaFi · 2016-08-08T16:24:59Z

I noticed that this pull request seems to be digging out the underlying pointer from the type structures it's using. You shouldn't have to do that. Instead, try to either use the invariants of the surrounding conditions and their types or use getAs<T>.

MnO2 · 2016-08-09T13:19:03Z

I've updated the code by the good suggestions. Using CanType makes the code much shorter. And I have used getType()->is<T>() with castTo<T>() as much as possible. However, due to the requirement of keeping the type sugared, I still write getPointer() in one of the places until I found a good alternative. Also I documented my thinking according to the feedback. Thanks for reviewing!

CodaFi · 2016-08-09T14:05:29Z

test/Constraints/closures.swift

@@ -202,7 +202,7 @@ func acceptNothingToInt (_: () -> Int) {}
 func testAcceptNothingToInt(ac1: @autoclosure () -> Int) {
  // expected-note@-1{{parameter 'ac1' is implicitly non-escaping because it was declared @autoclosure}}
  acceptNothingToInt({ac1($0)})
-  // expected-error@-1{{cannot convert value of type '(_) -> Int' to expected argument type '() -> Int'}}
+  // expected-error@-1{{contextual closure type '() -> Int' expects 0 arguments, but 1 were used in closure body}}


Did this diagnostic not change?

For the case where contextual type having empty parameter list (ex: () -> Int), I added a constraint where the number of elements has to be greater than or equal to 2. (at TypeCheckConstraints.cpp:1537). The rational behind is because in the case of () -> Int, the parameter would be derived as a TupleType with 0 elements but not ParenType, and for the case of (Int) -> Int it would be a sugared ParenType. And for the case empty tuple it would fall through and catched by TypeCheckPattern.cpp:1573, I cannot handle the case of empty tuple and early return is because it is still likely the closure has zero input, and that would be best delegated to coerceParameterListToType

I mean the were/was distinction. This should be in the singular.

This diagnostic is from closure_argument_list_tuple where I didn't touch (only use it). What I added was closure_argument_list_single_tuple. I think it is a minor bug sticking with closure_argument_list_tuple where it didn't use %select for {was|were}. Do you think the best way would be to put it in this commit? or have a separate bug fixing commit instead?

It's up to you whether you'd like to fix it here or not - it is relevant to the changes you're making. If you choose not to let me know so I can file a JIRA starterbug about it.

I've made the change. I decided to change in this pull request since I realized that the case of singular was would only be triggered by the change in this pull request. Before the change, all of the parameters would be treated as one single tuple and assigned to the single parameter of closure.

rintaro · 2016-08-12T01:24:05Z

include/swift/AST/DiagnosticsSema.def

+      "but %2 %select{was|were}3 used in closure body", (Type, unsigned, unsigned, bool))
+ERROR(closure_argument_list_single_tuple,none,
+      "contextual closure type specifies %0, but %1 %select{was|were}2 used in closure body, "
+      "try add extra parenthesis around the single tuple argument", (Type, unsigned, bool))


Could you add some test cases for this error?

For the singular was case, it has been covered by test/expr/closure/default_args.swift:6 and 7. For the plural case, it was covered by test/Constraints/closures.swift:118, 121, 127, 130, 135. Is there anything I missed or a better way to organize the tests?

I don't see expected-error that includes try add extra parenthesis around the single tuple argument.

my bad. I mis-read your comment as the change on line:2488 since they are quoted as one green block. I would add it, thanks!

Grammar nit: "try adding extra parentheses"

I've added a test case on test/Constraints/closures.swift:326 and corrected the grammar. :)

CodaFi · 2016-08-14T01:02:03Z

lib/Sema/TypeCheckPattern.cpp

-             paramListType);
+    auto fnType = FunctionType::get(paramListType->getDesugaredType(), FN->getResult());
+    diagnose(P->getStartLoc(), diag::closure_argument_list_tuple,
+             fnType, tupleTy->getNumElements(), P->size(), (P->size() > 1));


Just to be future-proof: Invert the select and make the P->size() > 1 into P->size() == 1 so we can report 0 in the (bizarre) plural correctly.

This commit built upon the work of Pull Request 3895. Apart from the work to make the following work ```swift let f: (Int, Int) -> Void = { x in } // this is now an error ``` This patch also implement the part 2 mentioned in the swiftlang#3895 ```swift let g: ((Int, Int)) -> Void = { y in } // y should have type (Int, Int) ```

CodaFi · 2016-08-14T16:47:30Z

This is looking fantastic.

@swift-ci please smoke test.

CodaFi · 2016-08-14T19:24:49Z

@swift-ci please smoke test Linux platform.

MnO2 · 2016-08-15T04:47:45Z

hmm, not sure the root of cause of smoke test failure on Linux.

One of the failure Traceback

======================================================================
FAIL: test_equality_operators_fileprivate_dwarf (TestEqualityOperators.TestUnitTests)
   Test that we resolve expression operators correctly
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1483, in dwarf_test_method
    return attrvalue(self)
  File "/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/lldb/packages/Python/lldbsuite/test/decorators.py", line 121, in wrapper
    func(*args, **kwargs)
  File "/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/lldb/packages/Python/lldbsuite/test/lang/swift/expression/equality_operators/TestEqualityOperators.py", line 37, in test_equality_operators_fileprivate
    self.do_test("Fooey.CompareEm1", "true", 1)
  File "/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/lldb/packages/Python/lldbsuite/test/lang/swift/expression/equality_operators/TestEqualityOperators.py", line 79, in do_test
    self.assertTrue(len(threads) == 1)
AssertionError: False is not True
Config=x86_64-/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test/branch-master/buildbot_linux/llvm-linux-x86_64/bin/clang-3.9

CodaFi · 2016-08-15T05:44:39Z

That's not your fault and you don't have to worry about it. Your patch passed our tests.

CodaFi · 2016-08-15T20:26:51Z

Now that LLDB has been dealt with, let's land this sucker.

@swift-ci please smoke test and merge.

CodaFi · 2016-08-15T21:10:51Z

Thank you @MnO2! Please don't forget to update the CHANGELOG and resolve SR-2008

DougGregor · 2016-08-19T05:53:53Z

Hrm. So, we're past the point where we can make source changes to Swift 3. We'll need to gate this feature on an as-yet-undesigned version flag.

MnO2 · 2016-08-19T06:31:24Z

I can help on that, but it would be great to have a reference commit on how to gate a feature so that I can figure out how to do it easily.

CodaFi reviewed Aug 8, 2016
View reviewed changes

dduan changed the title ~~[Sema] Implement SE-0010~~ [Sema] Implement SE-0110 Aug 8, 2016

MnO2 force-pushed the master branch from 23117a8 to 51ee442 Compare August 9, 2016 12:51

CodaFi reviewed Aug 9, 2016
View reviewed changes

MnO2 force-pushed the master branch from 51ee442 to 30af410 Compare August 11, 2016 06:50

rintaro reviewed Aug 12, 2016
View reviewed changes

MnO2 force-pushed the master branch from 30af410 to c9ecb44 Compare August 13, 2016 01:06

CodaFi reviewed Aug 14, 2016
View reviewed changes

MnO2 force-pushed the master branch from c9ecb44 to e172383 Compare August 14, 2016 05:12

swift-ci merged commit 83f2eb0 into swiftlang:master Aug 15, 2016

benrimmington mentioned this pull request Sep 24, 2016

SE-0140 and SE-0110 have been implemented swiftlang/swift-evolution#532

Merged

benrimmington mentioned this pull request Nov 13, 2016

[Status] Add section for “Implementation in progress” swiftlang/swift-evolution#558

Merged

jrose-apple mentioned this pull request Dec 14, 2016

Revert "[Sema] Implement SE-0110" #6260

Closed

swift-ci mentioned this pull request Aug 8, 2016

[SR-2008] Implement SE-0110: Distinguish between single-tuple and multiple-argument function types #44617

Closed

[Sema] Implement SE-0110 #4102

[Sema] Implement SE-0110 #4102

Uh oh!

Conversation

MnO2 commented Aug 8, 2016

What's in this pull request?

Resolved bug number: (SR-2008)

Uh oh!

dduan commented Aug 8, 2016

Uh oh!

CodaFi Aug 8, 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!

CodaFi commented Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MnO2 commented Aug 9, 2016

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!

CodaFi Aug 14, 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!

CodaFi commented Aug 14, 2016

Uh oh!

CodaFi commented Aug 14, 2016

Uh oh!

MnO2 commented Aug 15, 2016

Uh oh!

CodaFi commented Aug 15, 2016

Uh oh!

CodaFi commented Aug 15, 2016

Uh oh!

CodaFi commented Aug 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DougGregor commented Aug 19, 2016

Uh oh!

MnO2 commented Aug 19, 2016

Uh oh!

Uh oh!

CodaFi Aug 8, 2016 •

edited

Loading

CodaFi commented Aug 8, 2016 •

edited

Loading

CodaFi Aug 14, 2016 •

edited

Loading

CodaFi commented Aug 15, 2016 •

edited

Loading