Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TypeChecker] SE-0326: Enable multi-statement closure inference by default #39989

Merged

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Oct 30, 2021

  • Adjusts all of the improved test-cases
  • Fixes all the troubles with syntactic diagnostics inside of multi-statement closures (availability, constness, .self use, local declarations etc.)
  • Makes constraint system respect LeaveClosureBodyUnchecked flag
  • Fixes DebuggerTestingTransform to avoid re-using AST nodes
  • Restores discarded expressions warnings (expressions appearing directly in a brace statement)
  • Allows conjunctions to be considered before closure result or generic parameters are bound to holes, which improves diagnostic experience because result types could now be inferred from the body, so no more extraneous "cannot infer generic parameter '...'" diagnostics relates to multi-statement closures.
  • Enables multi-statement closure inference by default.

@xedin xedin requested a review from hborla October 30, 2021 03:07
@xedin
Copy link
Contributor Author

xedin commented Oct 30, 2021

@hborla This is a draft because I'd like to wait for the Core Team decision on this and if everything goes fine I'm just going to drop frontend flag in a follow-up commit...

@xedin xedin changed the title [TypeChecker] Prepare for multi-statement inference being enabled by default [TypeChecker] SE-0326: Enable multi-statement closure inference by default Nov 1, 2021
@xedin
Copy link
Contributor Author

xedin commented Nov 1, 2021

@swift-ci please clean test

@xedin
Copy link
Contributor Author

xedin commented Nov 1, 2021

@swift-ci please test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Nov 1, 2021

Build failed
Swift Test Linux Platform
Git Sha - 1d84a6d4d4b2fe82911fd5b8b8d7b12b23c1745a

@xedin xedin force-pushed the more-diag-improvements-for-multi-stmt-closures branch from 1d84a6d to 3ddd414 Compare November 1, 2021 20:20
@xedin
Copy link
Contributor Author

xedin commented Nov 1, 2021

@swift-ci please clean test

@xedin
Copy link
Contributor Author

xedin commented Nov 1, 2021

@swift-ci please test source compatibility

@xedin xedin force-pushed the more-diag-improvements-for-multi-stmt-closures branch from 3ddd414 to fa78540 Compare November 3, 2021 01:45
@xedin
Copy link
Contributor Author

xedin commented Nov 3, 2021

@swift-ci please test source compatibility

@xedin
Copy link
Contributor Author

xedin commented Nov 3, 2021

@swift-ci please clean test

@swift-ci
Copy link
Contributor

swift-ci commented Nov 3, 2021

Build failed
Swift Test OS X Platform
Git Sha - fa78540c0de3a515b9436551a86c236359692963

@xedin
Copy link
Contributor Author

xedin commented Nov 3, 2021

Infra issues, both source compat and macOS failed with EOFException, re-running.

@xedin
Copy link
Contributor Author

xedin commented Nov 3, 2021

@swift-ci please test macOS platform

@xedin
Copy link
Contributor Author

xedin commented Nov 3, 2021

@swift-ci please test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Nov 3, 2021

Build failed
Swift Test OS X Platform
Git Sha - fa78540c0de3a515b9436551a86c236359692963

@xedin
Copy link
Contributor Author

xedin commented Nov 3, 2021

@swift-ci please clean test macOS platform

@xedin xedin marked this pull request as ready for review November 8, 2021 18:35
xedin added 11 commits November 15, 2021 16:42
…ambiguous

If there are multiple overloads and all of them require closure
to have a parameter, let's diagnose that as such instead of
ambiguity.
…d code

While building a closure to inject `checkExpect` code, clone member
references associated with assignment. Re-using AST nodes is generally
invalid. This is visible with multi-statement closure inference enabled,
because the body is type-checked together with enclosing context
and both elements end up sharing `DeclRefExpr` and `MemberRefExpr`s.
This preserves previous behavior where multi-statement closures
where always type-checked without context.
…Unchecked` flag

Propagate `LeaveClosureBodyUnchecked` flag from `typeCheckASTNodeAtLoc`
down to declaration checker to skip checking closures associated with
pattern binding entry initializers. This is no-op until multi-statement
inference becomes enabled by default.
…es when inference is enabled

When multi-statement closure inference is enabled it's body is
type-checked together with enclosing context, so they could be
walked directly just like single-expressions ones.
… closures

Extract diagnostic into a method and use it while type-checking
`for-in` in top-level code and in closures.
…t inference is enabled

Scope down previous check to avoid walking into patterns that appear
in multi-statement closures if the inference is enabled.
Preserve pre SE-0326 for code completion, so it could be ported
gradually.
Put some common logic related to local declaration to the base
class and refactor other walkers to use it instead of `ASTWalker`.
…ic parameter holes

Closure result type or generic parameter associated with such a location
could bw inferred from a body of a multi-statement closure (when inference
is enabled), so we need to give closure a chance to run before attemtping
a hole for such positions in diagnostic mode.
Each of the elements in the result builder has to be fully pre-checked
now that multi-statement inference has been enabled.
@xedin xedin force-pushed the more-diag-improvements-for-multi-stmt-closures branch from fa78540 to b231cde Compare November 16, 2021 02:33
@xedin
Copy link
Contributor Author

xedin commented Nov 16, 2021

@hborla I dropped two commits that removed the frontend, instead for the time being it would just be enabled by default. We can clean it up later.

@xedin
Copy link
Contributor Author

xedin commented Nov 16, 2021

@swift-ci please clean test

Copy link
Member

@hborla hborla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagnostic changes in the test suite look awesome 👍🏻

@xedin
Copy link
Contributor Author

xedin commented Nov 16, 2021

@swift-ci please test Windows platform

3 similar comments
@xedin
Copy link
Contributor Author

xedin commented Nov 16, 2021

@swift-ci please test Windows platform

@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2021

@swift-ci please test Windows platform

@xedin
Copy link
Contributor Author

xedin commented Nov 17, 2021

@swift-ci please test Windows platform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants