Skip to content
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

Eliminate LazyRefs before comparing seen types in collectCompanions #21522

Closed
wants to merge 1 commit into from

Conversation

wjoel
Copy link
Contributor

@wjoel wjoel commented Aug 30, 2024

Closes #21521

I don't have a minimized test case for this yet. Perhaps someone more knowledgeable can figure out a good way to construct types with LazyRefs.

Also, I'm not sure if this is the best fix, as something else seems to have caused this in 3.5.0 (not many changes in Implicits.scala recently, that I could see, that would have caused this issue).

Still, I think the logic of eliminating LazyRefs before comparing with previously seen types is sound, as that's what TypeComparer does.

With this change, the sample (correctly) fails to compile with the same error messages as on Scala 3.4.2:

 7 |  Box.builder()
 8 |    .setOrientation(Orientation.VERTICAL)
 9 |    .setHalign(Align.START)
10 |    .setValign(Align.BASELINE)
11 |    .se
   |  ^
   |value se is not a member of org.gnome.gtk.Box#Builder[? <: org.gnome.gtk.Box#Builder[?]]#B#B² - did you mean B.ne?
   |
   |where:    B  is a type in trait Builder² with bounds <: io.github.jwharm.javagi.gobject.Builder³[B]
   |          B² is a type in class Builder⁴ with bounds <: org.gnome.gtk.Widget#Builder⁴[B²]

Copy link
Contributor

@EugeneFlesselle EugeneFlesselle left a comment

Choose a reason for hiding this comment

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

The StackOverflowError catching logic of handleRecursive is already quite sensitive. And the fact that it could fail such as in the original issue is quite worrying.
The proposed fix removes the error by avoiding the problem in that case. But there are still situations where cyclic references cannot be detected by the seen HashSet, and which would hence still be susceptible to potentially uncaught exceptions here.

We also have to be careful about performance here. The TypeComparer avoids entering the monitored subtype checking logic until the Config.LogPendingSubTypesThreshold has been passed, and can be disabled from doing so entirely with the Yno-deep-subtypes option.
Always proceeding with the recursive elimination of lazyrefs here could be expensive for most cases where it is unnecessary, even though it is indeed much cheaper in the overflowing cases.

While this PR can detect some cyclic cases earlier, it appears the underlying issue has to do with the bloop server (as discussed in #21521 (comment)).

@wjoel
Copy link
Contributor Author

wjoel commented Sep 7, 2024

Yeah, this might not be the right fix, see #21521 (comment)

@wjoel
Copy link
Contributor Author

wjoel commented Sep 14, 2024

Closed in favor of #21566

@wjoel wjoel closed this Sep 14, 2024
@wjoel wjoel deleted the fix-javagi-gtk branch September 14, 2024 11:58
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.

StackOverflowError when using Java code that creates types with LazyRef
3 participants