[TypeChecker] Incremental multi-statement closure type-checking (disabled by default) #38577

xedin · 2021-07-22T20:52:13Z

This is an attempt no improve multi-statement closure handling in the type-checker
to provide consistent semantics (expect to inout inference), better diagnostics and
code completion for multi-statement closures (including result builders).

There were attempts to improve this behavior over the years, the latest one being
#32223 by Doug Gregor. All of them ran into technical
difficulties related to internal organization of the type-checker, because multi-statement
closures, just like function/subscript/setter bodies, tend be composed of a substantial
number of statements and cannot be efficiently type-checked the same way as
single-expression closures are. This proposal is backed by a new approach that uses
incremental type-checking - each statement in the body is split into a number of independently
type-checkable units - e.g. expression, declaration, pattern binding etc., these
self-contained units are type-checked independently and inferred types are recorded
to be used for type-checking of subsequent units. Because inference is done unit-by-unit
and all unnecessary intermediate information is disregarded, this scheme is as efficient as
approach used to type-check e.g. function bodies.

New constraints:
- Conjunction - represents a logical and relationship between its nested constraints, if one of the elements fails to produce a solution the conjunction as a whole is considered failed. There are two types of conjunctions - isolated and non-isolated. "Isolated" means that each of the elements has to be solved without any outside context unless it explicitly references a type variable e.g. anonymous closure parameter, capture or a closure result if element is a closure body node.
- Closure body element - represents a single AST node found in the body of the closure e.g. statement, declaration, or expression. Simplification of this constraint triggers constraint generation for the AST node. Although this constraint doesn't have any direct types it is allowed to references type variables e.g. if AST refers to some declarations found in other nodes.
New step - ConjunctionStep - used to solve conjunction constraints, similar in that regard to existing DisjunctionStep, but has different semantics, namely, all of its elements have to produce a single solution for step to be considered successful;
- Upon exhaustion of isolated conjunction, step returns all of the omitted external constraints back to the system and attempts to continue solving.
Constraint generation for multi-statement closures happens incrementally across and inside of each of the statements in the body by means of generating a conjunction for each top-level and sub-elements, such conjunctions would have aforementioned closure body elements as its nested constraints that easier expanded in a new (smaller) conjunction (if it's a statement) or a set of solvable constraints (for declarations and expressions). This is helpful for diagnostics as well as performance because fail of a current element means omission of the rest.

xedin · 2021-07-22T20:53:32Z

It's still early but everything is set up to start working on statements.

xedin · 2021-08-25T04:53:45Z

@swift-ci please test source compatibility

xedin · 2021-08-27T03:39:24Z

@swift-ci please test source compatibility

xedin · 2021-08-28T03:57:24Z

@swift-ci please test source compatibility

xedin · 2021-09-03T00:53:07Z

@swift-ci please test source compatibility

xedin · 2021-09-08T04:43:29Z

@swift-ci please test source compatibility

sarru

don't depend* on anything except* ( mistake in pull request description)

hborla

I'm about half way through the commits, but here's a first round of questions. All of the added constraint generation and application for various statement kinds looks relatively straightforward to me, and the questions below are related to the new conjunction constraint infrastructure.

hborla · 2021-10-06T21:10:57Z

lib/Sema/CSStep.cpp

@@ -354,6 +354,9 @@ StepResult ComponentStep::take(bool prevFailed) {
    // Produce a disjunction step.
    return suspend(
        std::make_unique<DisjunctionStep>(CS, disjunction, Solutions));
+  } else if (auto *conjunction = CS.selectConjunction()) {


Mostly curious - what is the reason for attempting conjunctions after disjunctions? Presumably any disjunction that affects the contextual closure type will have already been attempted in order to generate the conjunction, right?

It's just models the way how closures used to be type-checked before, wait until everything that can be resolved.

hborla · 2021-10-06T21:18:29Z

include/swift/Sema/Constraint.h

@@ -780,6 +788,10 @@ class Constraint final : public llvm::ilist_node<Constraint>,
  /// e.g. coercion constraint "as X" which forms a disjunction.
  bool isExplicitConversion() const;

+  /// Determine whether this constraint should be solved in isolation
+  /// from the rest of the constraint system.


This feels similar to what one-way constraints effectively do. Can you elaborate on what the difference here is?

One-way constraints form a separate component for left-hand side type variable, in contrast isolated closures have to remove all of the inactive constraints and unrelated type variables and preserve only type variables related to parameter and result, they can't be separated into a component like one-way are because all of the constraints around a closure would have to be brought back after the body is type-checked.

hborla · 2021-10-06T21:26:05Z

lib/Sema/CSStep.cpp

+    CS.addTypeVariable(typeVar);
+
+  // Make sure that element is solved in isolation
+  // by dropping all scoring information.


Hmm, I don't understand how the score is related to solving the element in isolation. Could you please explain a bit more?

Because score from the outside is unrelated to the elements in the body, solutions can't be ranked on score from context and score from other elements because they are standalone units for type-checker, only types are transferable between them.

hborla

This looks really solid. I didn't notice anything suspicious in the remainder of the commits, and your explanatory comments are really helpful.

lib/Sema/CSClosure.cpp

xedin · 2021-10-08T04:31:41Z

I'm going to land this since all the changes are hidden under the flag -experimental-multi-statement-closures, and I want to start using underlying mechanism for result builders. If new semantics are accepts into the language I'm going to open a follow-up commit to default the flag to true.

xedin · 2021-10-08T04:31:54Z

@swift-ci please clean test

xedin · 2021-10-08T04:32:11Z

@swift-ci please test source compatibility

swift-ci · 2021-10-08T05:24:50Z

Build failed
Swift Test Linux Platform
Git Sha - 22e3fc6bdf97a4b29511e679d6f98f60878af2a7

swift-ci · 2021-10-08T07:01:57Z

Build failed
Swift Test OS X Platform
Git Sha - 22e3fc6bdf97a4b29511e679d6f98f60878af2a7

While solving a conjunction that represents a (multi-statement) closure constraint system should use such closure as its declaration context, otherwise member lookup would produce incorrect results.

Let's delay solution application to multi-statement closure bodies, because declarations found in the body of such closures may depend on its `ExtInfo` flags e.g. no-escape is set based on parameter if closure is used in a call.

It is possible that contextual type of a parameter has been assigned to an anonymous of named argument early, to facilitate closure type checking. Such a type can have type variables inside e.g. ```swift func test<T>(_: (UnsafePointer<T>) -> Void) {} test { ptr in ... } ``` Type variable representing `ptr` in the body of this closure would be bound to `UnsafePointer<$T>` in this case, where `$T` is a type variable for a generic parameter `T`.

Cant assume that `shouldTypeCheckInEnclosingExpression` always implies a single expression closure anymore.

…ts incrementally Attempting to pre-compute a set of referenced type variables upfront is incorrect because parameter(s) and/or result type could be bound before conjunction is attempted. Let's compute a set of referenced variables before each element gets attempted.

If one or more outer closure parameters used in the body of an inner closure have not been resolved when conjunction is created, let's reference them in the conjunction constraint to avoid disconnecting conjunction from its context.

…ional `Void` In cases like: ``` func test<T>(_: () -> T?) -> [T] { ... } test { if ... { return } ... } ``` Contextual return type would be first attempted as optional and then unwrapped if the first attempt did not succeed. To avoid having to solve the closure twice (which results in an implicit function conversion), let's add an implicit empty tuple (`()`) to the `return` statement and allow it be to injected into optional as many times as context requires.

Each return statement gets a contextual type from outer context, so the best model for that is via solution application target.

Let `visitForEachPattern` handle both pattern and the sequence expression associated with `for-in` loop because types in this situation flow in both directions: - From pattern to sequence, informing its element type e.g. `for i: Int8 in 0 ..< 8` - From sequence to pattern, when pattern has no type information.

… a closure

Solution application needs to set interface types for variables declared in a case statement, otherwise this would lead to crashes in SILGen.

…r type variables referenced It's possible to declare explicit parameter/result type without providing generic arguments, in such situation they'd be "opened" as type variables and inferred from the contextual type or the body. Type variable collector needs to account for that and include such inner type variables into scope of an isolated closure.

…erator::visitIfStmt`

…is enabled

… inference is disabled This restores original pre-check behavior and is important for diagnostics like `'_' can only appear in a pattern or on the left side of an assignment`.

xedin · 2021-10-08T19:31:39Z

@swift-ci please clean test

xedin · 2021-10-08T19:32:35Z

@swift-ci please test source compatibility release

swift-ci · 2021-10-08T22:29:26Z

Build failed
Swift Test OS X Platform
Git Sha - f074305

xedin · 2021-10-08T22:47:54Z

There is C++ interop related test-case failure in watchsimulator, going to run smoke test instead and land this.

xedin · 2021-10-08T22:48:03Z

@swift-ci please smoke test macOS platform

xedin · 2021-10-08T23:50:18Z

@swift-ci please test Windows platform

xedin · 2021-10-09T03:56:22Z

@swift-ci please test macOS platform

xedin · 2021-10-09T03:56:51Z

@swift-ci please test Windows platform

xedin · 2021-10-09T16:45:15Z

@swift-ci please test Windows platform

xedin requested a review from hborla July 22, 2021 20:52

xedin force-pushed the multi-statement-closure-type-checking branch 2 times, most recently from 26766e8 to f782357 Compare July 24, 2021 01:36

xedin force-pushed the multi-statement-closure-type-checking branch 3 times, most recently from 13fda4f to 033e1e4 Compare August 10, 2021 20:21

xedin force-pushed the multi-statement-closure-type-checking branch from 033e1e4 to d4d4e35 Compare August 25, 2021 04:53

xedin force-pushed the multi-statement-closure-type-checking branch from d4d4e35 to 2790e7c Compare August 27, 2021 03:38

xedin force-pushed the multi-statement-closure-type-checking branch from 2790e7c to 6893ca0 Compare August 28, 2021 03:57

xedin force-pushed the multi-statement-closure-type-checking branch from 6893ca0 to 58c96ce Compare September 3, 2021 00:52

xedin force-pushed the multi-statement-closure-type-checking branch from 58c96ce to 7470e79 Compare September 8, 2021 00:36

sarru reviewed Sep 8, 2021

View reviewed changes

xedin force-pushed the multi-statement-closure-type-checking branch 3 times, most recently from 32c839a to 8ecbf6f Compare September 24, 2021 00:31

xedin force-pushed the multi-statement-closure-type-checking branch 2 times, most recently from 1fa82ac to 22e3fc6 Compare October 6, 2021 19:49

hborla reviewed Oct 6, 2021

View reviewed changes

hborla approved these changes Oct 8, 2021

View reviewed changes

lib/Sema/CSClosure.cpp Outdated Show resolved Hide resolved

xedin added 17 commits October 8, 2021 10:08

[CSStep] Conjunctions representing closures affect declaration context

10fa811

While solving a conjunction that represents a (multi-statement) closure constraint system should use such closure as its declaration context, otherwise member lookup would produce incorrect results.

[ResultBuilders] Fix a crash in PreCheckResultBuilderApplication

da3cef2

Cant assume that `shouldTypeCheckInEnclosingExpression` always implies a single expression closure anymore.

[CSClosure] Avoid creating conjunctions for empty bodies

dfe6c34

[CSApply] Set types for for all delayed closures

9087613

[CSClosures] Model return statements as solution application targets

29ec113

Each return statement gets a contextual type from outer context, so the best model for that is via solution application target.

[CSClosure] Check parameters after solution is applied to the body of…

b7b492f

… a closure

[CSClosure] Make sure that case body variables get types

e5db01b

Solution application needs to set interface types for variables declared in a case statement, otherwise this would lead to crashes in SILGen.

[CSClosure] NFC: Add size to a SmallVector in `ClosureConstraintGen…

18b23aa

…erator::visitIfStmt`

[CSClosure] Check multi-statement closure attrs only in if inference …

87f41a9

…is enabled

[PreCheck] Allow pre-check to walk into patterns when multi-statement…

f074305

… inference is disabled This restores original pre-check behavior and is important for diagnostics like `'_' can only appear in a pattern or on the left side of an assignment`.

xedin force-pushed the multi-statement-closure-type-checking branch from 1f384ea to f074305 Compare October 8, 2021 19:31

xedin changed the title ~~[WIP][TypeChecker] Incremental multi-statement closure type-checking~~ [TypeChecker] Incremental multi-statement closure type-checking Oct 9, 2021

xedin changed the title ~~[TypeChecker] Incremental multi-statement closure type-checking~~ [TypeChecker] Incremental multi-statement closure type-checking (disabled by default) Oct 9, 2021

xedin merged commit 790b549 into swiftlang:main Oct 9, 2021

[TypeChecker] Incremental multi-statement closure type-checking (disabled by default) #38577

[TypeChecker] Incremental multi-statement closure type-checking (disabled by default) #38577

Uh oh!

Conversation

xedin commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Jul 22, 2021

Uh oh!

xedin commented Aug 25, 2021

Uh oh!

xedin commented Aug 27, 2021

Uh oh!

xedin commented Aug 28, 2021

Uh oh!

xedin commented Sep 3, 2021

Uh oh!

xedin commented Sep 8, 2021

Uh oh!

sarru left a comment

Choose a reason for hiding this comment

Uh oh!

hborla left a comment

Choose a reason for hiding this comment

Uh oh!

hborla Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

xedin Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

hborla Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

xedin Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

hborla Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

xedin Oct 6, 2021

Choose a reason for hiding this comment

Uh oh!

hborla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xedin commented Oct 8, 2021

Uh oh!

xedin commented Oct 8, 2021

Uh oh!

xedin commented Oct 8, 2021

Uh oh!

swift-ci commented Oct 8, 2021

Uh oh!

swift-ci commented Oct 8, 2021

Uh oh!

xedin commented Oct 8, 2021

Uh oh!

xedin commented Oct 8, 2021

Uh oh!

swift-ci commented Oct 8, 2021

Uh oh!

xedin commented Oct 8, 2021

Uh oh!

xedin commented Oct 8, 2021

Uh oh!

xedin commented Oct 8, 2021

Uh oh!

xedin commented Oct 9, 2021

Uh oh!

xedin commented Oct 9, 2021

Uh oh!

xedin commented Oct 9, 2021

Uh oh!

Uh oh!

xedin commented Jul 22, 2021 •

edited

Loading