From c98335081e85cbcf8ed962f96cb126e859de651a Mon Sep 17 00:00:00 2001 From: Philip Turner <71743241+philipturner@users.noreply.github.com> Date: Tue, 1 Feb 2022 12:59:51 -0500 Subject: [PATCH 1/5] Attempt to fix a bug --- include/swift/AST/DiagnosticsSema.def | 2 ++ lib/Sema/TypeCheckAttr.cpp | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 0a02c82334627..88a3ac78913ba 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -3313,6 +3313,8 @@ NOTE(protocol_witness_missing_differentiable_attr_invalid_context,none, "to %3", (StringRef, DeclName, Type, Type)) // @derivative +ERROR(derivative_attr_unknown_error,none, + "encountered unknown error while parsing @derivative(of:) attribute", ()) ERROR(derivative_attr_expected_result_tuple,none, "'@derivative(of:)' attribute requires function to return a two-element " "tuple; first element must have label 'value:' and second element must " diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp index fa5043b48d06b..7c125a0e95600 100644 --- a/lib/Sema/TypeCheckAttr.cpp +++ b/lib/Sema/TypeCheckAttr.cpp @@ -4749,11 +4749,24 @@ static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D, // to be enabled. if (checkIfDifferentiableProgrammingEnabled(Ctx, attr, D->getDeclContext())) return true; + auto *derivative = cast(D); auto originalName = attr->getOriginalFunctionName(); auto *derivativeInterfaceType = - derivative->getInterfaceType()->castTo(); + derivative->getInterfaceType()->getAs(); + if (!derivativeInterfaceType) { + // Crashes if the instance is not an ErrorType. + if (derivative->getInterfaceType()->is()) { + diags.diagnose(attr->getLocation(), + diag::derivative_attr_unknown_error); + return true; + } else { + // This should never happen, but leaving a runtime diagnostic incase it + // does because little is known about this failure. + llvm::report_fatal_error("Unknown failure in typeCheckDerivativeAttr."); + } + } // Perform preliminary `@derivative` declaration checks. // The result type should be a two-element tuple. From 9f15a9551d71b23df9bd7adeb500b4b499ad2a3a Mon Sep 17 00:00:00 2001 From: Philip Turner <71743241+philipturner@users.noreply.github.com> Date: Tue, 1 Feb 2022 15:40:54 -0500 Subject: [PATCH 2/5] Fix some stuff --- include/swift/AST/DiagnosticsSema.def | 2 -- lib/Sema/TypeCheckAttr.cpp | 19 +++++-------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 88a3ac78913ba..0a02c82334627 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -3313,8 +3313,6 @@ NOTE(protocol_witness_missing_differentiable_attr_invalid_context,none, "to %3", (StringRef, DeclName, Type, Type)) // @derivative -ERROR(derivative_attr_unknown_error,none, - "encountered unknown error while parsing @derivative(of:) attribute", ()) ERROR(derivative_attr_expected_result_tuple,none, "'@derivative(of:)' attribute requires function to return a two-element " "tuple; first element must have label 'value:' and second element must " diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp index 7c125a0e95600..c3dd5a8fa8718 100644 --- a/lib/Sema/TypeCheckAttr.cpp +++ b/lib/Sema/TypeCheckAttr.cpp @@ -4742,6 +4742,10 @@ void AttributeChecker::visitDifferentiableAttr(DifferentiableAttr *attr) { /// \returns true on error, false on success. static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D, DerivativeAttr *attr) { + if (D->isInvalid()) { + return true; + } + // Note: Implementation must be idempotent because it may be called multiple // times for the same attribute. auto &diags = Ctx.Diags; @@ -4749,24 +4753,11 @@ static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D, // to be enabled. if (checkIfDifferentiableProgrammingEnabled(Ctx, attr, D->getDeclContext())) return true; - auto *derivative = cast(D); auto originalName = attr->getOriginalFunctionName(); auto *derivativeInterfaceType = - derivative->getInterfaceType()->getAs(); - if (!derivativeInterfaceType) { - // Crashes if the instance is not an ErrorType. - if (derivative->getInterfaceType()->is()) { - diags.diagnose(attr->getLocation(), - diag::derivative_attr_unknown_error); - return true; - } else { - // This should never happen, but leaving a runtime diagnostic incase it - // does because little is known about this failure. - llvm::report_fatal_error("Unknown failure in typeCheckDerivativeAttr."); - } - } + derivative->getInterfaceType()->castTo(); // Perform preliminary `@derivative` declaration checks. // The result type should be a two-element tuple. From 3a1b282439d4e1707f7ae6a8c7a1c84c9ee02f0e Mon Sep 17 00:00:00 2001 From: Philip Turner <71743241+philipturner@users.noreply.github.com> Date: Tue, 1 Feb 2022 16:00:56 -0500 Subject: [PATCH 3/5] Relocate early return --- lib/Sema/TypeCheckAttr.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp index c3dd5a8fa8718..830291139617f 100644 --- a/lib/Sema/TypeCheckAttr.cpp +++ b/lib/Sema/TypeCheckAttr.cpp @@ -4742,10 +4742,6 @@ void AttributeChecker::visitDifferentiableAttr(DifferentiableAttr *attr) { /// \returns true on error, false on success. static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D, DerivativeAttr *attr) { - if (D->isInvalid()) { - return true; - } - // Note: Implementation must be idempotent because it may be called multiple // times for the same attribute. auto &diags = Ctx.Diags; @@ -4753,6 +4749,8 @@ static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D, // to be enabled. if (checkIfDifferentiableProgrammingEnabled(Ctx, attr, D->getDeclContext())) return true; + if (D->isInvalid()) + return true; auto *derivative = cast(D); auto originalName = attr->getOriginalFunctionName(); From 86524487ff40d8b7a6095243d94db3ee71febb4e Mon Sep 17 00:00:00 2001 From: Philip Turner <71743241+philipturner@users.noreply.github.com> Date: Tue, 1 Feb 2022 20:24:12 -0500 Subject: [PATCH 4/5] Fix bug and generics tests --- lib/Sema/TypeCheckAttr.cpp | 2 -- lib/Sema/TypeCheckGeneric.cpp | 1 - test/Constraints/overload.swift | 6 +++--- test/Generics/unbound.swift | 4 +++- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp index 830291139617f..fa5043b48d06b 100644 --- a/lib/Sema/TypeCheckAttr.cpp +++ b/lib/Sema/TypeCheckAttr.cpp @@ -4749,8 +4749,6 @@ static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D, // to be enabled. if (checkIfDifferentiableProgrammingEnabled(Ctx, attr, D->getDeclContext())) return true; - if (D->isInvalid()) - return true; auto *derivative = cast(D); auto originalName = attr->getOriginalFunctionName(); diff --git a/lib/Sema/TypeCheckGeneric.cpp b/lib/Sema/TypeCheckGeneric.cpp index 0e860c8ec4a57..c96d94d1702f8 100644 --- a/lib/Sema/TypeCheckGeneric.cpp +++ b/lib/Sema/TypeCheckGeneric.cpp @@ -506,7 +506,6 @@ void TypeChecker::checkReferencedGenericParams(GenericContext *dc) { // Produce an error that this generic parameter cannot be bound. paramDecl->diagnose(diag::unreferenced_generic_parameter, paramDecl->getNameStr()); - decl->setInvalid(); } } } diff --git a/test/Constraints/overload.swift b/test/Constraints/overload.swift index 34da8f7e4bda2..85ab17e5eb6e1 100644 --- a/test/Constraints/overload.swift +++ b/test/Constraints/overload.swift @@ -187,12 +187,12 @@ R_28051973().f(r28051973) // expected-error {{cannot use mutating member on immu // Fix for CSDiag vs CSSolver disagreement on what constitutes a // valid overload. -func overloadedMethod(n: Int) {} // expected-note {{'overloadedMethod(n:)' declared here}} -func overloadedMethod() {} +func overloadedMethod(n: Int) {} +func overloadedMethod() {} // expected-note {{in call to function 'overloadedMethod()'}} // expected-error@-1 {{generic parameter 'T' is not used in function signature}} overloadedMethod() -// expected-error@-1 {{missing argument for parameter 'n' in call}} +// expected-error@-1 {{generic parameter 'T' could not be inferred}} // Ensure we select the overload of '??' returning T? rather than T. func SR3817(_ d: [String : Any], _ s: String, _ t: String) -> Any { diff --git a/test/Generics/unbound.swift b/test/Generics/unbound.swift index 88e30dc03954e..a1b23cab56515 100644 --- a/test/Generics/unbound.swift +++ b/test/Generics/unbound.swift @@ -51,8 +51,10 @@ extension GC { class SomeClassWithInvalidMethod { - func method() { // expected-error {{generic parameter 'T' is not used in function signature}} + func method() { // expected-note {{in call to function 'method()'}} + // expected-error@-1 {{generic parameter 'T' is not used in function signature}} self.method() + // expected-error@-1 {{generic parameter 'T' could not be inferred}} } } From 26c9dee4a64111524931c5bad144c5f5df150161 Mon Sep 17 00:00:00 2001 From: Philip Turner <71743241+philipturner@users.noreply.github.com> Date: Tue, 1 Feb 2022 20:48:59 -0500 Subject: [PATCH 5/5] Add AutoDiff regression test --- test/AutoDiff/Sema/derivative_attr_type_checking.swift | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/AutoDiff/Sema/derivative_attr_type_checking.swift b/test/AutoDiff/Sema/derivative_attr_type_checking.swift index 69edbf96a67b3..8e99b4bdab4e0 100644 --- a/test/AutoDiff/Sema/derivative_attr_type_checking.swift +++ b/test/AutoDiff/Sema/derivative_attr_type_checking.swift @@ -79,6 +79,16 @@ func vjpOriginalFunctionNotFound(_ x: Float) -> (value: Float, pullback: (Float) fatalError() } +struct Q { } + +@derivative(of: remainder(_:_:)) // expected-error {{cannot find 'remainder' in scope}} +// expected-error @+1 {{generic parameter 'T' is not used in function signature}} +func _vjpRemainder(_ x: Q, _ y: Q) -> ( + value: Q, pullback: (Q) -> (Q, Q) +) { + fatalError() +} + // Test `@derivative` attribute where `value:` result does not conform to `Differentiable`. // Invalid original function should be diagnosed first. // expected-error @+1 {{cannot find 'nonexistentFunction' in scope}}