From 727147aad87463e17de509e47370c6cbc10ed0d6 Mon Sep 17 00:00:00 2001 From: "Kevin Ransom (msft)" Date: Mon, 1 Jun 2020 21:24:51 -0700 Subject: [PATCH] The walking dead version of Fixes #9217 (#9363) * Fix #9217 -- Nuget #r references do not resolve assemblies referenced through the primary one * Regression test * Feedback * triple quote --- src/fsharp/CompileOps.fs | 101 ++++++++++++------ src/fsharp/TypedTree.fs | 25 +---- src/fsharp/TypedTreePickle.fs | 8 +- .../FSharpScriptTests.fs | 24 +++++ 4 files changed, 104 insertions(+), 54 deletions(-) diff --git a/src/fsharp/CompileOps.fs b/src/fsharp/CompileOps.fs index 50a8c9481ec..ce9b868666f 100644 --- a/src/fsharp/CompileOps.fs +++ b/src/fsharp/CompileOps.fs @@ -4,6 +4,7 @@ module internal FSharp.Compiler.CompileOps open System +open System.Collections.Concurrent open System.Collections.Generic open System.Diagnostics open System.IO @@ -3938,6 +3939,13 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse let mutable dllTable: NameMap = NameMap.empty let mutable ccuInfos: ImportedAssembly list = [] let mutable ccuTable: NameMap = NameMap.empty + + /// ccuThunks is a ConcurrentDictionary thus threadsafe + /// the key is a ccuThunk object, the value is a (unit->unit) func that when executed + /// the func is used to fix up the func and operates on data captured at the time the func is created. + /// func() is captured during phase2() of RegisterAndPrepareToImportReferencedDll(..) and PrepareToImportReferencedFSharpAssembly ( .. ) + let mutable ccuThunks = new ConcurrentDictionary unit)>() + let disposeActions = ResizeArray() let mutable disposed = false let mutable ilGlobalsOpt = ilGlobalsOpt @@ -3949,7 +3957,7 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse #endif let disposal = new TcImportsSafeDisposal(disposeActions, disposeTypeProviderActions, compilationThread) - + let CheckDisposed() = if disposed then assert false @@ -3957,6 +3965,25 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse CheckDisposed() (disposal :> IDisposable).Dispose() + // This is used to fixe up unresolved ccuThunks that were created during assembly import. + // the ccuThunks dictionary is a ConcurrentDictionary and thus threadsafe. + // Algorithm: + // Get a snapshot of the current unFixedUp ccuThunks. + // for each of those thunks, remove them from the dictionary, so any parallel threads can't do this work + // If it successfully removed it from the dictionary then do the fixup + // If the thunk remains unresolved add it back to the ccuThunks dictionary for further processing + // If not then move on to the next thunk + let fixupOrphanCcus () = + let keys = ccuThunks.Keys + for ccuThunk in keys do + match ccuThunks.TryRemove(ccuThunk) with + | true, func -> + if ccuThunk.IsUnresolvedReference then + func() + if ccuThunk.IsUnresolvedReference then + ccuThunks.TryAdd(ccuThunk, func) |> ignore + | _ -> () + static let ccuHasType (ccu: CcuThunk) (nsname: string list) (tname: string) = let matchNameSpace (entityOpt: Entity option) n = match entityOpt with @@ -3988,13 +4015,13 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse CheckDisposed() tcImportsWeak #endif - + member tcImports.RegisterCcu ccuInfo = CheckDisposed() ccuInfos <- ccuInfos ++ ccuInfo // Assembly Ref Resolution: remove this use of ccu.AssemblyName ccuTable <- NameMap.add (ccuInfo.FSharpViewOfMetadata.AssemblyName) ccuInfo ccuTable - + member tcImports.RegisterDll dllInfo = CheckDisposed() dllInfos <- dllInfos ++ dllInfo @@ -4037,24 +4064,24 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse | Some res -> res | None -> error(Error(FSComp.SR.buildCouldNotResolveAssembly assemblyName, m)) - member tcImports.GetImportedAssemblies() = + member tcImports.GetImportedAssemblies() = CheckDisposed() - match importsBase with + match importsBase with | Some importsBase-> List.append (importsBase.GetImportedAssemblies()) ccuInfos - | None -> ccuInfos - - member tcImports.GetCcusExcludingBase() = + | None -> ccuInfos + + member tcImports.GetCcusExcludingBase() = CheckDisposed() - ccuInfos |> List.map (fun x -> x.FSharpViewOfMetadata) + ccuInfos |> List.map (fun x -> x.FSharpViewOfMetadata) - member tcImports.GetCcusInDeclOrder() = + member tcImports.GetCcusInDeclOrder() = CheckDisposed() List.map (fun x -> x.FSharpViewOfMetadata) (tcImports.GetImportedAssemblies()) - + // This is the main "assembly reference --> assembly" resolution routine. - member tcImports.FindCcuInfo (ctok, m, assemblyName, lookupOnly) = + member tcImports.FindCcuInfo (ctok, m, assemblyName, lookupOnly) = CheckDisposed() - let rec look (t: TcImports) = + let rec look (t: TcImports) = match NameMap.tryFind assemblyName t.CcuTable with | Some res -> Some res | None -> @@ -4069,9 +4096,8 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse match look tcImports with | Some res -> ResolvedImportedAssembly res | None -> UnresolvedImportedAssembly assemblyName - - member tcImports.FindCcu (ctok, m, assemblyName, lookupOnly) = + member tcImports.FindCcu (ctok, m, assemblyName, lookupOnly) = CheckDisposed() match tcImports.FindCcuInfo(ctok, m, assemblyName, lookupOnly) with | ResolvedImportedAssembly importedAssembly -> ResolvedCcu(importedAssembly.FSharpViewOfMetadata) @@ -4509,7 +4535,7 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse #endif FSharpOptimizationData = notlazy None } tcImports.RegisterCcu ccuinfo - let phase2 () = + let phase2 () = #if !NO_EXTENSIONTYPING ccuinfo.TypeProviders <- tcImports.ImportTypeProviderExtensions (ctok, tcConfig, filename, ilScopeRef, ilModule.ManifestOfAssembly.CustomAttrs.AsList, ccu.Contents, invalidateCcu, m) #endif @@ -4569,11 +4595,17 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse | None -> if verbose then dprintf "*** no optimization data for CCU %s, was DLL compiled with --no-optimization-data??\n" ccuName None - | Some info -> + | Some info -> let data = GetOptimizationData (filename, ilScopeRef, ilModule.TryGetILModuleDef(), info) - let res = data.OptionalFixup(fun nm -> availableToOptionalCcu(tcImports.FindCcu(ctok, m, nm, lookupOnly=false))) - if verbose then dprintf "found optimization data for CCU %s\n" ccuName - Some res) + let fixupThunk () = data.OptionalFixup(fun nm -> availableToOptionalCcu(tcImports.FindCcu(ctok, m, nm, lookupOnly=false))) + + // Make a note of all ccuThunks that may still need to be fixed up when other dlls are loaded + for ccuThunk in data.FixupThunks do + if ccuThunk.IsUnresolvedReference then + ccuThunks.TryAdd(ccuThunk, fun () -> fixupThunk () |> ignore) |> ignore + + if verbose then dprintf "found optimization data for CCU %s\n" ccuName + Some (fixupThunk ())) let ilg = defaultArg ilGlobalsOpt EcmaMscorlibILGlobals @@ -4599,19 +4631,25 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse () #endif data, ccuinfo, phase2) - + // Register all before relinking to cope with mutually-referential ccus ccuRawDataAndInfos |> List.iter (p23 >> tcImports.RegisterCcu) - let phase2 () = + let phase2 () = (* Relink *) (* dprintf "Phase2: %s\n" filename; REMOVE DIAGNOSTICS *) - ccuRawDataAndInfos |> List.iter (fun (data, _, _) -> data.OptionalFixup(fun nm -> availableToOptionalCcu(tcImports.FindCcu(ctok, m, nm, lookupOnly=false))) |> ignore) + ccuRawDataAndInfos + |> List.iter (fun (data, _, _) -> + let fixupThunk () = data.OptionalFixup(fun nm -> availableToOptionalCcu(tcImports.FindCcu(ctok, m, nm, lookupOnly=false))) |> ignore + fixupThunk() + for ccuThunk in data.FixupThunks do + if ccuThunk.IsUnresolvedReference then + ccuThunks.TryAdd(ccuThunk, fixupThunk) |> ignore + ) #if !NO_EXTENSIONTYPING ccuRawDataAndInfos |> List.iter (fun (_, _, phase2) -> phase2()) #endif - ccuRawDataAndInfos |> List.map p23 |> List.map ResolvedImportedAssembly + ccuRawDataAndInfos |> List.map p23 |> List.map ResolvedImportedAssembly phase2 - // NOTE: When used in the Language Service this can cause the transitive checking of projects. Hence it must be cancellable. member tcImports.RegisterAndPrepareToImportReferencedDll (ctok, r: AssemblyResolution) : Cancellable<_ * (unit -> AvailableImportedAssembly list)> = @@ -4653,16 +4691,16 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse ILAssemblyRefs = assemblyData.ILAssemblyRefs } tcImports.RegisterDll dllinfo let ilg = defaultArg ilGlobalsOpt EcmaMscorlibILGlobals - let phase2 = + let phase2 = if assemblyData.HasAnyFSharpSignatureDataAttribute then if not (assemblyData.HasMatchingFSharpSignatureDataAttribute ilg) then - errorR(Error(FSComp.SR.buildDifferentVersionMustRecompile filename, m)) - tcImports.PrepareToImportReferencedILAssembly (ctok, m, filename, dllinfo) + errorR(Error(FSComp.SR.buildDifferentVersionMustRecompile filename, m)) + tcImports.PrepareToImportReferencedILAssembly (ctok, m, filename, dllinfo) else - try + try tcImports.PrepareToImportReferencedFSharpAssembly (ctok, m, filename, dllinfo) - with e -> error(Error(FSComp.SR.buildErrorOpeningBinaryFile(filename, e.Message), m)) - else + with e -> error(Error(FSComp.SR.buildErrorOpeningBinaryFile(filename, e.Message), m)) + else tcImports.PrepareToImportReferencedILAssembly (ctok, m, filename, dllinfo) return dllinfo, phase2 } @@ -4683,6 +4721,7 @@ and [] TcImports(tcConfigP: TcConfigProvider, initialResolutions: TcAsse }) let dllinfos, phase2s = results |> List.choose id |> List.unzip + fixupOrphanCcus() let ccuinfos = (List.collect (fun phase2 -> phase2()) phase2s) return dllinfos, ccuinfos } diff --git a/src/fsharp/TypedTree.fs b/src/fsharp/TypedTree.fs index 8b12eb84295..59a370cb659 100644 --- a/src/fsharp/TypedTree.fs +++ b/src/fsharp/TypedTree.fs @@ -5021,28 +5021,21 @@ type CcuReference = string // ILAssemblyRef /// reference that has not had an appropriate fixup applied. [] type CcuThunk = - { + /// ccu.target is null when a reference is missing in the transitive closure of static references that + /// may potentially be required for the metadata of referenced DLLs. mutable target: CcuData - - /// ccu.orphanfixup is true when a reference is missing in the transitive closure of static references that - /// may potentially be required for the metadata of referenced DLLs. It is set to true if the "loader" - /// used in the F# metadata-deserializer or the .NET metadata reader returns a failing value (e.g. None). - /// Note: When used from Visual Studio, the loader will not automatically chase down transitively referenced DLLs - they - /// must be in the explicit references in the project. - mutable orphanfixup: bool - name: CcuReference } /// Dereference the asssembly reference member ccu.Deref = - if isNull (ccu.target :> obj) || ccu.orphanfixup then + if isNull (ccu.target :> obj) then raise(UnresolvedReferenceNoRange ccu.name) ccu.target /// Indicates if this assembly reference is unresolved - member ccu.IsUnresolvedReference = isNull (ccu.target :> obj) || ccu.orphanfixup + member ccu.IsUnresolvedReference = isNull (ccu.target :> obj) /// Ensure the ccu is derefable in advance. Supply a path to attach to any resulting error message. member ccu.EnsureDerefable(requiringPath: string[]) = @@ -5104,13 +5097,11 @@ type CcuThunk = /// Create a CCU with the given name and contents static member Create(nm, x) = { target = x - orphanfixup = false name = nm } /// Create a CCU with the given name but where the contents have not yet been specified static member CreateDelayed nm = { target = Unchecked.defaultof<_> - orphanfixup = false name = nm } /// Fixup a CCU to have the given contents @@ -5128,13 +5119,7 @@ type CcuThunk = match box avail.target with | null -> error(Failure("internal error: ccu thunk '"+avail.name+"' not fixed up!")) | _ -> avail.target - - /// Fixup a CCU to record it as "orphaned", i.e. not available - member x.FixupOrphaned() = - match box x.target with - | null -> x.orphanfixup<-true - | _ -> errorR(Failure("internal error: FixupOrphaned: the ccu thunk for assembly "+x.AssemblyName+" not delayed!")) - + /// Try to resolve a path into the CCU by referencing the .NET/CLI type forwarder table of the CCU member ccu.TryForward(nlpath: string[], item: string) : EntityRef option = ccu.EnsureDerefable nlpath diff --git a/src/fsharp/TypedTreePickle.fs b/src/fsharp/TypedTreePickle.fs index 94691a9fb63..0ceb9d01ec2 100644 --- a/src/fsharp/TypedTreePickle.fs +++ b/src/fsharp/TypedTreePickle.fs @@ -50,9 +50,11 @@ type PickledDataWithReferences<'rawData> = member x.OptionalFixup loader = x.FixupThunks |> Array.iter(fun reqd-> - match loader reqd.AssemblyName with - | Some loaded -> reqd.Fixup loaded - | None -> reqd.FixupOrphaned() ) + // Only fixup what needs fixing up + if reqd.IsUnresolvedReference then + match loader reqd.AssemblyName with + | Some loaded -> reqd.Fixup loaded + | _ -> () ) x.RawData //--------------------------------------------------------------------------- diff --git a/tests/FSharp.Compiler.Private.Scripting.UnitTests/FSharpScriptTests.fs b/tests/FSharp.Compiler.Private.Scripting.UnitTests/FSharpScriptTests.fs index 6654b46649b..386f9de9ecd 100644 --- a/tests/FSharp.Compiler.Private.Scripting.UnitTests/FSharpScriptTests.fs +++ b/tests/FSharp.Compiler.Private.Scripting.UnitTests/FSharpScriptTests.fs @@ -302,3 +302,27 @@ let x = " script.Eval(code) |> ignoreValue Assert.False(foundInner) + + [] + member _.``Script with nuget package that yields out of order dependencies works correctly``() = + // regression test for: https://github.com/dotnet/fsharp/issues/9217 + + let code = """ +#r "nuget: FParsec,1.1.1" + +open FParsec + +let test p str = + match run p str with + | Success(result, _, _) -> + printfn "Success: %A" result + true + | Failure(errorMsg, _, _) -> + printfn "Failure: %s" errorMsg + false +test pfloat "1.234" +""" + use script = new FSharpScript(additionalArgs=[|"/langversion:preview"|]) + let opt = script.Eval(code) |> getValue + let value = opt.Value + Assert.AreEqual(true, value.ReflectionValue :?> bool)