Skip to content

Display roles in HTML docs#4121

Merged
JordanMartinez merged 57 commits intopurescript:masterfrom
JordanMartinez:addRolesToDocs
Nov 17, 2021
Merged

Display roles in HTML docs#4121
JordanMartinez merged 57 commits intopurescript:masterfrom
JordanMartinez:addRolesToDocs

Conversation

@JordanMartinez
Copy link
Copy Markdown
Contributor

@JordanMartinez JordanMartinez commented Jun 26, 2021

Description of the change

Fixes #4116

WIP but should help continue the discussion already started.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Added myself to CONTRIBUTORS.md (if this is my first contribution)
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

Current changes made in this PR

  • role declarations will now be exported in AST.Exported.exportedDeclarations
  • another augmentation pass has been added that inserts the roles into the corresponding declaration
  • the child info types for data, newtype, and type declarations now include a [Role] argument

I haven't yet tried to render the roles in the HTML docs for a few reasons. First, we're still coming to an agreement about how we want to do this (though the subscript idea seems to be favored). Second, I'm still figuring out how the rendering code works and how I would insert role subscripts.

I'll ask other questions in the code itself.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Here's the control flow for the rendering pipeline

  1. a declaration is rendered via renderDeclaration
  2. the info part of the docs is rendered via a couple of parts:
    1. Convert the args back into a P.Type' via typeApp. We definitely can't add role information here.
    2. Convert P.Type' into PrettyPrintType via convertPrettyPrintType.
    3. Convert PrettyPrintType into a RenderedCode value, which consists of a list of RenderedCodeElement values, via combination of renderType', matchType, matchTypeAtom, and typeLiterals.
  3. Convert the RenderedCode value into its corresponding HTML via the convertToHtml function

If we updated PrettyPrintType's TypeVar to include a Maybe P.Role arg (or something like that) and used Nothing for that arg's value by default, then we could add a pass between 2.ii and 2.iii that would insert the corresponding role there. If we're using the subscript idea, then I believe we need to add a new data constructor for RenderedCodeElement, so that it renders that content using subscripts.

However, there are multiple uses of mintersperse sp throughout this code. We might need to adjust parts of that, so that we don't get a space between the type parameter and the role (e.g. data Map k nominal v).

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Finally getting back to this. I believe the final HTML and CSS we'll use will look like this (except all one one line rather than multiple lines like the example below). Here's a breakdown of what's happening:

<a href="#">
  <abbr title="The 'nominal' role means this type cannot be safely coerced into another type">
    <sub class="role__nominal"></sub>
  </abbr>
</a>
.role__nominal::after {
  content: "nominal";
}
.role__phantom::after {
  content: "phantom";
}

Below is a screenshot of HalogenM with a few roles added ad-hoc where I'm hovering over the first one. Do we want to style the role annotations differently than what I'm showing below?

role-annotation-example

@MonoidMusician
Copy link
Copy Markdown
Contributor

Looks good to me! I would only suggest for wording: “… this type argument cannot be safely coerced into another type”, so it’s clear we are talking about the arguments, not actual types. Maybe there’s even a clearer way to phrase it to show the relationship of type arguments/roles to coercing the entire type.

@rhendric
Copy link
Copy Markdown
Member

rhendric commented Jul 2, 2021

Re wording: how about: ‘... means this argument may not change when coercing this type’ and ‘... means this argument can change freely when coercing this type’ for nominal and phantom respectively?

Re style: this is all highly subjective, but I propose:

  • using the sans-serif proportional font family used by documentation body text, instead of the fixed-width family used for code
  • italicizing the role
  • hiding the underline, like with other links

I think maintaining the convention that fixed-width means code is important, and I think the italics help visually clarify the distinction when fixed-width and proportional fonts are mixed.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

For wording, I went with this before I saw @rhendric's comment above:

describeNominal = fold
          [ "The 'nominal' role means the type represented by this type "
          , "parameter CANNOT be safely coerced to another type when one "
          , "coerces the overall type to another type. For example, given "
          , "`data Foo a` where `a` has the nominal role, the type, "
          , "`Foo Age`, cannot be safely coerced to the type, "
          , "`Foo Int`."
          ]

describePhantom = fold
          [ "The 'phantom' role means the type represented by this type "
          , "parameter CAN be safely coerced to any other type when one "
          , "coerces the overall type to another type. For example, given "
          , "`data Foo a` where `a` has the phantom role, the type, "
          , "`Foo Age`, can be safely coerced to the type, "
          , "`Foo Int`, or even a non-sensical type like, "
          , "`Foo (Foo (Foo String))`."
          ]        

Re style: this is all highly subjective, but I propose:

While it is subjective, I agree with your idea. Part of the reason why I asked was I, too, thought the underline unneeded.

@rhendric
Copy link
Copy Markdown
Member

rhendric commented Jul 2, 2021

I don't think we need ‘for example’s in the tooltips. They're already links; if a quick reminder isn't sufficient, the user can click.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

JordanMartinez commented Jul 2, 2021

Given

module Main where

data Foo :: Type -> Type -> Type -> Type
data Foo a b c = Foo

type role Foo nominal phantom representational

the output of spago docs -S produces the following:

role-annotations-added


role-annotations-with-hover

Also, I'm currently linking to Roles.md. Is that the URL we want to use?

@rhendric
Copy link
Copy Markdown
Member

rhendric commented Jul 2, 2021

