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
merged 2 commits into from
Jan 7, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
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
51 changes: 34 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
}

Copy link
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.

Copy link
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.

Copy link
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 @@ -536,27 +546,34 @@ extension HasStoredProperty {
}
}

// Test cross-file derivative registration. Currently unsupported.
// TODO(TF-1021): Lift this restriction.
// Test derivative registration for protocol requirements. Currently unsupported.
// TODO(TF-982): Lift this restriction and add proper support.

extension AdditiveArithmetic where Self: Differentiable {
// 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.TangentVector) -> (Self.TangentVector, Self.TangentVector)
) {
return (x + y, { v in (v, v) })
protocol ProtocolRequirementDerivative {
func requirement(_ x: Float) -> Float
}
extension ProtocolRequirementDerivative {
// NOTE: the error is misleading because `findAbstractFunctionDecl` in
// TypeCheckAttr.cpp is not setup to show customized error messages for
// invalid original function candidates.
// expected-error @+1 {{could not find function 'requirement' with expected type '<Self where Self : ProtocolRequirementDerivative> (Self) -> (Float) -> Float'}}
@derivative(of: requirement)
func vjpRequirement(_ x: Float) -> (value: Float, pullback: (Float) -> Float) {
fatalError()
}
}

extension FloatingPoint where Self: Differentiable, Self == Self.TangentVector {
// Test cross-file derivative registration. Currently unsupported.
// TODO(TF-1021): Lift this restriction.

extension FloatingPoint where Self: Differentiable {
// 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)
@derivative(of: rounded)
func vjpRounded() -> (
value: Self,
pullback: (Self.TangentVector) -> (Self.TangentVector)
) {
return (x + y, { v in (v, v) })
fatalError()
}
}

Expand Down