Skip to content

Conversation

@goswinr
Copy link
Contributor

@goswinr goswinr commented Jan 8, 2023

Currently, compiler errors in fsi evaluations via FCS always report an error in a file called input.fsx.
This pull request adds an optional parameter overload that allows specifying a filename related to an evaluated string.

For example, calling fsiSession.EvalInteraction( "1 + 1.0" ) reports input.fsx (1,3)-(1,4) typecheck error The type 'float' does not match the type 'int'
But the actual file where this string comes from might be named differently.

I don't want to use fsiSession.EvalScript(path) since a file might not exist yet, or I might not be evaluating all lines of the current file.

@goswinr goswinr requested a review from a team as a code owner January 8, 2023 15:26
@T-Gro
Copy link
Member

T-Gro commented Jan 10, 2023

I know it probably isn't a big deal, but I would prefer if this would be done without breaking binary compatibility.
i.e. adding a new overload, and keeping the existing one WITHOUT requiring consuming code to be recompiled.

(in this case the code overhead is negligable)

@goswinr
Copy link
Contributor Author

goswinr commented Jan 10, 2023

@T-Gro sure , I can change the PR. I was not sure how to best expose the dummyScriptFileName . So instead of two optional members there should be and overload with two required members ( and the cancellation token still optional) ?

@T-Gro
Copy link
Member

T-Gro commented Jan 12, 2023

@T-Gro sure , I can change the PR. I was not sure how to best expose the dummyScriptFileName . So instead of two optional members there should be and overload with two required members ( and the cancellation token still optional) ?

Yep.

Because adding an optional member to existing functionality requires recompilation (with no code changes on consumer side), but is binary breaking.

A new overload would keep existing usages working.

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

As Tomas says, overloads are preferred for compatibility.

@goswinr goswinr force-pushed the inputFileName branch 2 times, most recently from ef9bed6 to 909b6f5 Compare January 14, 2023 14:09
@goswinr
Copy link
Contributor Author

goswinr commented Jan 14, 2023

@T-Gro , @KevinRansom I changed it to overloads. I also rebased it to use the new basline files for the API surface.

the new overloads are:

member EvalInteraction: code: string * scriptFileName: string * ?cancellationToken: CancellationToken -> unit
member EvalInteractionNonThrowing: code: string * scriptFileName: string * ?cancellationToken: CancellationToken ->  Choice<FsiValue option, exn> * FSharpDiagnostic[]
member EvalExpression: code: string * scriptFileName: string -> FsiValue option
member EvalExpressionNonThrowing: code: string * scriptFileName: string -> Choice<FsiValue option, exn> * FSharpDiagnostic[]

the existing ones:

member EvalInteraction: code: string *  ?cancellationToken: CancellationToken -> unit
member EvalInteractionNonThrowing: code: string *  ?cancellationToken: CancellationToken ->  Choice<FsiValue option, exn> * FSharpDiagnostic[]
member EvalExpression: code: string  -> FsiValue option
member EvalExpressionNonThrowing: code: string  -> Choice<FsiValue option, exn> * FSharpDiagnostic[]

In my own tests the scriptFileName now also shows for __SOURCE_FILE__ in scripts.

@vzarytovskii vzarytovskii enabled auto-merge (squash) January 19, 2023 13:47
@goswinr goswinr requested a review from KevinRansom January 25, 2023 13:18
@vzarytovskii vzarytovskii merged commit 376466c into dotnet:main Feb 1, 2023
@goswinr
Copy link
Contributor Author

goswinr commented Feb 1, 2023

Thanks @vzarytovskii ! Is there also some fcs documentation to update ?

@vzarytovskii
Copy link
Member

Thanks @vzarytovskii ! Is there also some fcs documentation to update ?

I don't think there's, docs should be automatically generated

@goswinr goswinr deleted the inputFileName branch February 2, 2023 11:23
@goswinr
Copy link
Contributor Author

goswinr commented Feb 15, 2023

Thanks for merging @vzarytovskii ! However, I did not find this in the new Nuget that was published yesterday.
Is there a preview nuget that contains this change?
image

@vzarytovskii
Copy link
Member

Thanks for merging @vzarytovskii ! However, I did not find this in the new Nuget that was published yesterday.
Is there a preview nuget that contains this change?
image

I don't think it made it to .200, it should be in the next .300 preview or our preview nuget feed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants