-
Notifications
You must be signed in to change notification settings - Fork 789
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
Revert 16348 #16536
Revert 16348 #16536
Conversation
❗ Release notes required
|
@auduchinok I think we should revert unless you have any ideas on how to fix it. |
Yes, sure, the tests stability is much more important. We can also try to reiterate on it once more later, e.g. after the Node/Cancellable builders refactoring, if needed. |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Yeah that does look interesting, thanks for the report. Wonder if |
@majocha actually it would be good to know if you saw those exceptions with VSIX from I have personally not seen that exception. |
@0101 I'm now on current main, with this PR (revert) merged. The concurrency problem is gone and things seem to run really smooth. The only thing I still see is exceptions surfacing in VS, like the screenshot above, whenever I enable Transparent Compiler. Simple repro:
|
@0101 oops, just now on main:
I'm positive I built and installed VSIX from current main. |
Thanks! I'll try that. You mean you open an existing file, or add a new one? I've seen some issues when adding new files into a project. |
on top of existing will do |
The exception seems to point here fsharp/src/Compiler/Driver/GraphChecking/FileContentMapping.fs Lines 11 to 14 in 3af3d41
And |
Ok, I've been able to reproduce it by just typing With an extra one from A nested module cannot have multiple identifiersSystem.AggregateException : One or more errors occurred. ---> One or more errors occurred. ---> One or more errors occurred. ---> A nested module cannot have multiple identifiers
at <StartupCode$FSharp-Compiler-Service>.$AsyncMemoize.Get@488-13.Invoke(Exception _arg17)
at FSharp.Compiler.BuildGraph.TryWith@72-2.Invoke(Exception x)
at Microsoft.FSharp.Control.AsyncPrimitives.CallFilterThenInvoke[T](AsyncActivation`1 ctxt,FSharpFunc`2 filterFunction,ExceptionDispatchInfo edi)
at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.CheckerExtensions.FSharpChecker-ParseAndCheckDocumentUsingTransparentCompiler@355(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.CheckerExtensions.FSharpChecker-ParseAndCheckDocument@448(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.VisualStudio.FSharp.Editor.WorkspaceExtensions.Document-GetFSharpParseAndCheckResultsAsync@538(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async StartupCode$FSharp-Editor(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async StartupCode$FSharp-Editor(<Unknown Parameters>)
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at async Microsoft.CodeAnalysis.Extensions.IExtensionManagerExtensions.PerformActionAsync(<Unknown Parameters>)
---> (Inner Exception #0) System.AggregateException : One or more errors occurred. ---> One or more errors occurred. ---> A nested module cannot have multiple identifiers
at <StartupCode$FSharp-Compiler-Service>.$AsyncMemoize.Get@488-13.Invoke(Exception _arg17)
at FSharp.Compiler.BuildGraph.TryWith@72-2.Invoke(Exception x)
at Microsoft.FSharp.Control.AsyncPrimitives.CallFilterThenInvoke[T](AsyncActivation`1 ctxt,FSharpFunc`2 filterFunction,ExceptionDispatchInfo edi)
at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction)
---> (Inner Exception #0) System.AggregateException : One or more errors occurred. ---> A nested module cannot have multiple identifiers
at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout,CancellationToken cancellationToken)
at System.Threading.Tasks.Parallel.ForWorker[TLocal](Int32 fromInclusive,Int32 toExclusive,ParallelOptions parallelOptions,Action`1 body,Action`2 bodyWithState,Func`4 bodyWithLocal,Func`1 localInit,Action`1 localFinally)
at System.Threading.Tasks.Parallel.For(Int32 fromInclusive,Int32 toExclusive,Action`1 body)
at Microsoft.FSharp.Collections.ArrayModule.Parallel.Map[T,TResult](FSharpFunc`2 mapping,T[] array)
at FSharp.Compiler.GraphChecking.DependencyResolution.mkGraph(FilePairMap filePairs,FileInProject[] files)
at <StartupCode$FSharp-Compiler-Service>.$TransparentCompiler.clo@917-598.Invoke(IDisposable _arg19)
at FSharp.Compiler.BuildGraph.Using@116-6.Invoke(IDisposable _arg6)
at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvoke[T,TResult](AsyncActivation`1 ctxt,TResult result1,FSharpFunc`2 part2)
at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction)
---> (Inner Exception #0) System.Exception : A nested module cannot have multiple identifiers
at FSharp.Compiler.GraphChecking.FileContentMapping.visitSynModuleDecl(SynModuleDecl decl)
at Microsoft.FSharp.Primitives.Basics.List.collectToFreshConsTail[T,TResult](FSharpFunc`2 f,FSharpList`1 list,FSharpList`1 cons)
at Microsoft.FSharp.Primitives.Basics.List.collect[T,TResult](FSharpFunc`2 f,FSharpList`1 list)
at FSharp.Compiler.GraphChecking.FileContentMapping.mkFileContent(FileInProject f)
at Microsoft.FSharp.Collections.ArrayModule.Parallel.Map@2060-2.Invoke(Int32 i)
at System.Threading.Tasks.Parallel.<>c__DisplayClass17_0`1.<ForWorker>b__1()
at System.Threading.Tasks.Task.InnerInvokeWithArg(Task childTask)
at System.Threading.Tasks.Task.<>c__DisplayClass176_0.<ExecuteSelfReplicating>b__0(Object <p0>) <---
<---
<--- |
Yeah, this is not concurrency related, just something in the code fix? |
Both of them are coming from the graph processing stuff (which I didn't touch). They just somehow bubble all the way to VS. It could be concurrency related if they end up in a wrong ThreadStatic DiagnosticsLogger which doesn't handle them correctly. Or maybe we're just missing a try/with somewhere to deal with these and gracefully abort the checking. Or there's some other issue which makes the input for constructing the graph somehow wrong. |
Yes, unfortunately this could be the case. fsharp/src/Compiler/Facilities/BuildGraph.fs Lines 16 to 26 in 276fc42
There is assumption the code after |
Yeah that would be bad. But most likely What we're seeing here is most likely all symptoms of Graph-based checking not expecting invalid (recovered) syntax trees as input, because it was only intended for a command line compiler where that was not an issue. (Example #1) So we'll have to find and fix those and probably add some guard for when it fails for whatever reason and try to recover it somehow or at least just not crash. |
It for sure is not explicitly launched on different thread. It will pretty much launch on the same thread async was in the moment when exception/completion happened. |
But after a few continuations in between it's not necessarily the same thread on which we saved the threadstatics initially? |
Yeah, we technically can end up on different thread, so ideally Unless the intent is to always restore threadstatics on the thread continuation is running. Which is likely the case (cc @0101). I am still wrapping my head around this. |
That's may not be possible in this model (?). So instead we need to make sure we copy them over from the previous thread whenever there's a chance we could have jumped to a new one.
Yeah that makes sense. The code that will run afterwards needs to have the same thread statics. It doesn't need to be back on the same thread. Still this test should be passing: #16576 So there must be a place where we don't copy over the phase when jumping on a new thread. |
Well I guess it's here. fsharp/src/Compiler/Facilities/BuildGraph.fs Lines 195 to 196 in 641d0ee
|
* Name resolution: keep type vars in subsequent checks (#16456) * Keep typars produced in name resolution * Better debug errors * Unwrap measure type vars * Undo check declarations change * Fix reported range * Undo occurrence change * Skip path typars * Add test * More freshen typar APIs properly * Fantomas * Cleanup * Add release notes * 123 --------- Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> * Build benchmarks in CI (#16518) * Remove profiling startpoint project * Add bench build job * Up * up * up --------- Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com> * More ValueOption in compiler: part 1 (#16323) * More ValueOption in compiler: part 1 * release notes * Update CheckComputationExpressions.fs * release notes * `[Experimental]` `[WIP]` Transparent Compiler (#15179) * Track CheckDeclarations.CheckModuleSignature activity. (#16534) * Add Computation Expression Benchmarks (#16541) * add benchmarks for various usages of CEs * refactor * move CE source files to dedicated ce folder * Update Roslyn to a version which uses Immutable v7 (#16545) * revert #16326 (addition of XliffTasks reference) (#16548) * updated devcontainer image (#16551) * Add higher-order-function-based API for working with untyped AST (#16462) * Add module-based API for working with untyped AST * Fantomas * tryPickUntil → tryPickDownTo * Don't need that * Thread path while walking * Update comment * Simplify * Expose `Ast.fold` and `Ast.tryPick`. * Expose `SyntaxNode.(|Attributes|)`. * Ensure a few more syntax node cases get hit. * Update FCS release notes * Update surface area * Add back `foldWhile`; add `exists`, `tryNode` * Put `Ast.foldWhile` back in. * Add `Ast.exists`. * Add `Ast.tryNode`. * `SyntaxTraversal.Traverse` → `Ast.tryPick`… * Replace uses of `SyntaxTraversal.Traverse` in `FSharpParseFileResults` with the appropriate function from the `Ast` module: `exists`, `tryPick`, `tryNode`. * Update surface area * Need that * Just to be safe * Add `Ast.tryPickLast` * Handle multiple args mid-pipeline * Before, no signature help was offered in a case like this: ```fsharp [1..10] |> List.fold (fun acc _ -> acc) ‸ |> List.filter (fun x -> x > 3) ``` The service will now offer help for the `state` parameter when the cursor ‸ is in that location. * `*` instead of error * `FSharpParseFileResults.TryRangeOfFunctionOrMethodBeingApplied` was previously returning the range of the (zero-width) `SynExpr.ArbitraryAfterError`. It now returns the range of the `*` (`op_Multiply`) instead. * Update surface area * Fmt * Missed in merge * Add VS release notes entry * # → ### * Add ryPick tests * Add a few more tests * \n * Bump release notes * Fmt * `Ast` → `ParsedInput` * Use `ParsedInput` as the main AST type. * Move the `position` parameter rightward. * Update surface area * Less `function` * Update untyped AST docs * Add basic examples for `ParsedInput` module functions. * Merge the existing `SyntaxVisitorBase` docs into the new file. * Clean up doc comments --------- Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> * Move paren entries to appropriate releases (#16561) * [main] Update dependencies from dotnet/source-build-reference-packages (#16532) * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240115.2 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24065.2 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240116.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24066.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240117.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24067.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240117.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24067.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240117.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24067.1 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240117.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24059.3 -> To Version 9.0.0-alpha.1.24067.1 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> * Attempt to make links from single identifier module names. (#16550) * Add scenarios where parentheses are around module name. * Address problem tighter to nameof usage. * Restore missing commit and inline nameof ident check. * Add release note entry. * rewrite SizeOfValueInfo in Optimizer.fs to be tail-recursive (#16559) * rewrite SizeOfValueInfo in Optimizer.fs to be tail-recursive * use Brians rewrite into one local function * stringbuilder is not threadsafe (#16557) * Array postfix notation in fsharp core api (#16564) * changed array types to postfix form in all signatures * changed array types to postfix form in the implementation files * Revert 16348 (#16536) * Improve AsyncMemoize tests * relax test condition * Revert "Cancellable: set token from node/async in features code (#16348)" This reverts commit d4e3b26. * remove UsingToken * remove UsingToken * test improvement * relax test condition * use thread-safe collections when collecting events from AsyncMemoize * fix flaky test * release note * Small code reshuffle for diff minimization (#16569) * Moving code around * Small code reshuffle for diff minimization * wat * Refactor parens API (#16461) * Refactor parens API * Remove `UnnecessaryParentheses.getUnnecessaryParentheses`. * Expose `SynExpr.shouldBeParenthesizedInContext`. * Expose `SynPat.shouldBeParenthesizedInContext`. * Expose `SyntaxTraversal.TraverseAll`. * Fantomas * Use `ParsedInput.fold` * Tests * Update surface area * Clean up sigs & comments * Update release notes * Remove redundant async * Remove stubs (no longer needed) * Preserve original stacktrace in state machines if available (#16568) * Preserve original stacktrace in state machines if available * Update release notes * Automated command ran: fantomas Co-authored-by: vzarytovskii <1260985+vzarytovskii@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * check reportErrors and feature support at top level (#16549) * Align DU case augmentation with previous behavior in EraseUnions (#16571) * Align DU case augment with previous behavior in EraseUnions * Update 8.0.300.md * modify tests * Refresh debug surface area (#16573) * Remove superfluous rec keywords and untangle some functions (#16544) * remove some superfluous rec keywords and untangle two functions that aren't mutually recursive. * Don't throw on invalid input in Graph construction (#16575) * More ValueOption in compiler: part 2 (#16567) * More ValueOption in complier: part 2 * Update release notes * extra optimization * extra optimization 2 * fantomas * Update dependencies from https://github.com/dotnet/arcade build 20240123.2 (#16579) Microsoft.DotNet.Arcade.Sdk From Version 8.0.0-beta.24060.4 -> To Version 8.0.0-beta.24073.2 Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> * [main] Update dependencies from dotnet/source-build-reference-packages (#16574) * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240122.5 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24067.1 -> To Version 9.0.0-alpha.1.24072.5 * Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20240123.1 Microsoft.SourceBuild.Intermediate.source-build-reference-packages From Version 9.0.0-alpha.1.24067.1 -> To Version 9.0.0-alpha.1.24073.1 --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> * Improve AsyncMemoize tests (#16580) --------- Co-authored-by: Eugene Auduchinok <eugene.auduchinok@gmail.com> Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com> Co-authored-by: Petr <psfinaki@users.noreply.github.com> Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com> Co-authored-by: Petr Pokorny <petrpokorny@microsoft.com> Co-authored-by: Florian Verdonck <florian.verdonck@outlook.com> Co-authored-by: dawe <dawedawe@posteo.de> Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> Co-authored-by: Martin <29605222+Martin521@users.noreply.github.com> Co-authored-by: Brian Rourke Boll <brianrourkeboll@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Jakub Majocha <1760221+majocha@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This reverts #16348 Cancellable: set token from node/async in features code
It seems it was generating some random cancellations in tests and I couldn't figure out why. It's likely that this can also occur in regular FCS usage outside tests.
There are also fixes for some unrelated test flakiness.