[DNM][requires-evolution] Explicit varargs expansion expression #25997

owenv · 2019-07-08T14:39:02Z

This is a WIP prototype of a new expression, #variadic, which can be used to pass array elements as variadic arguments. I should have a post with the corresponding proposal pitch up on the forums shortly.

This isn’t meant to be a finalized implementation, just enough to demo the pitched feature and collect feedback. It’s been working well for me so far and passing tests, but still needs:

Better test coverage
Better compiler fixits in a few areas
Some refactoring of argument coercion
General cleanup and refactoring

hamishknight

Great to see this being implemented!

hamishknight · 2019-07-09T18:41:21Z

lib/Sema/CSDiag.cpp

@@ -2296,6 +2297,34 @@ bool FailureDiagnosis::diagnoseContextualConversionError(
    }
  }

+  // Diagnose passing '[T]' instead of a sequence of 'T' to a variadic argument
+  // of type 'T...'.


It would be nice if we could implement this using the new diagnostics framework (see CSFix & CSDiagnostics), as we're currently trying to move stuff out of CSDiag.cpp.

What you'll want to do is add a new constraint fix to CSFix.h, and then add it as a viable fix within repairFailures when the locator's last element is an ApplyArgToParam and you're dealing with an array type being passed to a variadic param. Unfortunately I don't believe there's currently an easy way of accessing the parameter flags from repairFailures, the simplest solution would probably be to store them on the constraint locator – see hamishknight@7e3a601 for an example of that (feel free to cherry-pick!).

Then you'll want to add a case to simplifyFixConstraint where you try to match the array element types and record the fix if successful.

@hamishknight sounds good, I'll look into doing that. Thanks for the example!

…a VarargExpansionExpr

…xprs

This implementation isn't quite right yet because we're not yet checking the parameter flags to see if the param is actually variadic.

Cherry-picks hamishknight@7e3a601 with some deletions This is needed to support the ExpandArrayIntoVarargs ConstraintFix, which needs to be able to determine whether a parameter is variadic

owenv · 2019-07-11T15:32:13Z

This still needs some work, but if anyone with CI privileges is following along, mind starting a test run w/ the standard & validation suites? I see a lot of failures in the validation suite right now that read no such module 'StdlibCollectionUnittest' and I'm not sure if it's something I broke or an issue with my build setup.

hamishknight

Thanks for working on those changes! I just have a few minor comments, I'll leave the rest for the folks at Apple.

hamishknight · 2019-07-11T17:39:46Z

lib/Sema/CSSimplify.cpp

+
+    auto elementTy = lhs->getArrayElementType();
+    if (elt.getKind() == ConstraintLocator::ApplyArgToParam &&
+        elementTy && elt.getParameterFlags().isVariadic()) {


You could use ConstraintSystem::isArrayType here instead, which saves you from having to introduce isArrayDecl & getArrayElementType.

Also you no longer need to check for elt.getKind() == ConstraintLocator::ApplyArgToParam now, as there's an early exit for all other cases a few lines above.

hamishknight · 2019-07-11T17:42:08Z

include/swift/AST/DiagnosticsParse.def

+ERROR(expr_varargs_expansion_expected_expr,PointsToFirstBadToken,
+     "expected expression within '#variadic(...)'", ())
+ERROR(expr_varargs_expansion_expected_rparen,PointsToFirstBadToken,
+     "expected ')' to complete '#variadic' expression", ())


Nit: Strings should be indented by one more character to align with expr_[...].

hamishknight · 2019-07-11T17:42:29Z

include/swift/AST/DiagnosticsSema.def

+      "#variadic cannot be used alongside additional variadic arguments",
+      ())
+ERROR(pound_variadic_outside_arg_position,none,
+     "#variadic can only be used in an argument position",


hamishknight · 2019-07-11T17:42:39Z

include/swift/AST/DiagnosticsSema.def

+     "remove brackets to pass array elements directly", ())
+NOTE(suggest_pass_elements_using_pound_variadic,none,
+    "wrap the expression in '#variadic(...)' to pass array elements as "
+    "variadic arguments", ())


hamishknight · 2019-07-11T17:48:13Z

lib/Sema/CSSimplify.cpp

+    if (elt.getKind() == ConstraintLocator::ApplyArgToParam &&
+        elementTy && elt.getParameterFlags().isVariadic()) {
+      conversionsOrFixes.push_back(ExpandArrayIntoVarargs::create(
+          *this, lhs, rhs, getConstraintLocator(locator)));


Nit: You can use loc instead of getConstraintLocator(locator).

hamishknight · 2019-07-11T17:53:36Z

test/Constraints/diagnostics.swift

+variadic(arrayWithOtherEltType) // expected-error {{cannot convert value of type '[String]' to expected argument type 'Int'}}
+variadic(1, arrayWithOtherEltType) // expected-error {{cannot convert value of type '[String]' to expected argument type 'Int'}}
+variadic(["hello", "world"]) // expected-error 2 {{cannot convert value of type 'String' to expected element type 'Int'}}
+// expected-error@-1 {{cannot pass an array of type '[Int]' as variadic arguments of type 'Int'}}


Hmm, that's unfortunate. The good news at least is that it's not a specific problem with the fix you've introduced, it's a more general problem when we have multiple contextual fixes in play. I've filed a bug to track that: https://bugs.swift.org/browse/SR-11104.

Thanks for taking the time to file this!

…gs coercion - The diags and tests have not yet been updated to reflect the new syntax - The old syntax hasn't been removed yet - But, some type checking hacks the old syntax used have been removed.

…ansion

…on-variadic argument

…ition

…ansion

hamishknight · 2019-07-14T11:39:50Z

lib/Parse/ParseExpr.cpp

-    parsed = new (Context) CoerceExpr(asLoc, type.get());
+    bool includesVarargExpansion = false;
+    auto typeRepr = type.get();
+    if (Tok.isEllipsis()) {


You'll want to keep the Tok.is(tok::oper_postfix) check here to make sure we reject things like:

func foo(_ x: Int...) {} foo([0] as Int ...)

In addition, it'll allow us to continue accepting the following:

let r = 0 as Int8...5

which calls the infix ... operator.

🤦‍♂ Good catch, thanks. Going to work on adding some more parsing tests

…maintain source compatibility with the infix ... operattor

…or vararg expansion

owenv · 2019-10-10T21:22:37Z

Closing this since it's pretty hopelessly out of date and the diagnostics improvements have landed elsewhere.

hamishknight reviewed Jul 9, 2019

View reviewed changes

owenv force-pushed the pound_variadic_expr branch from 16abe55 to edb093d Compare July 10, 2019 14:23

owenv added 10 commits July 10, 2019 23:37

[Parse] Add initial support for parsing a new #variadic directive as …

62c7f4f

…a VarargExpansionExpr

[Sema] Tiny CSGen changes needed to support explicit VarargExpansionE…

ea418d9

…xprs

Clean up #variadic parsing

0fd419c

Improve the varargs conversion diag

4230aaf

Add new fixit offering to wrap arrays with #variadic

c46b303

better diags for improper #variadic usage

f74b993

Miscellaneous bug fixes and tests for #variadic

fde152f

Port variadic conversion diagnostics to the new diags framework.

0b04fdf

This implementation isn't quite right yet because we're not yet checking the parameter flags to see if the param is actually variadic.

Store the ParameterTypeFlags on ApplyArgToParam constraint locators

75debdd

Cherry-picks hamishknight@7e3a601 with some deletions This is needed to support the ExpandArrayIntoVarargs ConstraintFix, which needs to be able to determine whether a parameter is variadic

Remove original CSDiag.cpp implementation of variadic conversion diags

4e4a88b

owenv force-pushed the pound_variadic_expr branch from edb093d to 4e4a88b Compare July 11, 2019 14:48

hamishknight reviewed Jul 11, 2019

View reviewed changes

owenv added 12 commits July 11, 2019 23:17

Post-rebase refactoring of #variadic

02a5b12

Initial implementation of the new as T... syntax for array-to-varar…

9f5cb7f

…gs coercion - The diags and tests have not yet been updated to reflect the new syntax - The old syntax hasn't been removed yet - But, some type checking hacks the old syntax used have been removed.

Remove #variadic syntax in favor of 'as T...'

0cf9ecd

Update interpreter test to use the new as T... syntax for varargs exp…

eccee1d

…ansion

working on tests and fixes for as T...

09d51e5

fix compiler crash when trying to coerce an array to varargs with a n…

40485ad

…on-variadic argument

Reenable diagnostic requiring as T... only appears in an argument pos…

6bfacf2

…ition

Remove old syntax support for the abandoned #variadic syntax

0a98fec

Fixup most of the as T... diags to reflect the new syntax

f64fe96

Conversion diag fixes for as T...

c6b66a8

Remove the last few references to the #variadic syntax for vararg exp…

a5d7a94

…ansion

more varargs expansion refactoring

6924cc7

hamishknight reviewed Jul 14, 2019

View reviewed changes

owenv added 2 commits July 14, 2019 10:16

Fix as T... ellipsis parsing bug, add a couple tests to make sure we …

3736af7

…maintain source compatibility with the infix ... operattor

Better parser recovery in the event someone tries to use as! or as? f…

92a9417

…or vararg expansion

owenv added 2 commits July 14, 2019 13:41

Have VarargExpansionExpr forward source locations to its subexpr again

8667177

Add a couple more vararg expansion tests

d83b32f

This was referenced Jul 15, 2019

Add proposal: as T... Syntax to Pass Array Elements as Varargs swiftlang/swift-evolution#1060

Closed

[Diagnostics] Improve diagnostic when attempting to pass an Array to a variadic argument #26207

Merged

owenv mentioned this pull request Jul 28, 2019

Add proposal: Prefix * Varargs Splat Operator for Arrays swiftlang/swift-evolution#1065

Closed

rjmccall added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Aug 14, 2019

owenv closed this Oct 10, 2019

[DNM][requires-evolution] Explicit varargs expansion expression #25997

[DNM][requires-evolution] Explicit varargs expansion expression #25997

Uh oh!

Conversation

owenv commented Jul 8, 2019

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

hamishknight Jul 9, 2019 • 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!

owenv commented Jul 11, 2019

Uh oh!

hamishknight 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!

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!

owenv commented Oct 10, 2019

Uh oh!

Uh oh!

hamishknight Jul 9, 2019 •

edited

Loading