Omit kind sigs where kind is uninteresting, even when parens are involved by JordanMartinez · Pull Request #4137 · purescript/purescript

JordanMartinez · 2021-07-06T13:42:17Z

Description of the change

Fixes #4136

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)

kl0tl

Should we also drop sorts annotations from developper written kinds?

kl0tl · 2021-07-09T12:06:20Z

src/Language/PureScript/Docs/Convert.hs

  isUninteresting keyword = \case
-    P.TypeApp _ t1 t2 | t1 == kindFunctionType ->
+    -- `Type -> ...`
+    P.TypeApp _ (P.TypeApp _ (P.TypeConstructor () Prim.Function) ty) t2 | isKindPrimType ty ->


We need to account for the case where an application is wrapped in parens, like (Type -> Type), otherwise we deem such kinds interesting and they show up in the generated docs.

Meaning if we have something like this?

data Foo :: (Type -> Type) data Foo a = Foo a

which is the same as writing the uninteresting kind signature below:

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

Yeah exactly, data Foo :: (Type -> Type) should be treated like data Foo :: Type -> Type but the kind is currently documented.

Wouldn't that mean accounting for various other situations, too? I'm starting to wonder if this is really worth doing and if we should instead say, "Yeah, just don't do that."

As examples of other things:

-- or any uninteresting kind where the whole thing is wrapped in parens data Foo1 :: (Type -> Type -> Type) data Foo1 a b = Foo1 a b -- or any uninteresting kind where the the tail part is wrapped in parens data Foo2 :: Type -> (Type -> Type) data Foo2 a b = Foo2 a b

Yes those kinds should be uninteresting too. It should only be a matter of adding a P.ParensInType _ ty -> isUninteresting ty case to isUninteresting though.

Done and added tests for it. I haven't tested this locally.

Does this cover data Foo :: (->) Type Type? Or data Foo :: ((->) Type) Type, or, for that matter, data Foo :: ((((->)) Type)) Type?

These are profoundly obnoxious edge cases, but if they cause problems, maybe the thing to do is just strip out all parens using a traversal before calling isUninteresting.

Good question. I'll add a test for that.

Latest commit adds these three tests, the latter two of which currently fail.

Yeah, stripping those parenthesis out and then solving the problem with the original approach was much easier than trying to account for them at the same time.

JordanMartinez · 2021-07-09T19:51:47Z

Note: while I was working on this PR, I also learned that the parens situation will be displayed in the kind signature if it's included in an interesting kind signature. For example:

data Foo :: forall k. k -> (Type)
data Foo a = Foo

will be rendered as the above, not as below:

data Foo :: forall k. k -> Type
data Foo a = Foo

I'm not sure whether it's really worth it to remove the parenthesis because I doubt this will happen in practice.

JordanMartinez · 2021-07-10T00:02:50Z

Should we also drop sorts annotations from developper written kinds?

Meaning, if the developer writes

data Foo (a :: Type) = Foo a

then we render the following?

data Foo a = Foo a

JordanMartinez · 2021-07-10T00:15:20Z

Just added a test for this. It passed but nothing changed in my code:

data DataSortAnnotation (a :: Type) = DataSortAnnotation a

rhendric · 2021-07-10T00:43:30Z

data DataSortAnnotation (a :: Type) = DataSortAnnotation a

That's still a kind annotation. I think a sort annotation would be the :: Type in

foreign import data DataSortAnnotation :: forall (k :: Type). k -> Type

JordanMartinez · 2021-07-10T01:02:10Z

Mm... I'm not so sure. Harry said something about this that makes me think otherwise...?

JordanMartinez · 2021-07-12T20:56:17Z

Changelog entry has been added now that #4132 is in.

rhendric · 2021-07-12T22:00:25Z

The changelog file name should start with fix, not bug.

JordanMartinez · 2021-07-14T13:47:47Z

This PR should be ready for another review when someone has a moment.

kl0tl

I still think we should render

foreign import data DataSortAnnotation :: forall (k :: Type). k -> Type

as

foreign import data DataSortAnnotation :: forall k. k -> Type

for consistency with inferred kinds, but that’s not really related to this pull request so it can happen in another one (and perhaps it should rather be done by a formatter). Ditto for extraneous parens in interesting kinds.

This looks great though, thank you!

JordanMartinez · 2021-07-16T13:33:05Z

I still think we should render

Isn't that handled in #4121? Since the declaration is FFI, #4121 first converts that to a data declaration and then runs it through the code that removes the sort annotation.

Regardless, thanks for the review. Merging!

kl0tl · 2021-07-16T16:36:23Z

Isn't that handled in #4121? Since the declaration is FFI, #4121 first converts that to a data declaration and then runs it through the code that removes the sort annotation.

This doesn’t seem to be the case: on 73ff5f6 we render

for

foreign import data DataSortAnnotation :: forall (k :: Type). k -> Type

because we only drop sort annotations for inferred kinds.

JordanMartinez · 2021-07-17T14:48:03Z

Gotcha. I've reported this as a bug in #4160

kl0tl reviewed Jul 9, 2021

View reviewed changes

JordanMartinez added 11 commits July 12, 2021 13:53

Consider redundant parenthesis when checking for "uninteresting" kinds

cd6f798

Test that uninteresting kinds in redundant parens are still not rendered

5457575

Also exclude uninteresting kind sigs for head, tail, and whole variants

f7a17a0

Also test against parens around single kind

c51844a

Fix spelling mistake

07e54c0

Fix new spelling mistake

f9695a8

Create section for explicit kind signatures where parens are involved

32f6e8a

Test: uninteresting kind sigs via sort annotations are not displayed

6833fbb

Add variant of sort annotation wrapped in parens

136bc5e

Use correct terminology: kind annotation, not sort annotation

099e8d6

Add changelog entry

e042792

JordanMartinez added 6 commits July 12, 2021 18:39

Use correct prefix for changelog entry

a7f2dec

Add test for unique function application situation

8a0cfc9

Fix typo: add missing space

6c2f21a

Drop parens first and then find uninteresting kind sigs

4f214dd

Refer to correct function

b76bb12

Account for parens using better checks

a74fdeb

kl0tl approved these changes Jul 16, 2021

View reviewed changes

JordanMartinez merged commit 2b9c7a6 into purescript:master Jul 16, 2021

JordanMartinez deleted the fixUninterestingKindsWhenParensIncluded branch July 16, 2021 13:36

This was referenced Jul 17, 2021

Display roles in HTML docs #4121

Merged

Drop Type sort annotations in all kind signatures for docs, not just inferred kind signatures #4160

Closed

Conversation

JordanMartinez commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kl0tl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JordanMartinez commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JordanMartinez commented Jul 10, 2021

Uh oh!

JordanMartinez commented Jul 10, 2021

Uh oh!

rhendric commented Jul 10, 2021

Uh oh!

JordanMartinez commented Jul 10, 2021

Uh oh!

JordanMartinez commented Jul 12, 2021

Uh oh!

rhendric commented Jul 12, 2021

Uh oh!

JordanMartinez commented Jul 14, 2021

Uh oh!

kl0tl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JordanMartinez commented Jul 16, 2021

Uh oh!

kl0tl commented Jul 16, 2021

Uh oh!

JordanMartinez commented Jul 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JordanMartinez commented Jul 6, 2021 •

edited

Loading

JordanMartinez commented Jul 9, 2021 •

edited

Loading

kl0tl left a comment •

edited

Loading