[AutoDiff][SR-13152] Better diagnostic for `static` decl modifier mismatch. #36128

vguerra · 2021-02-24T11:12:44Z

Improved diagnostic for when the registered derivative function and
the function it derivates (the original) differ in terms of
static declaration modifier usage. Suggesting as well a fix-it,
to either remove or add the static keyword.

The registered derivative needs to be marked as static in two cases:

When the original function is a constructor.
When the original function is static as well.

When the original function is an instance method, the registered derivate
must be as well.

Given the following example:

struct Foo: Differentiable {
    static func sf1(_ x : Float) -> Float {
        return x * x * x
    }
}

extension Foo {
    @derivative(of: sf1)
    func vjpF1(_ x: Float) -> (value: Float, pullback: (Float.TangentVector) -> (Foo.TangentVector, Float.TangentVector)) {
        fatalError()
    }
}

Before this PR:

sr_13152.swift:25:21: error: referenced declaration 'sf1' could not be resolved
    @derivative(of: sf1)
                    ^
sr_13152.swift:19:17: note: candidate static method does not have expected type '(Foo) -> (Float) -> Float'
    static func sf1(_ x : Float) -> Float {

After this PR:

sr_13152.swift:13:21: error: unexpected derivative function declaration; 'sf1' requires the derivative function 'vjpF1' to be a 'static' method
    @derivative(of: sf1)
                    ^
sr_13152.swift:7:17: note: original function 'sf1' is a 'static' method
    static func sf1(_ x : Float) -> Float {
                ^
sr_13152.swift:14:5: note: make derivative function 'vjpF1' a 'static' method
    func vjpF1(_ x: Float) -> (value: Float, pullback: (Float.TangentVector) -> (Foo.TangentVector, Float.TangentVector)) {
    ^
    static

Resolves SR-13152.

@Derivative

…match. Improved diagnostic for when the registered derivative function and the function it derivates (the original) differ in terms of `static` declaration modifier usage. Suggesting as well a fix-it, to either remove or add the `static` keyword. The registered derivative needs to be marked as `static` in two cases: 1. When the original function is a constructor. 2. When the original function is static as well. When the original function is an instance method, the registered derivate must be as well. Given the following example: ```swift struct Foo: Differentiable { static func sf1(_ x : Float) -> Float { return x * x * x } } extension Foo { @Derivative(of: sf1) func vjpF1(_ x: Float) -> (value: Float, pullback: (Float.TangentVector) -> (Foo.TangentVector, Float.TangentVector)) { fatalError() } } ``` Before this PR: ```bash sr_13152.swift:25:21: error: referenced declaration 'sf1' could not be resolved @Derivative(of: sf1) ^ sr_13152.swift:19:17: note: candidate static method does not have expected type '(Foo) -> (Float) -> Float' static func sf1(_ x : Float) -> Float { ``` After this PR: ```bash sr_13152.swift:26:10: error: function must match usege of static declaration modifier from original function 'vjpF1' func vjpF1(_ x: Float) -> (value: Float, pullback: (Float.TangentVector) -> (Foo.TangentVector, Float.TangentVector)) { ^ sr_13152.swift:26:5: note: mark the function 'vjpF1' as static func vjpF1(_ x: Float) -> (value: Float, pullback: (Float.TangentVector) -> (Foo.TangentVector, Float.TangentVector)) { ^ static ``` Resolves [SR-13152](https://bugs.swift.org/browse/SR-13152).

vguerra · 2021-02-24T11:16:39Z

lib/Sema/TypeCheckAttr.cpp

@@ -4007,7 +4007,9 @@ static bool checkFunctionSignature(
  if (!std::equal(required->getParams().begin(), required->getParams().end(),
                  candidateFnTy->getParams().begin(),
                  [&](AnyFunctionType::Param x, AnyFunctionType::Param y) {
-                    return x.getOldType()->isEqual(mapType(y.getOldType()));
+                    auto xInstanceTy = x.getOldType()->getMetatypeInstanceType();
+                    auto yInstanceTy = y.getOldType()->getMetatypeInstanceType();


when either required or candidateFnTy are defined as static, the parameters are MetaTypes, hence I try to look into the instance type to be able to make this comparison work. Otherwise, this will fail and trigger the diagnostic we see today. Do you see any issues with this here?

I recommend checking for static-ness mismatches in differentiability witnesses before checking for signature mismatches. That'll let you diagnose type mismatches as a separate thing.

The signature mismatches are done during the process of looking up for the original function that the derivative is trying to derivate, that's why I modified this check so that the lookup does discards the candidate due to static-ness mismatch. Once the original candidate is found, I can do the static mismatch checks I introduced in this PR.

vguerra · 2021-02-24T11:17:15Z

lib/Sema/TypeCheckAttr.cpp

@@ -4581,6 +4583,42 @@ void AttributeChecker::visitDifferentiableAttr(DifferentiableAttr *attr) {
  (void)attr->getParameterIndices();
 }

+/// Checks if original candidate and registered derivative match in terms


Do you think there is a better place to move this utility functions to?

The treatment of constructors as static functions for the purpose of differentiability witness matching is pretty unique. I think this is where it needs to be.

ok, great! thanks.

CodaFi · 2021-02-25T00:13:05Z

include/swift/AST/DiagnosticsSema.def

@@ -3264,6 +3264,14 @@ ERROR(autodiff_attr_result_not_differentiable,none,
      "'Differentiable', but %0 does not conform to 'Differentiable'", (Type))
 ERROR(autodiff_attr_opaque_result_type_unsupported,none,
      "cannot differentiate functions returning opaque result types", ())
+ERROR(autodiff_attr_static_decl_mismatch,none,
+      "function must match usage of static declaration modifier "


How about

Suggested change

"function must match usage of static declaration modifier "

"derivative function %0 operates on %select{an instance|a type}1, not %select{an instance|a type}2 as required"

You can point back at the original function with a note.

Thank you for the suggestion. I changed the working of the error diagnostic and adapted the subsequent notes accordingly in cfd7932

CodaFi · 2021-02-25T00:32:31Z

lib/Sema/TypeCheckAttr.cpp

+/// Produces diagnostics for mismatch in static/instance method declaration
+/// between original candidate and registered derivative.
+static void diagnoseStaticDeclMismatch(AbstractFunctionDecl *originalCandidate,
+ FuncDecl *registred) {


Nit: Please run git-clang-format on this patch.

I made sure the code is clang-formatted. Fixed in cfd7932.

of commit: 14d6bf2

- git-clang-format on this PR. - rewording diagnostic messages.

rxwei · 2021-02-27T03:40:31Z

include/swift/AST/DiagnosticsSema.def

+      (/*derivative*/ DeclName, /*derivativeOperatesOnTy*/bool,
+       /*originalOperatesOnTy*/bool))
+NOTE(autodiff_attr_static_decl_original,none,
+      "original function %0 operates on %select{an instance|a}1 type",


I'm not sure "operates on a type" is the right user-facing error message, as it sounds quite uncommon in existing Swift diagnostics. Plus, "an instance type" doesn't sound quite right — shouldn't it just be "an instance"?

Instead of "operates on something", how about something like "original function 'foo(_:)' is {an instance|a static} method"?

Indeed, your suggestion seems more aligned with wording already existing in the diagnostic error messages.
I'll make this note disappear and include your suggestion in the error diagnostic itself. Thanks!

rxwei · 2021-02-27T03:46:13Z

lib/Sema/TypeCheckAttr.cpp

+static bool checkStaticDeclMismatch(AbstractFunctionDecl *originalCandidate,
+                                    AbstractFunctionDecl *registered) {
+  return (isa<ConstructorDecl>(originalCandidate) ||
+          originalCandidate->isStatic()) != registered->isStatic();
+}
+
+/// Produces diagnostics for mismatch in static/instance method declaration
+/// between original candidate and registered derivative.
+static void diagnoseStaticDeclMismatch(AbstractFunctionDecl *originalCandidate,


TypeCheckerAttr.cpp is not an AD-specific file, so if we are to define these as static functions we should make sure these two AD-specific helper functions have AD-specific names. That said, for better locality and readability, I'd recommend defining these as lambdas in typeCheckDerivativeAttr right above the call sites.

I started defining these helper functions as you suggested as lambdas within typeCheckDerivativeAttr but the diagnostics apply as well for the @transpose attribute. Type-checking for @transpose attribute happens in visitTransposeAttr hence the definitions here so that we can reuse them in both places. I could define twice the a lambda that performs the checks as it is not much code. wdyt?

I’m planning on revamping @transpose typing rules so the diagnostics may not apply to both anymore. So duplicating the core for now sounds good.

I have pushed a new commit that duplicates the check and diagnostics for both attribute type check routines. As well, I have split the diagnostic messages for both attributes given that in one case one refers to the derivative function and to the transpose function in the other, plus it would be easier to remove them for the '@transpose' attribute if they need to go as a result of your refactoring.

rxwei · 2021-02-27T04:05:08Z

test/AutoDiff/Sema/derivative_attr_type_checking.swift

+  // expected-note @+1 {{original function 'staticMethod' operates on a type}}
+  static func staticMethod(_ x: T) -> T { x }
+
+  @derivative(of: init)


In these diagnostics we should make the error appear at the location that triggered the error. For example, in this case vjpInit(_:) is a perfectly fine Swift function if it were compiled directly. The compilation error is caused by the addition of the @derivative(of: init) attribute, so the error should appear on the attribute, not on vjpInit(_:).

@derivative(of: init) ^~~~

The error should also say "as required by what" instead of just "as required" in order to provide the user with the sufficient context. In this case, maybe it can say something like:

error: unexpected derivative function declaration; 'StaticMismatch.init(_:)' requires that the derivative function be 'static'

yes, you are right. It seems a bit weird to have a note produced "far away" from the error itself. I'll address your comment in a soon-to-come commit :)

I have modified the wording of the error messages and notes to be more aligned to what you suggested. The error diagnostic is now pointing at the method stated in the 'of:' parameter.

@derivative(of: init) ^~~~

I overlooked your suggestion here of highlighting the original function (sorry for that :/ ). I've seen other diagnostics in this context do highlighting as well so I just pushed #36259 for the sake of consistency.

rxwei · 2021-02-27T04:09:53Z

include/swift/AST/DiagnosticsSema.def

@@ -3264,6 +3264,18 @@ ERROR(autodiff_attr_result_not_differentiable,none,
      "'Differentiable', but %0 does not conform to 'Differentiable'", (Type))
 ERROR(autodiff_attr_opaque_result_type_unsupported,none,
      "cannot differentiate functions returning opaque result types", ())
+ERROR(autodiff_attr_static_decl_mismatch,none,


Aren't these specific to the @derivative attribute? Should they be moved near derivative_attr_* diagnostics (around line 3171)?

As I mentioned before the same diagnostic can apply to the @transpose attr. hence the definition in this section as the wording is not specific to @derivative nor @transpose attributes. Otherwise I would have to define them twice.

Sounds good. The diagnostic name can use some clarification, something along the lines of autodiff_attr_instance_method_mismatching_original, basically spelling out the exact mismatch condition as succinctly as possible.

* splitting diagnostic messages for `@derivative` and `@transpose` attr. * duplicating the diagnostic logic using lambda functions within the respective attr checking methods. * change wording of error/notes to use non-`static`|`static`.

rxwei · 2021-03-03T02:06:28Z

@swift-ci please test

swift-ci · 2021-03-03T03:29:52Z

Build failed
Swift Test Linux Platform
Git Sha - 38433cf

rxwei · 2021-03-03T04:24:02Z

@swift-ci please test Linux

rxwei · 2021-03-03T09:54:11Z

Thanks Victor!

Small follow up of [PR-36128](swiftlang#36128) to be consistent with other diagnostics emitted when type-checking `@derivative` and `@transpose` attributes.

Small follow up of [PR-36128](#36128) to be consistent with other diagnostics emitted when type-checking `@derivative` and `@transpose` attributes.

vguerra commented Feb 24, 2021

View reviewed changes

dan-zheng requested review from rxwei and dan-zheng February 24, 2021 11:27

vguerra added 2 commits February 24, 2021 13:52

fix typo in diagnostics msg.

9932c88

Fix reference to origianl function name in error message.

bbade0b

CodaFi reviewed Feb 25, 2021

View reviewed changes

vguerra added 3 commits February 25, 2021 13:52

clang-formating

110726d

of commit: 14d6bf2

Addressing feedback on PR.

cfd7932

- git-clang-format on this PR. - rewording diagnostic messages.

fixing comments for diagnostic parameters

df75fa4

vguerra requested a review from CodaFi February 26, 2021 22:35

vguerra marked this pull request as ready for review February 26, 2021 22:35

rxwei reviewed Feb 27, 2021

View reviewed changes

vguerra added 2 commits March 2, 2021 23:28

addressing feedback on PR

56f09db

* splitting diagnostic messages for `@derivative` and `@transpose` attr. * duplicating the diagnostic logic using lambda functions within the respective attr checking methods. * change wording of error/notes to use non-`static`|`static`.

remove unneeded includes statements

38433cf

vguerra requested a review from rxwei March 2, 2021 23:04

rxwei approved these changes Mar 2, 2021

View reviewed changes

rxwei merged commit f81aba7 into swiftlang:main Mar 3, 2021

vguerra mentioned this pull request Mar 3, 2021

[AutoDiff] NFC: Highlights original function name in diagnostic #36259

Merged

vguerra mentioned this pull request Feb 24, 2021

[SR-13152] Add diagnostic for derivative registration instance/static method mismatch #54274

Closed

	"function must match usage of static declaration modifier "
	"derivative function %0 operates on %select{an instance\|a type}1, not %select{an instance\|a type}2 as required"

[AutoDiff][SR-13152] Better diagnostic for static decl modifier mismatch. #36128

[AutoDiff][SR-13152] Better diagnostic for static decl modifier mismatch. #36128

Uh oh!

Conversation

vguerra commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei Feb 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxwei commented Mar 3, 2021

Uh oh!

swift-ci commented Mar 3, 2021

Uh oh!

rxwei commented Mar 3, 2021

Uh oh!

rxwei commented Mar 3, 2021

Uh oh!

Uh oh!

[AutoDiff][SR-13152] Better diagnostic for `static` decl modifier mismatch. #36128

[AutoDiff][SR-13152] Better diagnostic for `static` decl modifier mismatch. #36128

vguerra commented Feb 24, 2021 •

edited

Loading

CodaFi Feb 25, 2021 •

edited

Loading

rxwei Feb 27, 2021 •

edited

Loading

rxwei Mar 2, 2021 •

edited

Loading

rxwei Mar 2, 2021 •

edited

Loading