Skip to content

[AutoDiff upstream] forbid @derivative of protocol req #29031

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.

Sign up for GitHub

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
swift-ci merged 2 commits into from
Jan 7, 2020
+37 −17
Merged

[AutoDiff upstream] forbid @derivative of protocol req #29031

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d69e892
[AutoDiff upstream] forbid @derivative of protocol req
Jan 6, 2020
29465f8
Add standalone test.
dan-zheng Jan 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/Sema/TypeCheckAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3503,6 +3503,9 @@ static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D,
};

auto isValidOriginal = [&](AbstractFunctionDecl *originalCandidate) {
// TODO(TF-982): Allow derivatives on protocol requirements.
if (isa<ProtocolDecl>(originalCandidate->getDeclContext()))
return false;
return checkFunctionSignature(
cast<AnyFunctionType>(originalFnType->getCanonicalType()),
originalCandidate->getInterfaceType()->getCanonicalType(),
Expand Down
34 changes: 17 additions & 17 deletions test/AutoDiff/Sema/derivative_attr_type_checking.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,11 @@ protocol StaticMethod: Differentiable {
static func generic<T: Differentiable>(_ x: T) -> T
}

@rxwei rxwei Jan 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel we need an error message stating clearly that defining derivatives for protocol requirements is not yet supported.

@dan-zheng dan-zheng Jan 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Customized diagnostics may require extra support in findAbstractFunctionDecl in TypeCheckAttr.cpp. Currently, when std::function<bool(AbstractFunctionDecl *)> &isValidCandidate returns false for all candidates, a single generic error is emitted (std::function<void()> &noneValidDiagnostic).

We could change isValidCandidate emit diagnostics as a side effect, but this requires clever diagnostic cancellation (via DiagnosticTransaction) in case a valid candidate is found.

Alternatively, we could keep track of all invalid candidates and change noneValidDiagnostic to std::function<void(AbstractFunctionDecl *)> &notValidDiagnostic, so customized diagnostics can be emitted for every invalid candidate.

@marcrasi marcrasi Jan 7, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This seems like more work than it's worth, especially since it's a temporary limitation, so I won't do this in this PR.

extension StaticMethod {
static func foo(_ x: Float) -> Float { x }
static func generic<T: Differentiable>(_ x: T) -> T { x }
}

extension StaticMethod {
@derivative(of: foo)
static func jvpFoo(x: Float) -> (value: Float, differential: (Float) -> Float)
Expand Down Expand Up @@ -215,11 +220,16 @@ extension StaticMethod {
// Test instance methods.

protocol InstanceMethod: Differentiable {
// expected-note @+1 {{'foo' defined here}}
func foo(_ x: Self) -> Self
func generic<T: Differentiable>(_ x: T) -> Self
}

extension InstanceMethod {
// expected-note @+1 {{'foo' defined here}}
func foo(_ x: Self) -> Self { x }

// expected-note @+1 {{'generic' defined here}}
func generic<T: Differentiable>(_ x: T) -> Self
func generic<T: Differentiable>(_ x: T) -> Self { self }
}

extension InstanceMethod {
Expand Down Expand Up @@ -539,24 +549,14 @@ extension HasStoredProperty {
// Test cross-file derivative registration. Currently unsupported.
// TODO(TF-1021): Lift this restriction.

extension AdditiveArithmetic where Self: Differentiable {
extension FloatingPoint where Self: Differentiable {
// expected-error @+1 {{derivative not in the same file as the original function}}
Comment thread
dan-zheng marked this conversation as resolved.
@derivative(of: +)
static func vjpPlus(x: Self, y: Self) -> (
@derivative(of: rounded)
func vjpRounded() -> (
value: Self,
pullback: (Self.TangentVector) -> (Self.TangentVector, Self.TangentVector)
) {
return (x + y, { v in (v, v) })
}
}

extension FloatingPoint where Self: Differentiable, Self == Self.TangentVector {
// expected-error @+1 {{derivative not in the same file as the original function}}
@derivative(of: +)
static func vjpPlus(x: Self, y: Self) -> (
value: Self, pullback: (Self) -> (Self, Self)
pullback: (Self.TangentVector) -> (Self.TangentVector)
) {
return (x + y, { v in (v, v) })
fatalError()
}
}

Expand Down