Skip to content

Improve request evaluator cycle diagnostics #36375

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Mar 9, 2021

This PR:

  • Refactors DiagnosticEngine's declaration-pretty-printing facility to allow it to be used from extractNearestSourceLoc(), improving cycle diagnostics that were previously emitted at <invalid>:0.
  • Modifies the -debug-cycles output to indicate which request in the dumped stack was repeated.

I haven't figured out how to test these changes yet, which is why this is still currently a WIP.

@beccadax beccadax force-pushed the the-cycle-begins-anew branch from 9771a7c to 1ef5414 Compare March 10, 2021 00:51
@beccadax
Copy link
Contributor Author

@swift-ci please smoke test

@beccadax beccadax marked this pull request as ready for review March 10, 2021 00:53
@beccadax beccadax requested a review from CodaFi March 10, 2021 00:54
@slavapestov
Copy link
Contributor

Nice!

@AnthonyLatsis
Copy link
Collaborator

@beccadax There's a PR with little traction that restores the way -debug-cycles output used to look like, in case you'd like to consider incorporating those changes.

@beccadax beccadax force-pushed the the-cycle-begins-anew branch from 637e534 to 0e13dba Compare March 11, 2021 00:28
@beccadax beccadax changed the title [WIP] Improve request evaluator cycle diagnostics Improve request evaluator cycle diagnostics Mar 11, 2021
@beccadax
Copy link
Contributor Author

@swift-ci please test

@beccadax
Copy link
Contributor Author

The previous test case was very fragile and is, in itself, a bug we probably ought to fix. I've filed that bug as SR-14324 and added a command-line argument to the frontend which artificially creates a request cycle involving arbitrary types that we can diagnose.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0e13dba14885d604f830a322d4129bd5f3b6a29a

@beccadax
Copy link
Contributor Author

@swift-ci please test Linux platform

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 0e13dba14885d604f830a322d4129bd5f3b6a29a

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 0e13dba14885d604f830a322d4129bd5f3b6a29a

@beccadax
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@beccadax
Copy link
Contributor Author

@swift-ci please smoke test

This commit makes the pretty-printing behavior of `DiagnosticEngine` easily accessible via a new `Decl::getLocByPrintingIfNeeded()` call and uses that call in various places related to `extractNearestSourceLoc()`, ultimately improving diagnostics for cycles involving serialized declarations.

Note that the test case added here should probably not cause a circularity error; SR-14324 tracks this bug.
This commit implements a facility to artificially cause a request cycle involving various types so we can test how circularity errors print without having to carefully craft test cases where they happen.
@beccadax beccadax force-pushed the the-cycle-begins-anew branch from 0e13dba to b96a107 Compare March 26, 2021 22:11
@beccadax
Copy link
Contributor Author

@swift-ci please smoke test

2 similar comments
@beccadax
Copy link
Contributor Author

beccadax commented Apr 6, 2021

@swift-ci please smoke test

@beccadax
Copy link
Contributor Author

@swift-ci please smoke test

@beccadax
Copy link
Contributor Author

@swift-ci please smoke test macOS platform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants