feat(dhall-docs): add jump-to-definition support for record-like expressions #1991

german1608 · 2020-08-12T21:02:45Z

(this is a quite big PR, please read the full description of this PR before reviewing the code)

Summary

This PR adds JTD support for record-like expressions. dhall-docs now tries to infer (really simple, not like Dhall.TypeCheck does, for example) the type of a dhall expression. As a result of this, the JTD-related features on record-like expressions are:

Fields on field-access expressions jumps to the definition of that field in the source code, where the left side of that expression was defined;
Deep field expressions are also supported (see JumpToRecordFieldWhenVarIsOfRecordTypeDeep test case);
Fields that use dot-syntax are correctly highlighted depending on the accessed field;
Punned entries are handled without jumping to the definition of that entry, but it's enough I think;
It supports transitivity of expressions, which allows jumping to the real definition of an expression if dhall-docs is able to keep the inferred type in a transitivity chain.

You can check the written test cases to see more specific uses, and also you can suggest more test cases as well.

How to review this

This work is divided into two parts:

Modifications of how we preserve whitespace on the AST:
1. The addition of the FieldAccess data-type for preserving whitespace on Field expression
2. The correct whitespace preservation on dot-syntax labels, which is documented in the RecordField haddock
3. Modify the rest of the repository packages to catch-up the change on Field
Modifications on how the source code on rendered in fragments:
- We have a DhallType ADT to aim a simple type-inference algorithm.
- Now we use a Writer [SourceCodeFragment] DhallType. Generating [SourceCodeFragment] for an expression will return the inferred 'DhallType'

The part 1 generates a lot of noise on the PR, but I was modifying the parser as I was developing record-JTD since some changes were really specific for dhall-docs. If you need to, I can split (not so easy, though) the dhall work in a separate PR that should be merged before this one.

dhall-docs/tasty/data/package/JumpToRecordFieldWhenVarIsOfRecordTypeTransitivity.dhall

dhall-docs/tasty/data/package/JumpToRecordFieldWhenVarIsAnnotatedWithRecordType.dhall

sjakobi

@german1608, can you show us some docs that show off the new features?

I have a few comments on the changes to dhall for now. I'm not done with the parser changes yet. I think it would be good if @Gabriel439 would take a closer look at these too.

dhall/src/Dhall/Syntax.hs

sjakobi · 2020-08-13T09:48:52Z

dhall/tests/Dhall/Test/Dhall.hs


-data RecursiveType a = RecursiveType (RecursiveType a)
+newtype RecursiveType a = RecursiveType (RecursiveType a)


I assume that this was changed by stylish-haskell?!

@Gabriel439, could you check that this is OK?

I changed that manually, I have a TOC that males me change to newtype when possible :) I can rollback the change, though

dhall/src/Dhall/Syntax.hs

dhall/src/Dhall/Parser/Expression.hs

dhall/src/Dhall/Normalize.hs

german1608 · 2020-08-13T12:59:05Z

@german1608, can you show us some docs that show off the new features?

I'm unable to provide specific examples right now since I'm not home, but you can check the generated docs for the test package and the prelude. renderAs on Prelude/JSON has a lot of stuff

german1608 · 2020-08-13T14:38:55Z

@german1608, can you show us some docs that show off the new features?

I'm unable to provide specific examples right now since I'm not home, but you can check the generated docs for the test package and the prelude. renderAs on Prelude/JSON has a lot of stuff

On the Prelude there isn't a lot of examples, some JTD features can be seen in the Prelude/JSON subpackage.

The following examples on the test packages shows off the new features:

...or you can manually check on the index package on all the examples that starts with JumpToRecordFieldWhen...

dhall/src/Dhall/Parser/Expression.hs

dhall/src/Dhall/Syntax.hs

dhall/src/Dhall/Normalize.hs

dhall/src/Dhall/Parser/Expression.hs

sjakobi

In https://hydra.dhall-lang.org/build/69960/download/1/docs/JSON/renderAs.dhall.html I noticed that every single label in a record literal seems to be highlighted. But when I hover over these or click them, nothing interesting happens. What's the reason for highlighting these labels?

german1608 · 2020-08-14T14:00:13Z

