Skip to content

Conversation

@majocha
Copy link
Contributor

@majocha majocha commented Apr 3, 2025

Fixes #18441

The idea is to make each FsiEvaluationSession produce uniquely named assemblies.
That way there is no possibility of confusion when different sessions resolve assemblies.

The first session creates an assembly named FSI-ASSEMBLY to keep things reasonably backwards compatible.
Subsequent sessions get names FSI-ASSEMBLY2, FSI-ASSEMBLY3 and so on.

To discover the name associated with the current session you can call
Assembly.GetExecutingAssembly().FullName from fsi.

TODO:

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

❗ Release notes required


✅ Found changes and release notes in following paths:

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

@majocha majocha changed the title Unique names for fsi generated assemblies Uniquely name generated assemblies for each fsi session Apr 3, 2025
@majocha
Copy link
Contributor Author

majocha commented Apr 3, 2025

Open question is: does any tooling expect the generated assembly name to be exactly "FSI-ASSEMBLY"? Will it break anything?

@baronfel
Copy link
Member

baronfel commented Apr 3, 2025

There's definitely code out there relying on it to some degree: https://github.com/search?q=%22FSI-ASSEMBLY%22&type=code

I know that Ionide watcher is multi-emit aware and so just looks for assemblies that start with that name, but some kind of tooling surface to determine if an Assembly is an FSI dynamic assembly would sidestep all of these hacks.

@majocha
Copy link
Contributor Author

majocha commented Apr 3, 2025

Yeah, current situation is that multi-emit assemblies are all named "FSI-ASSEMBLY" and differ by version number only.
It is possible to create many fsi sessions. This suggests that they are isolated and can be used concurrently, which is not fully the case.
It would be great to sort it out somehow.

@majocha
Copy link
Contributor Author

majocha commented Apr 3, 2025

I think it is possible to expose FsiEvaluationSession.DynamicAssemblies getter on the fsi object, to make it accessible also "from the inside" of a running session.

Unfortunately it seems fsi object is a singleton shared between all created sessions. That complicates things.

@majocha
Copy link
Contributor Author

majocha commented Apr 3, 2025

This is a rabbit hole already, but somewhat related:
It seems pdb generation for dynamic assemblies is back in net9.0
dotnet/runtime#99935
dotnet/runtime#100706

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

@majocha majocha marked this pull request as ready for review April 5, 2025 15:00
@majocha majocha requested a review from a team as a code owner April 5, 2025 15:00
@T-Gro T-Gro self-assigned this Apr 7, 2025
@T-Gro
Copy link
Member

T-Gro commented Apr 8, 2025

Can we imagine a scenario where this is a breaking change for users of fsharp scripts via FSharp.Compiler.Service.dll ?

@majocha
Copy link
Contributor Author

majocha commented Apr 8, 2025

Yes, if a user creates a FsiEvaluationSession, disposes it, and then creates another, the name of the assembly will be FSI-ASSEMBLY2.

@majocha
Copy link
Contributor Author

majocha commented Apr 8, 2025

Current situation is:
Some old code on Github can be found that assumes the name to be FSI-ASSEMBLY.
New stuff, like Ionide watcher expects the name to start with FSI-ASSEMBLY, adhering to current docs.
The docs are outdated, because currently multi-emit produces assemblies all named FSI-ASSEMBLY, differentiated by version.
This PR would restore the situation where many "FSI-ASSEMBLYn" can exsist, although with changed semantics, because different names mean different sessions.

Now I wonder if better fix wouldn't be to just never resolve by simple name, even in case of single dynamic assembly.
All the code in the wild just enumerates loaded assemblies and tries to find "FSI-ASSEMBLY". Nobody does Assembly.Load("FSI-ASSEMBLY").

@majocha majocha marked this pull request as draft April 8, 2025 20:58
@majocha majocha mentioned this pull request Apr 9, 2025
@majocha
Copy link
Contributor Author

majocha commented Apr 10, 2025

Closing in favor of #18465

@majocha majocha closed this Apr 10, 2025
@KevinRansom
Copy link
Contributor

KevinRansom commented Apr 10, 2025

Reflection with the name "FSI-ASSEMBLY" is not any sort of contract, so if it breaks someone, they have the deep joy of fixing it.

The prior multi-emit implementation created assemblies with distinct names for each submission, the current multi-emit creates them with distinct version numbers.

The comment for the PR, is that it needs more testing, I do want to consider the behaviour of multiple sessions. multi-emit+ treats each subsequent session similarly to a submission, in as much as it changes the version number and carries on regardless. This is probably not what a developer expects, but I think it should be a separate issue.

For developers wanting to access the current session, we should probably add something to the fsi object that gives access to it. Yet another separate issue.

@majocha
Copy link
Contributor Author

majocha commented Apr 10, 2025

The comment for the PR, is that it needs more testing, I do want to consider the behaviour of multiple sessions. multi-emit+ treats each subsequent session similarly to a submission, in as much as it changes the version number and carries on regardless. This is probably not what a developer expects, but I think it should be a separate issue.

Does it mean the IVT mechanism do not isolate sessions, i.e. previous submissions are visible to every other newer submission, regardless of session?

@majocha
Copy link
Contributor Author

majocha commented Apr 11, 2025

Does it mean the IVT mechanism do not isolate sessions, i.e. previous submissions are visible to every other newer submission, regardless of session?

    [<Fact>]
    let ``Internals from one session visible in next session``() =
        let args = [| "--multiemit+" |]

        use sessionOne = new FSharpScript(additionalArgs=args)
        sessionOne.Eval("""let x = 42""") |> ignore

        use sessionTwo = new FSharpScript(additionalArgs=args)
        let result = sessionTwo.Eval("""x""") |> getValue
        printfn "Result: %A" result
        Assert.Equal("42", string result.Value.ReflectionValue)

The test says the answer to my question is no. Although I still don't understand how it works :)

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

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

FSI multi-emit unstable

4 participants