From 402710e9102d1ea4ef060b75ccec045a36609bad Mon Sep 17 00:00:00 2001 From: nojaf Date: Wed, 7 Jun 2023 13:45:38 +0200 Subject: [PATCH 1/2] Add unit tests to illustrate the problem. --- .../FSharp.Compiler.ComponentTests.fsproj | 1 + .../Signatures/MissingDiagnostic.fs | 54 +++++++++++++++++++ .../TypeChecks/TyparNameTests.fs | 2 +- tests/FSharp.Test.Utilities/Compiler.fs | 4 +- tests/FSharp.Test.Utilities/CompilerAssert.fs | 4 +- 5 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 tests/FSharp.Compiler.ComponentTests/Signatures/MissingDiagnostic.fs diff --git a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj index 82efbccafab..f094c2e6e04 100644 --- a/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj +++ b/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj @@ -250,6 +250,7 @@ + diff --git a/tests/FSharp.Compiler.ComponentTests/Signatures/MissingDiagnostic.fs b/tests/FSharp.Compiler.ComponentTests/Signatures/MissingDiagnostic.fs new file mode 100644 index 00000000000..2d263fa728f --- /dev/null +++ b/tests/FSharp.Compiler.ComponentTests/Signatures/MissingDiagnostic.fs @@ -0,0 +1,54 @@ +module FSharp.Compiler.ComponentTests.Signatures.MissingDiagnostic + +open Xunit +open FSharp.Test +open FSharp.Test.Compiler + +let implementation = """ +module Foo + +let a (b: int) : int = 'x' +""" + +let signature = """ +module Foo + +val a: b: int -> int +""" + +[] +let ``Compile gives errors`` () = + Fsi signature + |> withAdditionalSourceFile (FsSource implementation) + |> compile + |> shouldFail + |> withSingleDiagnostic (Error 1, Line 4, Col 24, Line 4,Col 27, "This expression was expected to have type + 'int' +but here has type + 'char' ") + +[] +let ``Type check project with signature file doesn't get the diagnostic`` () = + Fsi signature + |> withAdditionalSourceFile (FsSource implementation) + |> typecheckProject false + |> fun projectResults -> + projectResults.Diagnostics |> ignore + Assert.False (projectResults.Diagnostics |> Array.isEmpty) + +[] +let ``Type check project without signature file does get the diagnostic`` () = + Fs implementation + |> typecheckProject false + |> fun projectResults -> + projectResults.Diagnostics |> ignore + Assert.False (projectResults.Diagnostics |> Array.isEmpty) + +[] +let ``Enabling enablePartialTypeChecking = true doesn't change the problem`` () = + Fsi signature + |> withAdditionalSourceFile (FsSource implementation) + |> typecheckProject true + |> fun projectResults -> + projectResults.Diagnostics |> ignore + Assert.False (projectResults.Diagnostics |> Array.isEmpty) \ No newline at end of file diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/TyparNameTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/TyparNameTests.fs index 8b81bc0a312..2761fb8cfeb 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/TyparNameTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/TyparNameTests.fs @@ -12,7 +12,7 @@ let private getGenericParametersNamesFor (additionalFile: SourceCodeFileKind) : string array = let typeCheckResult = - cUnit |> withAdditionalSourceFile additionalFile |> typecheckProject + cUnit |> withAdditionalSourceFile additionalFile |> typecheckProject false assert (Array.isEmpty typeCheckResult.Diagnostics) diff --git a/tests/FSharp.Test.Utilities/Compiler.fs b/tests/FSharp.Test.Utilities/Compiler.fs index c8adbc52e6e..caa56ba97e5 100644 --- a/tests/FSharp.Test.Utilities/Compiler.fs +++ b/tests/FSharp.Test.Utilities/Compiler.fs @@ -917,7 +917,7 @@ module rec Compiler = CompilerAssert.TypeCheck(options, fileName, source) | _ -> failwith "Typecheck only supports F#" - let typecheckProject (cUnit: CompilationUnit) : FSharp.Compiler.CodeAnalysis.FSharpCheckProjectResults = + let typecheckProject enablePartialTypeChecking (cUnit: CompilationUnit) : FSharp.Compiler.CodeAnalysis.FSharpCheckProjectResults = match cUnit with | FS fsSource -> let options = fsSource.Options |> Array.ofList @@ -935,7 +935,7 @@ module rec Compiler = |> async.Return let sourceFiles = Array.map fst sourceFiles - CompilerAssert.TypeCheckProject(options, sourceFiles, getSourceText) + CompilerAssert.TypeCheckProject(options, sourceFiles, getSourceText, enablePartialTypeChecking) | _ -> failwith "Typecheck only supports F#" let run (result: CompilationResult) : CompilationResult = diff --git a/tests/FSharp.Test.Utilities/CompilerAssert.fs b/tests/FSharp.Test.Utilities/CompilerAssert.fs index c5ec19006e5..f209beab7d0 100644 --- a/tests/FSharp.Test.Utilities/CompilerAssert.fs +++ b/tests/FSharp.Test.Utilities/CompilerAssert.fs @@ -858,8 +858,8 @@ Updated automatically, please check diffs in your pull request, changes must be static member TypeCheckSingleError (source: string) (expectedSeverity: FSharpDiagnosticSeverity) (expectedErrorNumber: int) (expectedErrorRange: int * int * int * int) (expectedErrorMsg: string) = CompilerAssert.TypeCheckWithErrors source [| expectedSeverity, expectedErrorNumber, expectedErrorRange, expectedErrorMsg |] - static member TypeCheckProject(options: string array, sourceFiles: string array, getSourceText) : FSharpCheckProjectResults = - let checker = FSharpChecker.Create(documentSource = DocumentSource.Custom getSourceText) + static member TypeCheckProject(options: string array, sourceFiles: string array, getSourceText, enablePartialTypeChecking) : FSharpCheckProjectResults = + let checker = FSharpChecker.Create(documentSource = DocumentSource.Custom getSourceText, enablePartialTypeChecking = enablePartialTypeChecking) let defaultOptions = defaultProjectOptions TargetFramework.Current let projectOptions = { defaultOptions with OtherOptions = Array.append options defaultOptions.OtherOptions; SourceFiles = sourceFiles } From 213a1439084acc8eeafb5efae4bf55ebf1c9aa6a Mon Sep 17 00:00:00 2001 From: Jakub Majocha Date: Fri, 9 Jun 2023 00:39:56 +0200 Subject: [PATCH 2/2] fix missing diags --- src/Compiler/Service/IncrementalBuild.fs | 62 ++++++++++++++++++------ 1 file changed, 47 insertions(+), 15 deletions(-) diff --git a/src/Compiler/Service/IncrementalBuild.fs b/src/Compiler/Service/IncrementalBuild.fs index 5403a37b86b..6dc16fedf0a 100644 --- a/src/Compiler/Service/IncrementalBuild.fs +++ b/src/Compiler/Service/IncrementalBuild.fs @@ -234,7 +234,8 @@ module ValueOption = | ValueSome x -> Some x | _ -> None -type private TypeCheck = TcInfo * TcResultsSinkImpl * CheckedImplFile option * string +type private SingleFileDiagnostics = (PhasedDiagnostic * FSharpDiagnosticSeverity) array +type private TypeCheck = TcInfo * TcResultsSinkImpl * CheckedImplFile option * string * SingleFileDiagnostics /// Bound model of an underlying syntax and typed tree. type BoundModel private ( @@ -299,7 +300,7 @@ type BoundModel private ( | _ -> None } - return tcInfo, sink, implFile, fileName + return tcInfo, sink, implFile, fileName, newErrors } let skippedImplemetationTypeCheck = @@ -322,13 +323,13 @@ type BoundModel private ( let getTcInfo (typeCheck: GraphNode) = node { - let! tcInfo , _, _, _ = typeCheck.GetOrComputeValue() + let! tcInfo , _, _, _, _ = typeCheck.GetOrComputeValue() return tcInfo } |> GraphNode let getTcInfoExtras (typeCheck: GraphNode) = node { - let! _ , sink, implFile, fileName = typeCheck.GetOrComputeValue() + let! _ , sink, implFile, fileName, _ = typeCheck.GetOrComputeValue() // Build symbol keys let itemKeyStore, semanticClassification = if enableBackgroundItemKeyStoreAndSemanticClassification then @@ -365,22 +366,36 @@ type BoundModel private ( } } |> GraphNode + let defaultTypeCheck = node { return prevTcInfo, TcResultsSinkImpl(tcGlobals), None, "default typecheck - no syntaxTree", [||] } + let typeCheckNode = syntaxTreeOpt |> Option.map getTypeCheck |> Option.defaultValue defaultTypeCheck |> GraphNode + let tcInfoExtras = getTcInfoExtras typeCheckNode + let diagnostics = + node { + let! _, _, _, _, diags = typeCheckNode.GetOrComputeValue() + return diags + } |> GraphNode + + let startComputingFullTypeCheck = + node { + let! _ = tcInfoExtras.GetOrComputeValue() + return! diagnostics.GetOrComputeValue() + } + let tcInfo, tcInfoExtras = - let defaultTypeCheck = node { return prevTcInfo, TcResultsSinkImpl(tcGlobals), None, "default typecheck - no syntaxTree" } - let typeCheckNode = syntaxTreeOpt |> Option.map getTypeCheck |> Option.defaultValue defaultTypeCheck |> GraphNode match tcStateOpt with | Some tcState -> tcState | _ -> match skippedImplemetationTypeCheck with - | Some info -> + | Some tcInfo -> // For skipped implementation sources do full type check only when requested. - GraphNode.FromResult info, getTcInfoExtras typeCheckNode + GraphNode.FromResult tcInfo, tcInfoExtras | _ -> - let tcInfoExtras = getTcInfoExtras typeCheckNode // start computing extras, so that typeCheckNode can be GC'd quickly - tcInfoExtras.GetOrComputeValue() |> Async.AwaitNodeCode |> Async.Ignore |> Async.Start + startComputingFullTypeCheck |> Async.AwaitNodeCode |> Async.Ignore |> Async.Start getTcInfo typeCheckNode, tcInfoExtras + member val Diagnostics = diagnostics + member val TcInfo = tcInfo member val TcInfoExtras = tcInfoExtras @@ -633,7 +648,7 @@ module IncrementalBuilderHelpers = #if !NO_TYPEPROVIDERS ,importsInvalidatedByTypeProvider: Event #endif - ) : NodeCode = + ) : NodeCode = node { let diagnosticsLogger = CompilationDiagnosticLogger("CombineImportedAssembliesTask", tcConfig.diagnosticsOptions) @@ -706,11 +721,11 @@ module IncrementalBuilderHelpers = fileChecked, tcInfo, None - ) + ), initialErrors } /// Finish up the typechecking to produce outputs for the rest of the compilation process - let FinalizeTypeCheckTask (tcConfig: TcConfig) tcGlobals partialCheck assemblyName outfile (boundModels: GraphNode seq) = + let FinalizeTypeCheckTask (tcConfig: TcConfig) tcGlobals partialCheck assemblyName outfile (initialErrors: SingleFileDiagnostics) (boundModels: GraphNode seq) = node { let diagnosticsLogger = CompilationDiagnosticLogger("FinalizeTypeCheckTask", tcConfig.diagnosticsOptions) use _ = new CompilationGlobalsScope(diagnosticsLogger, BuildPhase.TypeCheck) @@ -796,7 +811,18 @@ module IncrementalBuilderHelpers = mkSimpleAssemblyRef assemblyName, ProjectAssemblyDataResult.Unavailable true, None let finalBoundModel = Array.last computedBoundModels - let diagnostics = diagnosticsLogger.GetDiagnostics() :: finalInfo.tcDiagnosticsRev + + // Collect diagnostics. This will type check in parallel any implementation files skipped so far. + let! partialDiagnostics = + computedBoundModels + |> Seq.map (fun m -> m.Diagnostics.GetOrComputeValue()) + |> NodeCode.Parallel + let diagnostics = [ + diagnosticsLogger.GetDiagnostics() + yield! partialDiagnostics |> Seq.rev + initialErrors + ] + let! finalBoundModelWithErrors = finalBoundModel.Finish(diagnostics, Some topAttrs) return ilAssemRef, tcAssemblyDataOpt, tcAssemblyExprOpt, finalBoundModelWithErrors } @@ -805,6 +831,7 @@ module IncrementalBuilderHelpers = type IncrementalBuilderInitialState = { initialBoundModel: BoundModel + initialErrors: SingleFileDiagnostics tcGlobals: TcGlobals referencedAssemblies: ImmutableArray * (TimeStampCache -> DateTime)> tcConfig: TcConfig @@ -830,6 +857,7 @@ type IncrementalBuilderInitialState = static member Create ( initialBoundModel: BoundModel, + initialErrors: SingleFileDiagnostics, tcGlobals, nonFrameworkAssemblyInputs, tcConfig: TcConfig, @@ -852,6 +880,7 @@ type IncrementalBuilderInitialState = let initialState = { initialBoundModel = initialBoundModel + initialErrors = initialErrors tcGlobals = tcGlobals referencedAssemblies = nonFrameworkAssemblyInputs |> ImmutableArray.ofSeq tcConfig = tcConfig @@ -920,6 +949,7 @@ module IncrementalBuilderStateHelpers = let createFinalizeBoundModelGraphNode (initialState: IncrementalBuilderInitialState) (boundModels: GraphNode seq) = GraphNode(node { use _ = Activity.start "GetCheckResultsAndImplementationsForProject" [|Activity.Tags.project, initialState.outfile|] + let! initialErrors = initialState.initialBoundModel.Diagnostics.GetOrComputeValue() let! result = FinalizeTypeCheckTask initialState.tcConfig @@ -927,6 +957,7 @@ module IncrementalBuilderStateHelpers = initialState.enablePartialTypeChecking initialState.assemblyName initialState.outfile + initialErrors boundModels return result, DateTime.UtcNow }) @@ -1532,7 +1563,7 @@ type IncrementalBuilder(initialState: IncrementalBuilderInitialState, state: Inc let defaultTimeStamp = DateTime.UtcNow - let! initialBoundModel = + let! initialBoundModel, initialErrors = CombineImportedAssembliesTask( assemblyName, tcConfig, @@ -1572,6 +1603,7 @@ type IncrementalBuilder(initialState: IncrementalBuilderInitialState, state: Inc let initialState = IncrementalBuilderInitialState.Create( initialBoundModel, + initialErrors, tcGlobals, nonFrameworkAssemblyInputs, tcConfig,