In https://hydra.dhall-lang.org/build/69960/download/1/docs/JSON/renderAs.dhall.html I noticed that every single label in a record literal seems to be highlighted. But when I hover over these or click them, nothing interesting happens. What's the reason for highlighting these labels?

That is because the implementation doesn't recognize where a record (literal or type) is stored and in any case it renders the highlight. On this moment, we recognize when a record literal has been bound to a let binding and when a lambda name is of record-type, but the rest of the cases are not handled.

I guess that we could remove the highlight on unused names, although when handling imported names (as I explained here) it would get messy. Something that a couple of tests doesn't solve.

german1608 · 2020-08-14T14:06:19Z

I'll be quite bussy today, so I'll address remaining comments this night.

sjakobi · 2020-08-15T10:52:21Z

I guess that we could remove the highlight on unused names, although when handling imported names (as I explained here) it would get messy. Something that a couple of tests doesn't solve.

OK. Currently it seems to me that there's not much point in highlighting things that don't offer more information in the same view. So when a label in one file is only used in other files, I think it's probably not worth highlighting in the first file.

Maybe this issue could also be addressed by using a more sophisticated highlighting approach that uses different visual cues instead of the current one-size-fits-all design with the underlining. I'm not sure.

I noticed that some types in the index pages also contain highlighting, e.g. in https://hydra.dhall-lang.org/build/70067/download/1/docs/List/index.html. Can these labels be jumped to from anywhere?

dhall-docs/src/Dhall/Docs/CodeRenderer.hs

german1608 · 2020-08-15T14:10:26Z

OK. Currently it seems to me that there's not much point in highlighting things that don't offer more information in the same view. So when a label in one file is only used in other files, I think it's probably not worth highlighting in the first file.

I see. Then let me do some work to remove the unused highlights. It shouldn't be difficult

german1608 · 2020-08-15T16:25:11Z

@sjakobi Alright, I've applied your suggestions and I removed the highlighting on unused names. Note that unused let-bindings are not highlighted as well. Check this test case

A thing that I'd like to do is to rename var to name when possible since I feel that var is more specific to let-bindings, but I'll do it in a follow-up PR to not add more noise here

sjakobi

LGTM apart from a few more wibbles.

Great job, @german1608! 👍

dhall-docs/src/Dhall/Docs/CodeRenderer.hs

whitespaces is preserved now... * Between a label and a colon or equal sign * Between a colon or equal sign and an expression

…let-bindings

german1608 · 2020-08-17T02:33:03Z

Great job, @german1608!

Thanks for your good reviews!

german1608 added dhall-docs dhall-docs:jump-to-def labels Aug 12, 2020

german1608 mentioned this pull request Aug 12, 2020

Advice for updating Expr AST in the context of dhall-docs & dhall format #1986

Closed

german1608 force-pushed the feat/jump-to-record-labels branch from 63d94fd to 9457a9e Compare August 12, 2020 21:03

Gabriella439 reviewed Aug 13, 2020

View reviewed changes

dhall-docs/tasty/data/package/JumpToRecordFieldWhenVarIsOfRecordTypeTransitivity.dhall Outdated Show resolved Hide resolved

dhall-docs/tasty/data/package/JumpToRecordFieldWhenVarIsAnnotatedWithRecordType.dhall Outdated Show resolved Hide resolved

sjakobi reviewed Aug 13, 2020

View reviewed changes

german1608 mentioned this pull request Aug 13, 2020

dhall-docs input as expression #1994

Open

german1608 force-pushed the feat/jump-to-record-labels branch from d922d46 to 33700b0 Compare August 13, 2020 19:11

german1608 mentioned this pull request Aug 13, 2020

Test setup for preserved whitespace on AST #1995

Open

4 tasks

sjakobi reviewed Aug 14, 2020

View reviewed changes

sjakobi reviewed Aug 15, 2020

View reviewed changes

dhall-docs/src/Dhall/Docs/CodeRenderer.hs Outdated Show resolved Hide resolved

dhall-docs/src/Dhall/Docs/CodeRenderer.hs Show resolved Hide resolved

