Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add FSharp.Compiler.Scripting #7437

Merged
merged 4 commits into from Aug 23, 2019
Merged

add FSharp.Compiler.Scripting #7437

merged 4 commits into from Aug 23, 2019

Conversation

brettfo
Copy link
Member

@brettfo brettfo commented Aug 22, 2019

Adds a simple wrapper around the same engine that powers fsi. The reasoning behind this is to make other interactive-like experiences (e.g., those in dotnet/try) easier to integrate with.

The bulk of the PR is in the FSharp.Compiler.Scripting.FSharpScript class, with the Eval() method being the interesting one. Essentially it's currently a thin wrapper around an existing method in FSharp.Compiler.Private.dll with some optional special handling of stdin/stdout. The return type of Eval() is also significantly simpler, Result<FsiValue, Exception> * FSharpErrorInfo[].

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

This looks OK if it's what you need and I'm fine with you engineering things how you see fit.

FsiEvaluationSession is sort of meant to be the one-shop public API containing everything needed for scripting but in practice there is enough cruft and complexity there that people do seem to like to write simpler wrappers both simplifying things (reducing scope of functionality) and mixing in things like in/out streams. Another example is Yaaf scripting https://github.com/matthid/Yaaf.FSharp.Scripting

open System.IO
open System.Text

type internal CapturedTextReader() =
Copy link
Contributor

Choose a reason for hiding this comment

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

How do these relate to

type CompilerInputStream() =
?

I'm fine with throwing away the old implementations or duplication - just wondering

Copy link
Member Author

Choose a reason for hiding this comment

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

The answer that I should give:
I didn't want to strip CompilerInputStream out of fsi.fs and put it somewhere that this new assembly could consume it, especially since it's really just an internal implementation detail.

The real answer:
I never even looked so I didn't know it existed.

I don't care too much either way; leaving as is, or moving that type to compiler.private.

@dsyme
Copy link
Contributor

dsyme commented Aug 22, 2019

@brettfo What's your design vision here for the other functionality exposed by FsiEvaluationSession , e.g. to parse and check script fragments in the evaluation context, or other such things? As I mentioned above I don't really mind - it makes sense to make a simpler wrapper hiding some of the complexity - just wondering about your thinking really

@brettfo
Copy link
Member Author

brettfo commented Aug 22, 2019

@dsyme The APIs (and assembly) I've introduced will certainly change, but the vision is that eventually (don't know when) there might be some common IInteractiveProvider interface that both F# and C# can implement and that'll be what's consumed by the Jupyter kernel work in the dotnet/try repo, and possibly beyond. Since the contract of that interface doesn't yet exist, we're currently seeing what existing C#/F# scripting APIs there are and how we might want to change them. As such, this PR is fulfilling 2 purposes: (1) provide a simple Eval() method without having to think too hard about what's actually happening, and (2) provide a package created and signed by us that we can directly consume (currently via MyGet). Otherwise, the option is to consume FSharp.Compiler.Server, but that's not published by us and might be delayed in any internal API changes we might need to make. It also seems wrong to add a dependency to FSharp.Compiler.Server to wherever we eventually consume IInteractiveProvider from; I'd like to keep the external dependencies limited to FSharp.Compiler.Scripting.

All of that being said, I'm absolutely open to persuasion another way if necessary.

@baronfel
Copy link
Member

We've talked for a while in the community about folding back in publishing of FCS into this repo, as long as there was a way that the community could also have a hand in publishing updates out-of-band with Microsoft timings and strategy. I'd love to help make that happen in whatever way I can, and in fact I've been intending to reverse-integrate FCS into this repository for some time now, seeing as it should just be primarily packaging changes.

If there are use cases that would make sense surfacing in an FCS-like package to make F# scripting more embeddable, I think it makes sense to consider this path + an evolution of the existing FCS APIs. Of course this is coming from someone who doesn't work in the space 24/7, just contributes to FCS and FSAC at times.

@brettfo
Copy link
Member Author

brettfo commented Aug 22, 2019

Adding @cartermp to the conversation about folding FCS publishing back into this repo. We've discussed it a bunch in the office, but where he sits 5 feet away from me, nobody else gets to benefit from those discussions, so hopefully we can hash out some of the points in public.

@baronfel
Copy link
Member

Link to the tooling RFC @cartermp wrote a few months ago.

@brettfo
Copy link
Member Author

brettfo commented Aug 22, 2019

@dsyme Can you take a peek at commit 6746a3b 61b1a06 (sorry for the edit, had to fix a test), specifically fsi.fs and fsi.fsi? I don't think it's an issue, but I wanted to confirm I'm OK modifying EvalInteractionNonThrowing to not discard the return value.

@dsyme
Copy link
Contributor

dsyme commented Aug 23, 2019

Cool, thanks @brettfo. It looks like this is experimental/in-progress for the moment and eventually we should probably have a design doc here. I can see you're at the stage of prototyping the API outside of FCS/FCP. That's ok.

Eventually this should be a public component (like FCS). Since it is currently private, please put Private in the name, FSharp.Compiler.Private.Scripting?

In terms of components - in the long term just merge this into FSharp.Compiler.Service.dll. We don't try to componentize FCS or FCP, and I don't think there is yet enough here to justify making a new public component. (From the look of the code it belongs in FCS.dll as much as FsiEvaluationSession does - if we were going to componentize we would slice at a different point).

@dsyme
Copy link
Contributor

dsyme commented Aug 23, 2019

@dsyme Can you take a peek at commit 6746a3b 61b1a06 (sorry for the edit, had to fix a test), specifically fsi.fs and fsi.fsi? I don't think it's an issue, but I wanted to confirm I'm OK modifying EvalInteractionNonThrowing to not discard the return value.

Yes, this is ok.

@cartermp
Copy link
Contributor

If this acts as an impetus to get going with publishing FCS from here, let's do it like shia lebouf

Also remove publishing steps from official build; it will be done elsewhere.
@brettfo
Copy link
Member Author

brettfo commented Aug 23, 2019

Renamed FSharp.Compiler.Scripting to FSharp.Compiler.Private.Scripting as per @dsyme's request.

I've left the separate FSharp.Compiler.Private.Scripting.fsproj project as an easy internal publishing point until we've finalized the work to publish FCS. At that point I'll merge Private.Scripting into Compiler.Private.

@brettfo brettfo merged commit 503c391 into dotnet:master Aug 23, 2019
@brettfo brettfo deleted the scripting branch August 23, 2019 22:37
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* add FSharp.Compiler.Scripting

* allow evaluation of non-expressions

* rename FSharp.Compiler.Scripting to FSharp.Compiler.Private.Scripting

* remove unnecessary project reference
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.

None yet

4 participants