[WIP] Serialization and deserialization of the typechecking data #18162
Conversation
❗ Release notes requiredCaution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository:
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request. You can open this PR in browser to add release notes: open in github.dev
|
|
|
||
|
|
||
| [<Fact>] | ||
| let PickleAndUnpickleTcData() = |
There was a problem hiding this comment.
More generally, how to test this?
Comparing the original and restored TC information is hard currently since many types don't have equality allowed. Should we change this just for the purpose of testing? Doesn't feel right.
What else - create baselines for resources (bytes)?
Rely on the higher-level testing?
Hope & pray? (like we do with other pickling & unpickling stuff...)
There was a problem hiding this comment.
E2e testing of an integrated scenario.
Which can then compare e.g. IL baseline and separately compare debug info data, their equality is easier tested. Also, those should not be deviating anyway.
The same way pickling and unpickling is tested - it is not tested by hope & pray, it is tested by integration tests using referenced pickled version of F# code, such as the code in FSharp.Core and its signature and optimization data. (e.g. integration tests proving that Fsharp.Core calls are being correctly inlined thanks to optimization data)
|
|
||
| () | ||
|
|
||
| tcState, topAttrs, declaredImpls, tcEnvAtEndOfLastFile |
There was a problem hiding this comment.
So out of 4 things, tcState and tcEnvAtEndOfLastFile will be cumbersome (yet likely possible) for pickling/unpickling - it's a lot of information we haven't dealt with. Wonder if this can be somehow worked around?
| @@ -901,17 +949,17 @@ type TcAssemblyResolutions(tcConfig: TcConfig, results: AssemblyResolution list, | |||
|
|
|||
| for UnresolvedAssemblyReference (referenceText, _ranges) in unresolved do | |||
| if referenceText.Contains("mscorlib") then | |||
| Debug.Assert(false, sprintf "whoops, did not resolve mscorlib: '%s'%s" referenceText addedText) | |||
| //Debug.Assert(false, sprintf "whoops, did not resolve mscorlib: '%s'%s" referenceText addedText) | |||
There was a problem hiding this comment.
For some reason this breaks some test mockery, didn't investigate yet.
| @@ -5899,6 +5899,12 @@ type PickledCcuInfo = | |||
|
|
|||
| override _.ToString() = "PickledCcuInfo(...)" | |||
|
|
|||
| type PickledTcInfo = { | |||
| MainMethodAttrs: Attribs | |||
There was a problem hiding this comment.
Can't use TopAttribs as they are defined later in Checking
There was a problem hiding this comment.
BTW would a similar problem for things like TcEnv or TcState
| p_list p_pragma x st | ||
|
|
||
| and p_checked_impl_file (file: CheckedImplFile) st = | ||
| p_tup5 |
There was a problem hiding this comment.
So this doesn't pickle all the info yet, see the comment for unpickling.
| let WriteTypecheckingData (tcConfig: TcConfig, tcGlobals, fileName, inMem, ccu, tcInfo) = | ||
|
|
||
| // need to understand the naming and if we even want two resources here... | ||
| let rName = "FSharpTypecheckingData" |
There was a problem hiding this comment.
Why do we need 2 resources here?
Typechecking reuse should not be complicated with backwards compat, just support file format of the same compiler version only.
| let memA = byteReaderA () | ||
|
|
||
| let memB = | ||
| match byteReaderB with |
There was a problem hiding this comment.
What is byteReaderB good for here?
I think it should work without it.
UNLESS it is needed to reuse the most of existing pickling for signatures. Then of course let's have it for the sake of not reimplementing what has been done.
| qualifiedNameOfFile, | ||
| pragmas, | ||
| signature, | ||
| // ModuleOrNamespaceContents needs implementing, feels hard but doable |
There was a problem hiding this comment.
This is where the continuous progress should be happening.
Starting from most common types/expressions to the more rare ones.
| isScript, | ||
| // this anon record map can be likely easily built in top of primitives here | ||
| Unchecked.defaultof<_>, | ||
| // something about debug points, not sure we care here |
There was a problem hiding this comment.
We do care to have debugging working in VS, yes :)
There was a problem hiding this comment.
After all, this whole story is to speed up the inner dev loop, which is happening in DEBUG mode normally.
| @@ -0,0 +1,166 @@ | |||
| module FSharp.Compiler.Service.Tests.TypedTreePickleTests | |||
There was a problem hiding this comment.
I see this as an exploratory tool.
If the build times of the test project make this exploration slower, I believe (?) the same should be doable as an .fsx as well - referencing the compiler and experimenting there.
(just a proposal - this can temporarily live in the test project as well, but maybe adhocly playing with the inner details like here would have a faster feedback loop when in a script file, scratchpad.fsx)
psfinaki
changed the base branch from
main
to
feature/reuse-typechecking-results
psfinaki
force-pushed
the
reuse-typecheck-d
branch
from
e13c345 to
cb8fa3e
Compare
Wonder if this even counts as a prototype? But at least some steps towards that direction :)
This will be needed for the Stage 2 of the reuse typechecking results feature, as per the design doc here.
See the comments in code and in the PR.