fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

port fsih to fsi as a hash directive

Open dawedawe opened this issue 1 year ago • 3 comments

Description

This is a port of fsih to fsi itself as a hash directive.

image

I think using a hash directive for this makes the most sense but as we use the normal parser for them it also means we have to wrap the expression in parentheses. I hope that's an acceptable tradeoff. If you want to give this a try, you have to copy the FSharp.Core.xml manually to the fsi artifacts folder. The build process doesn't do it. Later, in the SDK distribution, the xml file will be there as far as I can see.

Checklist

  • [ ] Test cases added

  • [ ] Performance benchmarks added in case of performance changes

  • [X] Release notes entry updated:

    Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

    Release notes files:

    • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
    • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/.Language/preview.md
    • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

    Information about the release notes entries format can be found in the documentation. Example:

    If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

dawedawe avatar May 12 '24 14:05 dawedawe

:heavy_exclamation_mark: Release notes required


:white_check_mark: Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.400.md

github-actions[bot] avatar May 12 '24 14:05 github-actions[bot]

Nice, I'm fine with having it shipped by default, it will need tests though (simple baseline which verifies couple of outputs).

And an approval from @KevinRansom

vzarytovskii avatar May 12 '24 18:05 vzarytovskii

From a usability perspective, is there any auto complete support for hash directive contents? If so, users could dot-into the expression they want docs for.

baronfel avatar May 12 '24 19:05 baronfel

Hey Dawe, I think this would be a great addition to FSI, worth spreading a word about it once it's in.

A note on errh clean code, would you want to move the new module to a separate file or something like this? fsi.fs is already very fat and seems like a rather easy target for extraction.

psfinaki avatar May 13 '24 14:05 psfinaki

Considerations:

  • would it be better done as a FSI extension through #r "fsih: List.map"?
  • for terminal output, it would be possible to pretty print a bit the output, using brighter coloring for headings such as "Description:", "Parameters:", etc. as well as bulleted items, it will improve the legibility of the output; or maybe in markdown format (the IDE can then figure out how to pretty print when handling the evaluation)
  • the code samples would also have syntax colouring (probably not semantic coloring that requires type checking, but whatever we could have), but this is probably more challenging item

Disregarding this, it already sound like nice feature that will speak to users of python REPL and other similar environments 🙂.

smoothdeveloper avatar May 13 '24 15:05 smoothdeveloper

Considerations:

  • would it be better done as a FSI extension through #r "fsih: List.map"?

It's not a dependency manager related functionality, so probably not(?). There's no such thing as "FSI extension", #r is a dependency manager functionality/extension. And if done via DM, it will be quite verbose and confusing.

  • for terminal output, it would be possible to pretty print a bit the output, using brighter coloring for headings such as "Description:", "Parameters:", etc. as well as bulleted items, it will improve the legibility of the output; or maybe in markdown format (the IDE can then figure out how to pretty print when handling the evaluation)

In this case, we will need additional testing for it for different $TERMs, and that it's not breaking the output of interactive and when used as API.

  • the code samples would also have syntax colouring (probably not semantic coloring that requires type checking, but whatever we could have), but this is probably more challenging item

Also would require much more testing with both non-terminal environments as well as all popular emulators and shells.

vzarytovskii avatar May 13 '24 15:05 vzarytovskii

From a usability perspective, is there any auto complete support for hash directive contents?

There is autocomplete in VS for filepaths inside the "…" after #I "…", #r "…", etc., in an .fsx file.

image

But I think I'm a bit thrown off by the #h "expr";; syntax myself.

What is the primary motivation for using a hash directive?

  1. For symmetry with #help?
  2. So that this feature itself shows up in #help (which currently only shows help for itself and other FSI directives)?
  3. To ensure that the user can't accidentally shadow the ability to access this functionality by defining their own value h?

(1) doesn't feel super-compelling to me, since all the other FSI directives, including #help, operate on the level of (i.e., refer to or affect) the FSI session, not on expressions within the session.

If (2), then I think we could just add the help for h to the information that #help shows. We could also add help for the existing fsi object while we were at it.

If (3), then what about just adding an h method to the existing fsi object, which is less likely to be accidentally shadowed than h on its own?

fsi.h List.map;;

brianrourkeboll avatar May 13 '24 17:05 brianrourkeboll

Hey Dawe, I think this would be a great addition to FSI, worth spreading a word about it once it's in.

A note on errh clean code, would you want to move the new module to a separate file or something like this? fsi.fs is already very fat and seems like a rather easy target for extraction.

Thanks :) Yeah, I thought about that but then decided to follow the existing all-in-one-file style of fsi for consistency. But sure, if y'all are okay with a dedicated file, I'll definitely split it off.

dawedawe avatar May 13 '24 21:05 dawedawe

What is the primary motivation for using a hash directive?

1. For symmetry with `#help`?

2. So that this feature itself shows up in `#help` (which currently only shows help for itself and other FSI directives)?

3. To ensure that the user can't accidentally shadow the ability to access this functionality by defining their own value `h`?

My motivation was an uniform distribution among all three points. Avoiding shadowing is a must-have for me.

