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] Require same access level for original/derivative functions. #31527

Merged
merged 3 commits into from May 6, 2020

Conversation

dan-zheng
Copy link
Collaborator

@dan-zheng dan-zheng commented May 4, 2020

Require @derivative functions and their original functions to have the same access level.
Public original functions may also have internal @usableFromInline derivatives, as a special case. Diagnose access level mismatches.

This simplifies derivative registration rules, and may enable simplification of AutoDiff symbol linkage rules.

Resolves TF-1099 and TF-1160.


Example:

import _Differentiation

public func publicOriginal(_ x: Float) -> Float { x }

@derivative(of: publicOriginal)
private func privativeDerivative(_ x: Float) -> (value: Float, pullback: (Float) -> Float) {
  fatalError()
}
$ swift example.swift
example.swift:5:17: error: derivative function must have same access level as original function; derivative function 'privativeDerivative' is private, but original function 'publicOriginal' is public
@derivative(of: publicOriginal)
                ^
example.swift:6:14: note: mark the derivative function as 'public' to match the original function
private func privativeDerivative(_ x: Float) -> (value: Float, pullback: (Float) -> Float) {
~~~~~~~      ^
public

Future direction: anonymous func declarations (SR-12003) will enable public derivative functions without exposing them as public API.

public func bar(_ x: Float) -> Float { x }

@derivative(of: bar)
public func _(_ x: Float) -> (value: Float, differential: (Float) -> Float) {
  (x, { dx in dx })
}

This additional restriction will break users. Fix all @derivative usage errors caught by "community CI" before merging.

Done: tensorflow/swift-apis#929

@dan-zheng dan-zheng requested review from rxwei and marcrasi May 4, 2020 03:19
@dan-zheng
Copy link
Collaborator Author

@swift-ci Please smoke test

Copy link
Member

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

This additional restriction will break users.

Well, this will break users by bringing possible linker errors to compile-time errors :)

Require `@derivative` functions and their original functions to have the same
access level. Public original functions may also have internal
`@usableFromInline` derivatives, as a special case. Diagnose access level
mismatches.

This simplifies derivative registration rules, and may enable simplification of
AutoDiff symbol linkage rules.

Resolves TF-1099 and TF-1160.
Use a single consistent error message stating "derivative function must have
same access level as original function".

Add access level fix-it on the derivative function.
@dan-zheng dan-zheng force-pushed the derivative-access-level-same branch from a6bba14 to 7851a74 Compare May 6, 2020 02:29
@dan-zheng dan-zheng requested a review from rxwei May 6, 2020 02:31
@dan-zheng dan-zheng force-pushed the derivative-access-level-same branch from fe0987b to 542eb4f Compare May 6, 2020 07:05
This is better than suggesting derivative access level change to public.
@dan-zheng dan-zheng force-pushed the derivative-access-level-same branch from 542eb4f to 2a23bfc Compare May 6, 2020 07:08
@dan-zheng dan-zheng requested a review from rxwei May 6, 2020 07:08
@dan-zheng
Copy link
Collaborator Author

@swift-ci Please test

@swift-ci
Copy link
Collaborator

swift-ci commented May 6, 2020

Build failed
Swift Test Linux Platform
Git Sha - a6bba14a2d7c6bee70e0da20c98bc65bdddd2082

@swift-ci
Copy link
Collaborator

swift-ci commented May 6, 2020

Build failed
Swift Test OS X Platform
Git Sha - a6bba14a2d7c6bee70e0da20c98bc65bdddd2082

@dan-zheng dan-zheng merged commit 5036c95 into apple:master May 6, 2020
@dan-zheng dan-zheng deleted the derivative-access-level-same branch May 6, 2020 12:55
hlopko pushed a commit to hlopko/swift that referenced this pull request May 6, 2020
…s. (apple#31527)

Require `@derivative` functions and their original functions to have the same
access level. Public original functions may also have internal
`@usableFromInline` derivatives, as a special case.

Diagnose access level mismatches. Produce a fix-it to change the derivative
function's access level.

This simplifies derivative registration rules, and may enable simplification of
AutoDiff symbol linkage rules.

Resolves TF-1099 and TF-1160.
rxwei pushed a commit to rxwei/swift that referenced this pull request Jun 3, 2020
…s. (apple#31527)

Require `@derivative` functions and their original functions to have the same
access level. Public original functions may also have internal
`@usableFromInline` derivatives, as a special case.

Diagnose access level mismatches. Produce a fix-it to change the derivative
function's access level.

This simplifies derivative registration rules, and may enable simplification of
AutoDiff symbol linkage rules.

Resolves TF-1099 and TF-1160.
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.

None yet

3 participants