Skip to content

[Features] Rename the BuiltinBuildTaskExecutor feature guard. #71599

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

hborla
Copy link
Member

@hborla hborla commented Feb 14, 2024

The name of the TaskExecutor protocol was recently changed in #71145 to remove underscores after the task executor preference proposal was accepted in Swift Evolution. An implication of that rename is that the buildOrdinaryTaskExecutorRef builtin changed the type that it expected as the argument. However, the original _TaskExecutor protocol landed in the _Concurrency library which as since produced .swiftinterface files that contain the following @inlinable code:

@inlinable public init<E: _TaskExecutor>(ordinary executor: __shared E) {
  #if $BuiltinBuildTaskExecutor
  self.executor = Builtin.buildOrdinaryTaskExecutorRef(executor)
  #else
  fatalError("Swift compiler is incompatible with this SDK version")
  #endif
}

When a compiler containing the TaskExecutor protocol rename attempts to type check the above @inlinable code when rebuilding the _Concurrency module from its .swiftinterface file, it crashes because the builtin is expecting an argument conforming to TaskExecutor, which doesn't exist in this version of the standard library. Specifically, the crash happens here:

  case _taskExecutor:
    return SC.Context.getProtocol(KnownProtocolKind::TaskExecutor)
      ->getDeclaredInterfaceType();

The issue is that the current compiler still supports the $BuiltinBuildTaskExecutor feature guard, and thus attempts to typecheck self.executor = Builtin.buildOrdinaryTaskExecutorRef(executor), but the type of executor conforms to _TaskExecutor instead of TaskExecutor. Because TaskExecutor doesn't exist in this version of the _Concurrency library, SC.Context.getProtocol(KnownProtocolKind::TaskExecutor) returns nullptr.

To resolve this issue, we need to stop supporting the $BuiltinBuildTaskExecutor feature guard and introduce a new one that is only supported by compiler versions that contain the TaskExecutor rename. This approach relies on nothing having adopted the API, otherwise we would need to stage in the rename as a parallel set of APIs, and only remove the old APIs once nothing is relying on the old _Concurrency .swiftinterface files. With this rename of the feature guard, compilers type checking older _Concurrency.swiftinterface files will behave as if they do not support the feature, and thus will not attempt to type check the code guarded by #if $BuiltinBuildTaskExecutor.

The name of the `TaskExecutor` protocol was recently changed to remove
underscores after the feature was accepted in Swift Evolution. An implication
of that rename is that the `buildOrdinaryTaskExecutorRef` builtin changed
the type that it expected as the argument. However, the original change
landed in the standard library which as since produced swiftinterfaces
that contain the following inlinable code:

```
@inlinable public init<E>(ordinary executor: __shared E) where E : _Concurrency._TaskExecutor {
  #if $BuiltinBuildTaskExecutor
  self.executor = Builtin.buildOrdinaryTaskExecutorRef(executor)
  #else
  fatalError("Swift compiler is incompatible with this SDK version")
  #endif
}
```

When a compiler containing the protocol rename attempts to type check the
above inlinable code, it crashes because the builtin is expecting an argument
conforming to `TaskExecutor`, which doesn't exist in this version of the
standard library. The issue is that the current compiler still supports
the `$BuiltinBuildTaskExecutor` feature guard, but the builtin supported
has since changed.

To resolve this issue, we need to stop supporting the `$BuiltinBuildTaskExecutor`
feature guard and introduce a new one that is only supported by compiler versions
that contain the rename. This approach relies on nothing having adopted the
API, otherwise we would need to stage in the rename as a parallel set of APIs,
and only remove the old APIs once nothing is relying on the old _Concurrency
swiftinterfaces.
@hborla
Copy link
Member Author

hborla commented Feb 14, 2024

@swift-ci please smoke test

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, thank you -- didn't realize we need to do that, will keep an eye out for those!

@shahmishal shahmishal merged commit 61ed95a into swiftlang:main Feb 14, 2024
@hborla hborla deleted the update-build-task-executor-feature-guard branch February 14, 2024 16:23
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.

4 participants