[AutoDiff] Check @noDerivative for function type comparison #67121
Conversation
As described in the issue swiftlang#62922, the compiler should not allow to discard @noDerivative attribute and keep @differentiable. The patch adds a diagnostic for this case. differentiation_diagnostics.swift LIT test already had this pattern, but I was unable to add an expected-error for it, because the compiler exits early and does not emit other diagnostics in this file.
|
@swift-ci please test |
| @@ -741,7 +741,7 @@ struct TF_675 : Differentiable { | |||
| let _: @differentiable(reverse) (Float) -> Float = TF_675().method | |||
|
|
|||
| // TF-918: Test parameter subset thunk + partially-applied original function. | |||
There was a problem hiding this comment.
I can no longer find TF-918... Is there a GitHub issue for it?
There was a problem hiding this comment.
I'm afraid all TF issues gone long time ago. And if they were not converted to normal Swift JIRA issues, then they cannot be found anywhere...
@shahmishal will you please confirm?
There was a problem hiding this comment.
We should probably go over all TF- bugs in comments and remove them.
There was a problem hiding this comment.
And rewrite to present issues when the issue was migrated (there are few of them IIRC). Some of the TF's refer to some design decisions made in the past. It would be good to document them somehow while there are some memories on them :) I'll check what are the most important / interesting ones
| @@ -741,7 +741,7 @@ struct TF_675 : Differentiable { | |||
| let _: @differentiable(reverse) (Float) -> Float = TF_675().method | |||
|
|
|||
| // TF-918: Test parameter subset thunk + partially-applied original function. | |||
| let _: @differentiable(reverse) (Float, Float) -> Float = (+) as @differentiable(reverse) (Float, @noDerivative Float) -> Float | |||
| let _: @differentiable(reverse) (Float, @noDerivative Float) -> Float = (+) as @differentiable(reverse) (Float, @noDerivative Float) -> Float | |||
There was a problem hiding this comment.
I'm surprised this test was there! Maybe it was meant to test function conversions to a function with more @noDerivative. That should be possible with "subset parameter thunks".
let _: @differentiable(reverse) (Float, @noDerivative Float) -> Float = (+) as @differentiable(reverse) (Float, Float) -> FloatIs the above possible today?
There was a problem hiding this comment.
Yes, the above example is also possible. I think it is the same as test1 in the new LIT test, but we can add it to differentiation_diagnostics.swift as well.
There was a problem hiding this comment.
Does it actually work in a runtime test?
|
@swift-ci please test |
| @@ -0,0 +1,25 @@ | |||
| // RUN: %target-typecheck-verify-swift | |||
|
|
|||
| import _Differentiation | |||
There was a problem hiding this comment.
Because the test is not in the test/AutoDiff folder, it needs REQUIRES: differentiable_programming or people that build without that feature will fail this test.
There was a problem hiding this comment.
@asavonic Please submit a follow-up PR
There was a problem hiding this comment.
I'm sorry for causing this issue. I've submitted a PR to fix it: #67190
As described in the issue #62922, the compiler should not allow to discard
@noDerivativeattribute and keep@differentiable. The patch adds a diagnostic for this case.differentiation_diagnostics.swiftLIT test already had this pattern, but I was unable to add anexpected-errorfor it, because the compiler exits early and does not emit other diagnostics in this file.Resolves #62922.