Skip to content

Scaladoc: type rendering fixes and improvements #17213

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 6 commits into from
Jun 2, 2023

Conversation

Florian3k
Copy link
Contributor

@Florian3k Florian3k commented Apr 6, 2023

This PR introduces multiple fixes and improvements to type rendering in Scaladoc

fixes #16024
fixes #16143
fixed #16057
fixes #16084 (partially) - fixed rendering of Int instead of n.type and (This, n.type) instead of Split[This, n.type]. this.type in type bounds remains unfixed. I created separate issue for that #17226

Documentation with those changes has been deployed to https://scala3doc.virtuslab.com/pr-types-rendering-fixes/scala3/api/index.html

@KacperFKorban KacperFKorban requested a review from szymon-rd April 6, 2023 11:45
@Florian3k Florian3k marked this pull request as ready for review April 6, 2023 11:45
Copy link
Contributor

@Sporarum Sporarum left a comment

Choose a reason for hiding this comment

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

Looks amazing, great job !

I think we should keep/add parens anytime it's ambiguous however, see the proposed changes for more details

@Florian3k Florian3k requested a review from Sporarum April 13, 2023 12:22
Copy link
Contributor

@Sporarum Sporarum left a comment

Choose a reason for hiding this comment

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

Thank you for addressing my comments

I found some other things, feel free to address them as well if you want to ^^
(I consider them non-blocking)

// issue 16024

class X[Map[_, _[_]]]:
inline def map[F[_]](f: [t] => t => F[t]): Map[this.type, F] = //expected: inline def map[F[_]](f: [t] => (x$1: t) => F[t]): Map[this.type, F]
Copy link
Contributor

Choose a reason for hiding this comment

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

More nitpicking:
Would it be possible to only print the identifier (x$1) if it is required ?

Ex: [t] => (x: t) => t

I see two ways of doing so:

  1. Look at whether the name is synthetic/mangled: ex prints as [t] => (x: t) => t
  2. Look at whether the name is used on the rhs: ex prints as [t] => t => t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave that for future improvements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you open an issue for it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: #17756

@@ -14,18 +14,18 @@ class A: //unexpected
= 1
var aVar1: 1
= 1
type HKT[T[_], X] //expected: final type HKT = [T[_], X] =>> HKT[T, X]
type HKT[T[_], X] //expected: final type HKT = [T[_], X] =>> a.HKT[T, X]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why we need the a/A in theses, could you explain ?

Copy link
Contributor Author

@Florian3k Florian3k Apr 19, 2023

Choose a reason for hiding this comment

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

This file is part of test consisting of 2 files: exports1.scala and exports2.scala

The test:
https://github.com/lampepfl/dotty/blob/ef974367932cb148bda152ed056183f319a9a2fc/scaladoc/test/dotty/tools/scaladoc/signatures/TranslatableSignaturesTestCases.scala#L96

exports2.scala for reference:
https://github.com/lampepfl/dotty/blob/ef974367932cb148bda152ed056183f319a9a2fc/scaladoc-testcases/src/tests/exports2.scala#L1-L12

So it's testing for exported member signatures in class B.

Signatures are rendered based on TypeRepr from Tasty.
For example TypeRepr for val x: HKT[List, Int] rendered in class B looks like this (not valid scala, this is slightly formatted output from println):

AppliedType(
  TypeRef(
    ThisType(TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class tests)),object exports1),class A)), // TypeRepr for class A
    type HKT
  ),
  List(
    TypeRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),package),List), // TypeRepr for List
    TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),Int) // TypeRepr for Int
  )
)

Based solely on this representation, signature that should be rendered is A#HKT[List, Int]

To be honest, I'm not entirely sure whether this is correct way to render signatures of exported members.
If this is not correct, we may want to add some additional logic for checking if current signature is of an exported member and render it in a different way, but I'm probably gonna need some help with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sjrd or @bishabosha, you are very familiar with tasty, what do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

a.HKT[T, X] seems absolutely correct to me. Otherwise you would be defining B.HKT in terms of itself.

The A#HKT[List, Int] seem a bit suspicious to me. this prefix substitution (the so-called asSeenFrom operation) should be replacing this.HKT[List, Int] from A by a.HKT[List, Int] when created by an export a._.

In fact, the TypeRef seems even more suspicious. As is, it means A.this.HKT, not really A#HKT. But that's quite bad if that's the type given to a member of B, since such a type is only valid when you're inside A. It seems the compiler is doing something wrong here.

From the point of view of Scaladoc, I think it should display A.this.HKT for the TypeRef(ThisType(A), type HKT).

@Florian3k Florian3k requested a review from Sporarum June 2, 2023 11:10
@Florian3k
Copy link
Contributor Author

@Sporarum I fixed rendering of ThisType and removed parens around occurrences of the same operator

@Sporarum
Copy link
Contributor

Sporarum commented Jun 2, 2023

Perfect, then from my point of view, this is good to merge !

@Florian3k Florian3k merged commit 57c2b66 into scala:main Jun 2, 2023
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 2, 2023
Kordyjan added a commit that referenced this pull request Nov 21, 2023
…8971)

Backports #17213 to the LTS branch.

PR submitted by the release tooling.
[skip ci]
@Kordyjan Kordyjan modified the milestones: 3.4.0, 3.3.2 Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants