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

Fixes #17438 - Ensure that isinteractive multi-emit backing fields are not public #17439

Merged
merged 7 commits into from
Jul 30, 2024

Conversation

KevinRansom
Copy link
Member

@KevinRansom KevinRansom commented Jul 23, 2024

Fixes #17438 - Ensure that isinteractive multi-emit backing fields are not public

dotnet interactive multiemit+ previously made the backing fields for records, unions, E.t.c public, whereas multiemit-makes them internal.

Some poorly written serializers, include public fields specified with a CompilerGeneratedAttribute in the serialization stream.

This means that multiemit**-** has become a work around in scenarios that rely on fsi and serialization using these types of serializers.

This PR fixes the problem by correctly specifying fields as Private (actually internal). As implementation details they are also internal when compiled with realsig**+**

I believe the original reason for making the backing field public was so that fsi could reliably access it from multi-emit assemblies. Due to implementation issues there was previously a limit of 30 internal visible following assemblies. Now that it has been corrected, all multi-emit assemblies in a session have internals visible so, making the fields internal should work correctly.

Todo:

  • Implementation tests
  • There are some other spots that seem to special case multiemit, verify that they are no longer necessary and remove them if proved unnecessary

Background:
@Szer

#17308 (comment)

Could you verify that this fix works for your scenario ... I believe it does, but I don't really use ML.net

@KevinRansom KevinRansom requested a review from a team as a code owner July 23, 2024 23:26
Copy link
Contributor

github-actions bot commented Jul 23, 2024

❗ 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.100.md

@Szer
Copy link
Contributor

Szer commented Jul 24, 2024

I'll test it in 24 hours in all scenarios where it broke for me

@KevinRansom
Copy link
Member Author

@Szer, thanks mate, that would help.

@Szer
Copy link
Contributor

Szer commented Jul 24, 2024

checked with snippet below

Fails on Microsoft (R) F# Interactive version 12.8.301.0 for F# 8.0
Works on Microsoft (R) F# Interactive version 12.9.100.0 for F# 9.0 (built from this PR)

@KevinRansom

#r "nuget: Microsoft.ML, 3.0.1"

open Microsoft.ML
open Microsoft.ML.Data
open System.IO

[<CLIMutable>]
type SpamOrHam =
    { [<LoadColumn(0)>]
      text: string
      [<LoadColumn(1)>]
      spam: bool }

let content = """text,spam
abc,spam
123,ham
"""

let tempPath = Path.GetTempFileName()
File.WriteAllText(tempPath, content)

try
    MLContext().Data.LoadFromTextFile<SpamOrHam>(tempPath, separatorChar = ',', hasHeader = true)
finally
    File.Delete tempPath

@Szer
Copy link
Contributor

Szer commented Jul 24, 2024

Newtonsoft.Json snippet below works everywhere
That was fixed somewhere in .NET 6 with #13494
Tested it as well, just in case

#r "nuget: Newtonsoft.Json, 13.0.3"

open Newtonsoft.Json

type Foo = { id: int }

{ id = 1 }
|> JsonConvert.SerializeObject
|> fun x -> if x <> """{"id":1}""" then failwith x

@KevinRansom
Copy link
Member Author

@Szer , thanks this it really helps.
I am testing an update with a bit more cleanup code reduction which looks good. Still have to add some additional tests. and we will try to get this in.

Kevin

src/Compiler/Interactive/fsi.fs Show resolved Hide resolved
Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

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

Is it worth adding some sort of IL test for the interactive? I don't think I've been adding support of it to the testing framework though.

@KevinRansom
Copy link
Member Author

KevinRansom commented Jul 29, 2024

Is it worth adding some sort of IL test for the interactive? I don't think I've been adding support of it to the testing framework though.

I would like to, but we do it in memory. Generating IL would be a touch slower.

Perhaps in the future we can consider adding a --testonly:writetofile option to fsi to

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.

Ensure that isinteractive multi-emit backing fields are not public
5 participants