Skip to content

Add -Vcyclic to improve reporting of "cyclic reference" errors #10680

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

Merged
merged 1 commit into from
May 22, 2024

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Feb 3, 2024

-Vcyclic improves reporting of "cyclic reference" errors.

It enables "tracing" of symbol locking to show which symbols were involved in the cycle.

This is also helpful when a synthetic symbol name is reported, since that name is not meaningful.

Look for a non-synthetic symbol to blame.

Fixes scala/bug#7808

@scala-jenkins scala-jenkins added this to the 2.13.14 milestone Feb 3, 2024
@som-snytt som-snytt force-pushed the issue/7808-synth-message branch from c04850d to f8708b2 Compare February 3, 2024 16:52
@som-snytt
Copy link
Contributor Author

Dotty also has message about symbols involved in the cycle IIRC.

This just uses Stack for POC. Dotty has an ArrayBuffer for optionally tracing in the context.

IIRC here the lock/unlock are not symmetric?

@som-snytt som-snytt marked this pull request as ready for review February 16, 2024 16:36
@lrytz
Copy link
Member

lrytz commented Mar 13, 2024

I'm concerned about performance (completeInfo is core) and about leaking symbols.

How about a single previousLocked cache? Hopefully the next hop after a synthetic symbol would be organic.

@SethTisue SethTisue modified the milestones: 2.13.14, 2.13.15 Mar 13, 2024
@som-snytt
Copy link
Contributor Author

som-snytt commented Mar 13, 2024

it ought to be behind a diagnostic flag. -Vcyclic-ref.

scala/scala3#19413 (comment)

is where I learned about

Run with both -explain-cyclic and -Ydebug-cyclic to see full stack trace.

@som-snytt som-snytt changed the title Use CyclicRef trace to pick sym in message Use CyclicRef trace to pick sym in message [ci: last-only] Mar 16, 2024
@som-snytt som-snytt force-pushed the issue/7808-synth-message branch 2 times, most recently from c449be6 to 4a80252 Compare March 17, 2024 03:43

val badsym = if (!sym.isSynthetic) sym else {
val organics = trace.filter(!_.isSynthetic)
if (organics.length == 0) sym
Copy link
Member

Choose a reason for hiding this comment

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

How about adding something to the error message here? $sym is synthetic; use -Vcyclic to find which definition needs an explicit type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apparently, I followed that suggestion.

@som-snytt som-snytt force-pushed the issue/7808-synth-message branch 2 times, most recently from 6781f7a to b8901c0 Compare April 9, 2024 16:05
@som-snytt som-snytt changed the title Use CyclicRef trace to pick sym in message [ci: last-only] Use CyclicRef trace to pick sym in message! [ci: last-only] May 18, 2024
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

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

LGTM! Let's squash before merge.

@som-snytt som-snytt force-pushed the issue/7808-synth-message branch from b8901c0 to ecdaecc Compare May 21, 2024 14:25
@som-snytt som-snytt changed the title Use CyclicRef trace to pick sym in message! [ci: last-only] Use CyclicRef trace to pick sym in message May 21, 2024
@som-snytt som-snytt requested a review from lrytz May 21, 2024 15:18
@lrytz lrytz merged commit b92014d into scala:2.13.x May 22, 2024
3 checks passed
@som-snytt som-snytt deleted the issue/7808-synth-message branch May 22, 2024 07:47
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Aug 5, 2024
@SethTisue SethTisue changed the title Use CyclicRef trace to pick sym in message Add -Vcyclic to improve reporting of "cyclic reference" errors Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird error message with compiler-generated variable names
4 participants