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

[AutoDiff upstream] Relax @differentiable for protocol witnesses. #30629

Merged
merged 1 commit into from
Mar 25, 2020

Conversation

dan-zheng
Copy link
Contributor

Previously, all witnesses of a @differentiable protocol requirement were
required to have the same attribute (or one with superset parameter indices).

However, this leads to many annotations on witnesses and is not ideal for
usability. @differentiable attributes are really only significant on
public witnesses, so that they are clearly @differentiable at a glance (in
source code, interface files, and API documentation), without looking through
protocol conformance hierarchies.

Now, only public witnesses of @differentiable protocol requirements are
required to have the same attribute (or one with superset parameter indices).
For less-visible witnesses, an implicit @differentiable attribute is created
with the same configuration as the requirement's.

Resolves TF-1117.
Upstreams #29771 from tensorflow branch.


Example:

public protocol Layer: Differentiable {
  associatedtype Input: Differentiable
  associatedtype Output: Differentiable
  @differentiable(wrt: (self, input))
  func callAsFunction(_ input: Input) -> Output
}

// Internal conforming type.
struct DummyInternalLayer: Layer {
  func callAsFunction(_ input: Float) -> Float {
    return input
  }
}

Before (misleading diagnostic due to TF-1014):

layer.swift:9:8: error: type 'DummyInternalLayer' does not conform to protocol 'Layer'
struct DummyInternalLayer: Layer {
       ^
layer.swift:2:18: note: protocol requires nested type 'Input'; do you want to add it?
  associatedtype Input: Differentiable
                 ^
layer.swift:3:18: note: protocol requires nested type 'Output'; do you want to add it?
  associatedtype Output: Differentiable
                 ^

After: no error.

An implicit @differentiable attribute is created for DummyInternalLayer.callAsFunction:

$ swiftc -print-ast layer.swift
...

internal struct DummyInternalLayer : Layer {
  @differentiable(wrt: (self, input))
  internal func callAsFunction(_ input: Float) -> Float
  ...
}

Previously, all witnesses of a `@differentiable` protocol requirement were
required to have the same attribute (or one with superset parameter indices).

However, this leads to many annotations on witnesses and is not ideal for
usability. `@differentiable` attributes are really only significant on
public witnesses, so that they are clearly `@differentiable` at a glance (in
source code, interface files, and API documentation), without looking through
protocol conformance hierarchies.

Now, only *public* witnesses of `@differentiable` protocol requirements are
required to have the same attribute (or one with superset parameter indices).
For less-visible witnesses, an implicit `@differentiable` attribute is created
with the same configuration as the requirement's.

Resolves TF-1117.
@dan-zheng dan-zheng requested review from rxwei and marcrasi March 25, 2020 09:27
@dan-zheng
Copy link
Contributor Author

@swift-ci Please smoke test

@dan-zheng
Copy link
Contributor Author

Merging to unblock progress. #29771 was previously approved.

@dan-zheng dan-zheng merged commit 07596cb into swiftlang:master Mar 25, 2020
@dan-zheng dan-zheng deleted the autodiff-upstream branch March 25, 2020 15:13
kovdan01 added a commit to kovdan01/swift that referenced this pull request Jan 15, 2025
In AutoDiff/Sema/differentiable_attr_type_checking.swift, we have a
couple of following FIXMEs:

```
// FIXME(TF-284): Fix unexpected diagnostic.
```

However, the diagnostic is expected for the case of public protocol
requirements: see description swiftlang#30629.
This PR removed the diagnostic for less-than-public-visible requirements,
and the FIXME was initially related to them.

It looks like that the FIXMEs present now are a result of copy-paste and
have no meaning, and the diagnostic is expected and should be present and
does not need to be removed.

Fixes swiftlang#78609
asl pushed a commit that referenced this pull request Jan 16, 2025
In AutoDiff/Sema/differentiable_attr_type_checking.swift, we have a
couple of following FIXMEs:

```
// FIXME(TF-284): Fix unexpected diagnostic.
```

However, the diagnostic is expected for the case of public protocol
requirements: see description #30629.
This PR removed the diagnostic for less-than-public-visible requirements,
and the FIXME was initially related to them.

It looks like that the FIXMEs present now are a result of copy-paste and
have no meaning, and the diagnostic is expected and should be present and
does not need to be removed.

Fixes #78609
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.

1 participant