dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.88k stars 783 forks source link

port fsih to fsi as a hash directive #17140

Closed dawedawe closed 3 months ago

dawedawe commented 4 months ago

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.

I want to mention that this PR was made possible by the supporters of the Amplifying FSharp Open Collective.

Checklist

github-actions[bot] commented 4 months ago

: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
vzarytovskii commented 4 months ago

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

baronfel commented 4 months ago

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.

psfinaki commented 4 months ago

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.

smoothdeveloper commented 4 months ago

Considerations:

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

vzarytovskii commented 4 months ago

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.

brianrourkeboll commented 4 months ago

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;;
dawedawe commented 4 months ago

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 commented 4 months ago

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.

smoothdeveloper commented 4 months ago

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).

dawedawe commented 4 months ago

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 commented 4 months ago

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.

baronfel commented 4 months ago

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.

edgarfgp commented 4 months ago

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

KevinRansom commented 4 months ago

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

dawedawe commented 4 months ago

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

KevinRansom commented 4 months ago

@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.

dawedawe commented 4 months ago

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

psfinaki commented 3 months ago

/azp run

azure-pipelines[bot] commented 3 months ago
Azure Pipelines successfully started running 2 pipeline(s).
psfinaki commented 3 months ago

/azp run

azure-pipelines[bot] commented 3 months ago
Azure Pipelines successfully started running 2 pipeline(s).
psfinaki commented 3 months ago

/azp run

azure-pipelines[bot] commented 3 months ago
Azure Pipelines successfully started running 2 pipeline(s).
psfinaki commented 3 months ago

/azp run

azure-pipelines[bot] commented 3 months ago
Azure Pipelines successfully started running 2 pipeline(s).
KevinRansom commented 3 months ago

Thanks for this.