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.

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

Conversation

marcrasi
Copy link

@marcrasi marcrasi commented Jan 6, 2020

Forbids @derivatives of protocol requirements, because they do not do anything until TF-982 is implemented.

This is the master branch version of #28890.

@marcrasi marcrasi requested a review from dan-zheng January 6, 2020 23:34
@@ -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.

@marcrasi marcrasi force-pushed the forbid-protocol-req branch from 7561235 to d69e892 Compare January 6, 2020 23:58
@marcrasi
Copy link
Author

marcrasi commented Jan 7, 2020

@swift-ci please test and merge

@dan-zheng
Copy link
Contributor

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Jan 7, 2020

Build failed
Swift Test OS X Platform
Git Sha - d69e892

@swift-ci
Copy link
Contributor

swift-ci commented Jan 7, 2020

Build failed
Swift Test Linux Platform
Git Sha - d69e892

@swift-ci swift-ci merged commit b6e0ffd into swiftlang:master Jan 7, 2020
@marcrasi marcrasi deleted the forbid-protocol-req branch January 7, 2020 19:14
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