Oops, sorry, also I think the roles should be normal weight and not bold. (It was much less obvious that they were bold in the other font!)

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

No worries. Latest screenshot:

role-ann-without-bold

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

So, this PR is about ready for review. There's a few remaining questions:

  • any changes requested on the abbr title content (i.e. what appears when you hover over a nominal/phantom role)?
  • the tests currently prove that the roles are added to the Docs.Declaration. However, there's nothing testing that the rendering of those docs is accurate. What do we need to do to test that (e.g. golden test or something)?
  • looking at declName, there are only four other usages of it. Once in IDE.State.hs, twice in Linter.hs (first and second), and once in Sugar.Names.hs. Should I refactor declName to export RoleDeclaration names?

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

CI started hanging due to an internal error. Getting #4126 in will stop that.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

CI passes now.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

Given the below code...

module Main where

data DataAll :: Type -> Type -> Type -> Type
data DataAll a b c = DataAll
type role DataAll nominal phantom representational

data TypeOnly_UninterestingKind_InferRole :: Type -> Type -> Type
data TypeOnly_UninterestingKind_InferRole a b

data TypeOnly_InterestingKind_InferRole :: Type -> (Type -> Type) -> Type
data TypeOnly_InterestingKind_InferRole a b

foreign import data FFI_UninterestingKind_InferRole :: Type -> Type -> Type

foreign import data FFI_InterestingKind_InferRole :: forall k. k -> Symbol -> Type

foreign import data FFI_UninterestingKind_ExplicitRole :: Type -> Type -> Type
type role FFI_UninterestingKind_ExplicitRole phantom nominal

foreign import data FFI_InterestingKind_ExplicitRole :: forall k. k -> Symbol -> Type
type role FFI_InterestingKind_ExplicitRole phantom phantom
the current PR produces the following screenshots

inferred-roles-1

inferred-roles-2

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

I realized I was only inserting the inferred roles for FFI declarations. Latest commit also inserts inferred roles into data declarations.

@kl0tl
Copy link
Copy Markdown
Member

kl0tl commented Jul 16, 2021

Synonyms are not supported in foreign data types kinds: we render

Capture d’écran 2021-07-16 à 18 18 17

for

foreign import data FFI1 :: (Function Type) Type

but

Capture d’écran 2021-07-16 à 18 18 29

for

type To = Function
foreign import data FFI1 :: (To Type) Type

This is perhaps too uncommon to be dealt with in this pull request but we could do so by building a SynonymMap, like we do when desugaring instances, and then call Language.PureScript.TypeChecker.Synonyms.replaceAllTypeSynonymsM in convertFFIDecl.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

@kl0tl The "sort annotations are only dropped for inferred kind signatures" issue you described here has been addressed in latest commits here. Not sure whether it's sensible to add it in this PR. However, the advantage of doing so here means I can add FFI declaration tests here, too.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

This is perhaps too uncommon to be dealt with in this pull request but we could do so by building a SynonymMap, like we do when desugaring instances, and then call Language.PureScript.TypeChecker.Synonyms.replaceAllTypeSynonymsM in convertFFIDecl.

Yeah... that sounds like it should be in its own PR.


, ShouldHaveRoleAnnotation (n "RoleAnnotationDocs") "FFI_HeadParens" [P.Representational, P.Nominal, P.Phantom]
-- , ShouldHaveRoleAnnotation (n "RoleAnnotationDocs") "FFI_TailParens" [P.Representational, P.Nominal, P.Phantom]
-- , ShouldHaveRoleAnnotation (n "RoleAnnotationDocs") "FFI_WholeParens" [P.Representational, P.Nominal, P.Phantom]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pending #4157

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

With #4160 being resolved, is there any other work to do here besides waiting for #4157 to get merged and then adding additional tests?

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

@kl0tl I generated markdown docs to see whether the role annotations are rendered correctly. Looks like we can't use the HTML tags where I put them because the entire declaration is wrapped in a code block.

Given

module Main where


data Foo a = Foo a
type role Foo nominal

we get (horizontal lines added to clearly indicate start/finish of docs):


Module Main

Foo

data Foo a<sub title="The 'nominal' role means this argument may not change when coercing the type."><em><a href="https://github.com/purescript/documentation/blob/master/language/Roles.md">nominal</a></em></sub>
  = Foo a

Copy link
Copy Markdown
Member

@kl0tl kl0tl left a comment

Choose a reason for hiding this comment

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

Looks like we can't use the HTML tags where I put them because the entire declaration is wrapped in a code block.

Oh, let’s not bother with roles in Markdown docs then.

…, is there any other work to do here besides waiting for #4157 to get merged and then adding additional tests?

It looks quite good to me modulo the Markdown docs, sorry about that!

JordanMartinez and others added 2 commits July 23, 2021 11:02
Co-authored-by: Cyril <sobierajewicz.cyril@gmail.com>
@JordanMartinez
Copy link
Copy Markdown
Contributor Author

The markdown docs commit has been reverted and I fixed the typo you found.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

CI passes.

@JordanMartinez
Copy link
Copy Markdown
Contributor Author

If no one has any further feedback on this, I'm going to merge this in two days.

@JordanMartinez JordanMartinez merged commit 2c78eb6 into purescript:master Nov 17, 2021
@JordanMartinez JordanMartinez deleted the addRolesToDocs branch November 17, 2021 21:27
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.

Include role information in HTML docs

5 participants