dhall-docs/src/Dhall/Docs/CodeRenderer.hs Outdated Show resolved Hide resolved

sjakobi approved these changes Aug 16, 2020

View reviewed changes

german1608 added 10 commits August 16, 2020 22:11

preserve more whitespaces on record key-valuese

97d8238

whitespaces is preserved now... * Between a label and a colon or equal sign * Between a colon or equal sign and an expression

add test cases for what do we expect from record-literals jtd

223fd34

add TODO

8a8cec6

[wip] add dhallTypeFromExpr

36ba6cd

highlight record literal' labels

9bd7319

add support for prefix whitespace for label on 'Field'

ba79d8e

[wip] add jtd on fields

0c45d93

document expected behaviour of Maybe s on RecordField for dot-syntax

c7133bc

correctly parse whitespace in-between on dot-syntax

f3b5f0a

remove some Justs

430c4ce

german1608 added 23 commits August 16, 2020 22:11

remove Debug.Trace usages

5b7cbbd

update golden files

8279aca

merge inferWithContext and go on infer

503279c

handle let-binding record-like annotations and add more test cases

92b27c4

remove Debug.Trace (again)

a204b71

fix haddock

0339db8

add haddock for makeFieldAccess

cd986bf

fix bench suite

b669e2f

FieldAccess -> FieldSelection

28c70cb

improve haddocks

d57ba8d

check that Justs were removed on normalization

bfd6a72

update test cases to reflect changes of ignoring type-annotations on …

84f56b1

…let-bindings

add test for Lam bindings

feb2d33

enable type from annotation on lam bindings

51cd7c7

fieldParser -> fieldSelection

09ae409

use pattern matching instead of Maybe.isNothing

72f8b5c

use 'Nothing' un init of dot-syntax fields

bb599f3

DhallType -> JtdInfo, RecordLiteral -> RecordFields, AnyType -> NoInfo

9e17164

improve haddocks

dcdf3a0

remove highlights on unused names

1cec6d2

add unused name test case

1edf723

fix typos

970703a

remove unused before sort

357d7c8

german1608 force-pushed the feat/jump-to-record-labels branch from 229842a to 357d7c8 Compare August 17, 2020 02:12

german1608 added the merge me label Aug 17, 2020

mergify bot merged commit 8d89b44 into master Aug 17, 2020

mergify bot deleted the feat/jump-to-record-labels branch August 17, 2020 03:17

german1608 mentioned this pull request Aug 17, 2020

fix(dhall-docs): rename var to name when it makes sense #2002

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dhall-docs): add jump-to-definition support for record-like expressions #1991

feat(dhall-docs): add jump-to-definition support for record-like expressions #1991

german1608 commented Aug 12, 2020

sjakobi left a comment

sjakobi Aug 13, 2020

german1608 Aug 13, 2020

german1608 commented Aug 13, 2020

german1608 commented Aug 13, 2020

sjakobi left a comment

german1608 commented Aug 14, 2020

german1608 commented Aug 14, 2020

sjakobi commented Aug 15, 2020

german1608 commented Aug 15, 2020

german1608 commented Aug 15, 2020

sjakobi left a comment

german1608 commented Aug 17, 2020


		data RecursiveType a = RecursiveType (RecursiveType a)
		newtype RecursiveType a = RecursiveType (RecursiveType a)

feat(dhall-docs): add jump-to-definition support for record-like expressions #1991

feat(dhall-docs): add jump-to-definition support for record-like expressions #1991

Conversation

german1608 commented Aug 12, 2020

Summary

How to review this

sjakobi left a comment

Choose a reason for hiding this comment

sjakobi Aug 13, 2020

Choose a reason for hiding this comment

german1608 Aug 13, 2020

Choose a reason for hiding this comment

german1608 commented Aug 13, 2020

german1608 commented Aug 13, 2020

sjakobi left a comment

Choose a reason for hiding this comment

german1608 commented Aug 14, 2020

german1608 commented Aug 14, 2020

sjakobi commented Aug 15, 2020

german1608 commented Aug 15, 2020

german1608 commented Aug 15, 2020

sjakobi left a comment

Choose a reason for hiding this comment

german1608 commented Aug 17, 2020