Skip to content

[6.0] Support closures that capture opened pack element types #73541

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

Merged

Conversation

slavapestov
Copy link
Contributor

@slavapestov slavapestov commented May 9, 2024

Partial 6.0 cherry-pick of #73712 and previous PRs.

  • Description: Closures can capture values of pack element type, which are introduced by a repeat expression or a for ... in repeat loop. For example, here we accidentally capture a pack element type, because && takes an autoclosure:

    protocol P { var foo: Bool { get } }
    
    func f<each T: P>(_ ts: repeat each T) {
      for t in repeat each ts {
        _ = true && t.foo
      }
    }
    

    This would cause crashes and type checking errors. This PR implements the SIL lowering support for closures that capture pack elements. Some solver support is incomplete, however a bunch of cases now work.

  • Scope of the issue: Affects many users, and is even easier to hit now with for ... in repeat loops.

  • Origination: This never worked.

  • Risk: I believe Low for code that doesn't use packs, Medium for code using packs. There's a bunch of stuff here:

    • I redid ASTVerifier support for local archetypes. This is off in an noassert build.
    • I changed the HasArchetype bit in TypeBase into HasPrimaryArchetype and added a hasPrimaryArchetype() predicate to test it, but the existing hasArchetype() predicate has the same behavior as before. I only updated a few callers to use the new hasPrimaryArchetype().
    • There are some new assertions. I made a workaround in autodiff to simulate the old noassert behavior in a narrow case
    • Capture analysis now tracks uses of local archetypes. Any existing code that didn't crash before should end up getting an empty list of local archetype captures here. But a logic error might break existing code that uses packs.
    • Type lowering now consults the list of captured archetypes. If it's not empty, a bunch of new code runs to build a new generic signature, extend the substitution map for the call, etc. I hooked this into the existing mechanism which drops the generic signature for a closure in generic context that has no generic captures. It should all short-circuit trivially if the list of captured archetypes is empty.
    • There is a new pass that runs after SILGen to rewrite SIL instructions in a non-trivial way. This pass only runs if there were captured local archetypes.
    • There's a representation change for variables declared within the scope of a for ... in repeat loop. Some extra book-keeping that used to exist is now subsumed into my new logic. This might break existing code, but pack iteration is a new feature in 6.0. This change was separately reviewed on main by @simanerush.
    • For the most part validation tests pass at each intermediate commit so it should be easy to bisect any regressions.
  • Reviewed by: review pending - @hborla @rjmccall @simanerush

  • Radar: rdar://113505724, rdar://122293832, rdar://124329076.

@slavapestov slavapestov requested a review from a team as a code owner May 9, 2024 17:00
@slavapestov slavapestov marked this pull request as draft May 9, 2024 17:01
@slavapestov slavapestov force-pushed the pack-expansion-closures-6.0 branch 7 times, most recently from 6a9ef4c to 6181034 Compare May 17, 2024 20:34
@slavapestov slavapestov force-pushed the pack-expansion-closures-6.0 branch 6 times, most recently from 3b6a294 to a203283 Compare May 21, 2024 02:23
@slavapestov slavapestov force-pushed the pack-expansion-closures-6.0 branch from a203283 to f6d6774 Compare May 21, 2024 02:24
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov
Copy link
Contributor Author

@swift-ci Please test source compatibility

@slavapestov slavapestov marked this pull request as ready for review May 21, 2024 11:33
@eeckstein
Copy link
Contributor

@slavapestov
Copy link
Contributor Author

(swift-distributed-actors fails with the same error in the baseline source compat run)

On main I refactored it away, but here I'm just renaming the old one to
avoid a name conflict.
The existing hasArchetype() is now deprecated. It is equivalent to:

    hasPrimaryArchetype() || hasLocalArchetype()

Callers should be changed to check one or both of the above predicates
in the most precise way possible.
We don't need to build a DenseMap here. More importantly, this
changes the logic to avoid calling mapTypeOutOfContext() on
element archetypes, instead doing the mapping directly.
…nresolved local archetypes

We maintained a counter of the number of pending local archetypes
that had not yet been defined. However, if an instruction that
references a pending local archetype was deleted before the
local archetype was defined, the counter would never decrement.

Before reading the counter value, garbage collect any inserted
placeholders that have no uses. These correspond to pending
local archetypes that are no longer in use and will never be
defined.
This implements support for autoclosures, closures and local functions
nested within a pack iteration for loop.

The combination of explicit closure expressions and pack expansion
expressions still needs some work.

Fixes swiftlang#66917.
Fixes swiftlang#69947.
Fixes rdar://113505724.
Fixes rdar://122293832.
Fixes rdar://124329076.
@slavapestov slavapestov force-pushed the pack-expansion-closures-6.0 branch from f6d6774 to 4c36819 Compare May 21, 2024 18:06
@slavapestov
Copy link
Contributor Author

@swift-ci Please test

@slavapestov slavapestov merged commit 8c230de into swiftlang:release/6.0 May 23, 2024
5 checks passed
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