Skip to content

Don't Unnecessarily Invalidate Unreferenced Generic Parameters #41128

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.

Sign up for GitHub

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
CodaFi merged 5 commits into from
Feb 3, 2022
Merged

Don't Unnecessarily Invalidate Unreferenced Generic Parameters #41128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c983350
Attempt to fix a bug
philipturner Feb 1, 2022
9f15a95
Fix some stuff
philipturner Feb 1, 2022
3a1b282
Relocate early return
philipturner Feb 1, 2022
8652448
Fix bug and generics tests
philipturner Feb 2, 2022
26c9dee
Add AutoDiff regression test
philipturner Feb 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down
15 changes: 14 additions & 1 deletion lib/Sema/TypeCheckAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<FuncDecl>(D);
auto originalName = attr->getOriginalFunctionName();

auto *derivativeInterfaceType =
derivative->getInterfaceType()->castTo<AnyFunctionType>();
derivative->getInterfaceType()->getAs<AnyFunctionType>();
if (!derivativeInterfaceType) {
// Crashes if the instance is not an ErrorType.
if (derivative->getInterfaceType()->is<ErrorType>()) {
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.");
}
}
Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we encounter an ErrorType here, it means we already produced an appropriate diagnostic elsewhere (see first line in the logs). I think we should simply bail out if there's an error in the interface type. Also, we should not be expecting getInterfaceType() to return a null type.

Copy link
Contributor Author

@philipturner philipturner Feb 1, 2022

Choose a reason for hiding this comment

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

Calling Decl->isInvalid() returns false, so guarding against that does nothing for bailing out early. The only way to do so is to check that the pointer is null, then exit early. Since this behavior deserves to be caught, I'm issuing a second diagnostic.

Copy link
Contributor Author

@philipturner philipturner Feb 1, 2022

Choose a reason for hiding this comment

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

getInterfaceType() is not what's returning a null. It's getAs<...> that's returning the null. In fact, I call a function on the result of getInterfaceType() in that conditional block.

Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Calling Decl->isInvalid() returns false

Are you sure? Decl->isInvalid() on a FuncDecl is equivalent to getInterfaceType()->hasError(). It should have an error with the code in your example.

Copy link
Contributor Author

@philipturner philipturner Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So just check hasError(), silently bail if true, otherwise cast to AnyFunctionType as was previously done?

Copy link
Contributor Author

@philipturner philipturner Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I just said that Decl->isInvalid doesn't do anything useful. It always returns *false* here.

Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Feb 1, 2022

Choose a reason for hiding this comment

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

It always returns true here

That is exactly what you need. The declaration is invalid when its interface type contains an ErrorType.

Copy link
Contributor Author

@philipturner philipturner Feb 1, 2022

Choose a reason for hiding this comment

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

I edited my response.

Copy link
Collaborator

@AnthonyLatsis AnthonyLatsis Feb 1, 2022

Choose a reason for hiding this comment

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

Can you show me the exact code you're running?

Copy link
Contributor Author

@philipturner philipturner Feb 1, 2022

Choose a reason for hiding this comment

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

The behavior just changed. It now does return true. Maybe I misremembered my experience debugging it.


// Perform preliminary `@derivative` declaration checks.
// The result type should be a two-element tuple.
Expand Down