You're making a strong argument for a new fsi.h function to provide the feature. But users should be able to discover it in the output of #help. So yeah, as long as the team is cool with it, that would also be my preferred way forward.

dawedawe avatar May 13 '24 21:05 dawedawe

#r is a dependency manager functionality/extension. And if done via DM, it will be quite verbose and confusing.

I concede it might sound unatural, but #r stands for "reference" (looking for "reference documentation"), and #r extensions can/do print to the output. It would need 0 parser changes.

@brianrourkeboll suggestion of fsi.h sounds much better in any case and very close to current fsih package.

I also prefer it to hash directive and quoting the expression.

Are there special events that FSI session will trigger?

Can such events be done from a type extension over FSharp.Compiler.Interactive.InteractiveSession?

This could help extending IDEs in terms of how the output is parsed / displayed (with the idea of markdown or pretty printing strategies that could be implemented on top, rather than raw parsing of stdout).

smoothdeveloper avatar May 13 '24 22:05 smoothdeveloper

Okay, I moved to fsi.h as an user interface and added a printer for the output processing. Meaning, the record option containing the various documentation pieces is available as it afterwards. image

The downside is, that the FSharp.Compiler.Interactive.Settings project (which hosts the fsi aka InteractiveSession) can't access the shim filesystem.

dawedawe avatar May 15 '24 21:05 dawedawe

I think this is in an okay state now but I don't know how to add tests for it. Is there some testing facility that can actually access the fsi object? I tried FSharpScript from ScriptHelpers.fs but it seems like it can only do hash directives and normal code, no member calls of fsi.

dawedawe avatar May 16 '24 18:05 dawedawe

Maybe just test the implementation directly with various Exprs? I'm not sure there's value in testing that you can call fsi.h directly, it's more important that the behavior of the expression-traversal be covered by tests.

baronfel avatar May 16 '24 18:05 baronfel

@vzarytovskii @KevinRansom Can we please have a review here ?

edgarfgp avatar May 23 '24 14:05 edgarfgp

Okay, we like the idea of this PR, but perhaps not the experience: A func on the session fsi object is not really ideal

Fsi already has a #help command which does something, and is probably the likeliest guess a naive user is going to make.

Microsoft (R) F# Interactive version 12.8.400.0 for F# 8.0
Copyright (c) Microsoft Corporation. All Rights Reserved.
For help type #help;;

> #help
- ;;

  F# Interactive directives:

    #r "file.dll";;                               // Reference (dynamically load) the given DLL
    #i "package source uri";;                     // Include package source uri when searching for packages
    #I "path";;                                   // Add the given search path for referenced DLLs
    #load "file.fs" ...;;                         // Load the given file(s) as if compiled and referenced
    #time ["on"|"off"];;                          // Toggle timing on/off
    #help;;                                       // Display help
    #r "nuget:FSharp.Data, 3.1.2";;               // Load Nuget Package 'FSharp.Data' version '3.1.2'
    #r "nuget:FSharp.Data";;                      // Load Nuget Package 'FSharp.Data' with the highest version
    #clear;;                                      // Clear screen
    #quit;;                                       // Exit

  F# Interactive command line options:

      See 'dotnet fsi --help' for options
>

extending this to also support: #help "System.Console.In";; should be trivial.

As observed, #help "System.Console.In";; with quotes is a bit irritating to this end we have proposed this PR: https://github.com/dotnet/fsharp/pull/17206 which will remove the necessity of quotes, allowing long idents to be retrieved.

So I believe we will be in favour of merging functionality similar to this with the above experience changes.

Thanks

Kevin

KevinRansom avatar Jun 03 '24 19:06 KevinRansom

Thanks for the reviews @psfinaki and @KevinRansom, once #17206 is merged, I'll rework this PR to use #help as Kevin suggested.

dawedawe avatar Jun 03 '24 22:06 dawedawe

@dawedawe, no need to wait mate. You can make it work with quotes, while we work the language process. When the other PR is merged it should either just work, or be a trivial fix.

KevinRansom avatar Jun 03 '24 22:06 KevinRansom

@KevinRansom Cool, so this is the state now: Screenshot from 2024-06-06 12-02-30

dawedawe avatar Jun 06 '24 10:06 dawedawe

/azp run

psfinaki avatar Jun 10 '24 14:06 psfinaki

Azure Pipelines successfully started running 2 pipeline(s).

azure-pipelines[bot] avatar Jun 10 '24 14:06 azure-pipelines[bot]

/azp run

psfinaki avatar Jun 10 '24 17:06 psfinaki

Azure Pipelines successfully started running 2 pipeline(s).

azure-pipelines[bot] avatar Jun 10 '24 17:06 azure-pipelines[bot]

/azp run

psfinaki avatar Jun 11 '24 09:06 psfinaki

Azure Pipelines successfully started running 2 pipeline(s).

azure-pipelines[bot] avatar Jun 11 '24 09:06 azure-pipelines[bot]

/azp run

psfinaki avatar Jun 12 '24 10:06 psfinaki

Azure Pipelines successfully started running 2 pipeline(s).

azure-pipelines[bot] avatar Jun 12 '24 10:06 azure-pipelines[bot]

Thanks for this.

KevinRansom avatar Jun 12 '24 17:06 KevinRansom