-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diagnostic wasn't missing, it's a compiler bug. We should try to fix the bug instead.
Should a function be passing in an error in the first place? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test as well?
lib/Sema/TypeCheckAttr.cpp
Outdated
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."); | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I edited my response.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I don't have much experience with detecting diagnostics in tests, but I could copy and paste some source code for you to refine. |
You can add a test to |
How to I incrementally build the compiler without rebuilding the standard library? I tried what it says on the README (only swift-frontend) but it doesn't actually change anything. For example, I change the Swift version text, incrementally build, and it doesn't register a change. @AnthonyLatsis this is why it's taking me several minutes to answer your question. |
You use |
I've been running this command and it still keeps rebuilding Standard Library modules like SwiftOnoneSupport and _MatchingEngine:
|
I did this. It compiled in seconds, then acted like nothing happened. For example, I removed a fatal error, yet the now-removed fatal error still fired.
I can sort of get around it by pressing ^C when it says it's in the middle of building something I know is stdlib. |
What exactly are you doing to run the compiler? |
|
If it's |
And passing in "swift" to ninja gives an error. |
|
It says it's compiling, and I can tell because it shows errors if I take a semicolon off of something. The problem might be that my command starts with this:
I'm not using swift-frontend or swiftc. |
It should be fine, |
It actually isn't a direct alias.
|
I grabbed the hard-coded program arguments for swift-frontend (pasted below). I messed with the C++ code, but nothing changed:
|
@AnthonyLatsis could you try incrementally rebuilding the compiler yourself (after a modification that breaks it) and attaching the build log? |
I can reproduce the crash if I run |
This happens:
|
I'm not talking about reproducing the crash. I'm talking about this:
|
This is expected, my commands are enough to reproduce the crash and fix it, but you need to specify an SDK to properly compile AutoDiff code.
What are the edits to the C++ code, and what is the unexpected output? |
Can you try running the following command and see it the fatal error appears?
|
I ran |
About the new bug I'm facing, I've narrowed it down further. The problem is in the AST stage. When I set the closure's parameter type to It seems that it's skipping over something in /lib/Sema/TypeChecker.cpp, line 3027. It should make a call to It should be throwing a diagnostic error of type |
I'll have a look. Out of curiosity, could you try placing the |
I'm currently figuring out what's happening at the place it's supposed to throw an error diagnostic. I found a very handy trick that makes debugging so much easier (and doesn't corrupt stdlib builds):
For example, I found out that it reaches line 580 in both cases (with |
You could file a bug so that we can move the discussion to JIRA. |
lib/Sema/TypeCheckAttr.cpp
Outdated
@@ -4749,6 +4749,8 @@ static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D, | |||
// to be enabled. | |||
if (checkIfDifferentiableProgrammingEnabled(Ctx, attr, D->getDeclContext())) | |||
return true; | |||
if (D->isInvalid()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the invalid bit and skipping work in Sema is certainly a way to make crashes go away. But we probably want to identify a root cause and fix that first. There's very few instances where this pattern actually helps. In practice it can hinder many more processes downstream as it either hides bugs, shifts data dependencies, or hides dependencies from the incremental build. We're trying to eliminate usages of this pattern where we can - unfortunately many of them are load bearing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The root cause is that we're calling setInvalid()
(or setting the interface type to ErrorType
) when a generic parameter is not used in the type signature of a function/subscript etc., although there seems to be nothing wrong with leaving the interface type as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the real question is: why was typeCheckDerivativeAttr
called when an error diagnostic was issued?
Is this correct?
Update: I made this comment while GitHub hadn't registered that the comment by @AnthonyLatsis had been made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The root cause is that we're calling setInvalid() when a generic parameter is not used in the type signature of a function/subscript etc., although there seems to be nothing wrong with leaving the interface type as-is.
Agreed. Start by eliminating that call and seeing what changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AnthonyLatsis I have my mind focused on the new bug I'm to file on JIRA. Would you be able to investigate that call and send over the necessary changes via a code review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start by eliminating that call and seeing what changes.
Pretty reassuring, running tests locally shows only 2 new generic parameter 'T' could not be inferred
errors that are now expected.
@philipturner if you are willing to see this through, you have to delete this call and update the following failing tests (and add the AutoDiff regression test):
Swift(macosx-x86_64) :: Constraints/overload.swift
Swift(macosx-x86_64) :: Generics/unbound.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Thanks for narrowing down the crux of the bug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and add the AutoDiff regression test):
That just means add a new test proving this bug was fixed, correct? If so, I'll need a bit of help setting that up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a test file for @derivative
checking: ../test/AutoDiff/Sema/derivative_attr_type_checking.swift
. Just add your test case to that and run lit
on it to see the diagnostics that are expected. If the expectations are correct, incorporate the missing diagnostics into your test using expected-*
directives, as shown elsewhere in that file. The lit command is
cd ../build/Ninja-RelWithDebInfoAssert/test-macosx-$(uname -m) && ../llvm-project/llvm/utils/lit/lit.py -sv --filter=derivative_attr_type_checking AutoDiff/Sema
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AnthonyLatsis done!
I got the correct diagnostic using @AnthonyLatsis's suggested patch! It was hidden away by the bug!
|
@CodaFi btw racy_async_let_fibonacci just failed while I was running tests using this command:
Update: it failed a second time too Now that I think about it, I think it did fail a few times during normal testing. |
Well, if it wasn't for @CodaFi, I probably would not mind the root cause, so thanks to Robert!
Should be unrelated. |
@swift-ci please smoke test macOS |
macOS smoke test failed. |
@swift-ci please clean smoke test macOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the latest changes, it is no longer a fix directly related to autodiff. I'd suggest removing "[AutoDiff] " and modifying the PR description to describe how this affects non-autodiff Swift diagnostics.
Is that sufficient? |
|
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better.
@swift-ci clean smoke test |
Could you please squash your changes into a single commit? |
We can squash and merge to avoid rerunning tests. |
Currently, a
Decl
is set to invalid at the end ofTypeChecker::checkReferencedGenericParams
. That should not happen, and finally manifested in a crash discovered while testing AutoDiff:As a result of this fix, error diagnostics for incorrect generic functions slightly change:
And, this affects a battle between CSDiag and CSSolver: