Skip to content

Scaladoc: fixes and improvements to context bounds and extension methods #22156

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

Conversation

Florian3k
Copy link
Contributor

@Florian3k Florian3k commented Dec 6, 2024

Closes #21662

More specifically:

Additionally fixed and improved simplifying of type lambdas (TypesSupport.scala)

Removed MemberInfo and unwrapMemberInfo, which were quite complicated, doing several things at the same time and caused parseMember to have two sources of truth.

@Florian3k Florian3k force-pushed the scaladoc-context-bounds-extension-methods branch 2 times, most recently from 67ee8ec to 7540be2 Compare December 6, 2024 17:14
@rjolly
Copy link
Contributor

rjolly commented Dec 31, 2024

The change works for me.

@rjolly
Copy link
Contributor

rjolly commented Feb 10, 2025

@Florian3k , if you do not have more changes, maybe we could remove the draft status, what do you think? @mbovel please could you consider a review? Thanks!

@Florian3k
Copy link
Contributor Author

There's one more thing to fix. Changes in this PR uncovered some bugs in rendering of this.type in some places. I was busy in January, but I'll be finishing this soon.

@jchyb jchyb force-pushed the scaladoc-context-bounds-extension-methods branch from 7540be2 to 86c6894 Compare May 14, 2025 14:30
@jchyb
Copy link
Contributor

jchyb commented May 15, 2025

Note for @Florian3k, about my adjustments with this-types:
Truthfully I didn’t change much since the WIP PR, since it mostly looked good to go. The added this types in most cases can be helpful in avoiding ambiguities - notably in the tastcase case class IAmACaseClassWithParam[T](x: Documentation.this.T, id: T) I added, without the rendered Documentation.this.T, we would not know which T was referenced. I adjusted test cases classSignatureTestSource.scala and exports1.scala with that in mind.
They aren’t really helpful when it comes to the supertypes in the extends clause - they will always be classlikes, and they will always have links in the signature, so ambiguities aren’t an issue. For those I ended up just adding skipThisTypeInPrefix parameter to TypesSupport. I unfortunately was not able to make it cleaner by adding it as a default parameter (when I did that I had some surpiring issues with path dependent types), so the asSignature methods are overridden to achieve the same effect.

@rjolly
Copy link
Contributor

rjolly commented May 27, 2025

The new changes work for me.

@Florian3k Florian3k marked this pull request as ready for review June 29, 2025 21:21
@Florian3k
Copy link
Contributor Author

Florian3k commented Jun 29, 2025

Those new changes seem to be working fine.
It's a bit unfortunate that we have to render Documentation.this.T instead of T (this effect will be also visible in some places in std lib docs), but I understand that correctness beats prettiness.
I think improving that should be tracked in separate issue.

@@ -26,7 +26,7 @@ abstract class Documentation[T, A <: Int, B >: String, -X, +Y](c1: String, val c

sealed trait CaseImplementThis(id: Int)

case class IAmACaseClass(x: Documentation.this.T, id: Int) extends CaseImplementThis/*<-*/(id)/*->*/
case class IAmACaseClass(x: T, id: Int) extends CaseImplementThis(id) //expected: case class IAmACaseClass(x: Documentation.this.T, id: Int) extends CaseImplementThis
Copy link
Contributor Author

@Florian3k Florian3k Jun 29, 2025

Choose a reason for hiding this comment

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

I've adjusted this test case to better reflect what is really going on here.

@Florian3k Florian3k requested a review from jchyb June 29, 2025 21:44
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgodzik tgodzik merged commit 665f6d7 into scala:main Jul 1, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants