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

Preserve original stacktrace in state machines if available #16568

Merged
merged 5 commits into from
Jan 23, 2024

Conversation

vzarytovskii
Copy link
Member

Description

Preserve original stacktrace in resumable state machines generated code, if avalable.

Before:

at Microsoft.FSharp.Core.CompilerServices.ResumableCode.TryFinallyCompensateDynamic[TData](ResumableStateMachine`1& sm, ResumptionFunc`1 mf, FSharpOption`1 savedExn) in /Users/u/code/fsharp3/src/FSharp.Core/resumable.fs:line 299
at Microsoft.FSharp.Core.CompilerServices.ResumableCode.TryFinallyAsyncDynamic[TData,T](ResumableStateMachine`1& sm, ResumableCode`2 body, ResumableCode`2 compensation) in /Users/u/code/fsharp3/src/FSharp.Core/resumable.fs:line 326
at VahterBanBot.Bot.onUpdate@391-46.Invoke(ResumableStateMachine`1& sm)
at VahterBanBot.Bot.onUpdate@389-48.Invoke(ResumableStateMachine`1& sm)
at <StartupCode$FSharp-Core>.$Tasks.initialResumptionFunc@168.Invoke(ResumableStateMachine`1& sm) in /Users/u/code/fsharp3/src/FSharp.Core/tasks.fs:line 168
at <StartupCode$FSharp-Core>.$Tasks.resumptionInfo@171.MoveNext(ResumableStateMachine`1& sm) in /Users/u/code/fsharp3/src/FSharp.Core/tasks.fs:line 177
      --- End of stack trace from previous location ---
at Program.webApp@110-2.MoveNext() in /Users/u/code/vahter-bot/src/VahterBanBot/Program.fs:line 115

After:

at VahterBanBot.Bot.onUpdate@392-1.Invoke(Activity banOnReplyActivity) in /Users/u/code/vahter-bot/src/VahterBanBot/Bot.fs:line 392
at VahterBanBot.Bot.onUpdate@391-44.Invoke(ResumableStateMachine`1& sm)
at Microsoft.FSharp.Core.CompilerServices.ResumableCode.TryFinallyAsyncDynamic[TData,T](ResumableStateMachine`1& sm, ResumableCode`2 body, ResumableCode`2 compensation) in /Users/u/code/fsharp3/src/FSharp.Core/resumable.fs:line 323
      --- End of stack trace from previous location ---
at Microsoft.FSharp.Core.CompilerServices.ResumableCode.TryFinallyCompensateDynamic[TData](ResumableStateMachine`1& sm, ResumptionFunc`1 mf, FSharpOption`1 savedExn) in /Users/u/code/fsharp3/src/FSharp.Core/resumable.fs:line 302
at Microsoft.FSharp.Core.CompilerServices.ResumableCode.TryFinallyAsyncDynamic[TData,T](ResumableStateMachine`1& sm, ResumableCode`2 body, ResumableCode`2 compensation) in /Users/u/code/fsharp3/src/FSharp.Core/resumable.fs:line 329
at VahterBanBot.Bot.onUpdate@391-46.Invoke(ResumableStateMachine`1& sm)
at VahterBanBot.Bot.onUpdate@389-48.Invoke(ResumableStateMachine`1& sm)
at <StartupCode$FSharp-Core>.$Tasks.initialResumptionFunc@168.Invoke(ResumableStateMachine`1& sm) in /Users/u/code/fsharp3/src/FSharp.Core/tasks.fs:line 168
at <StartupCode$FSharp-Core>.$Tasks.resumptionInfo@171.MoveNext(ResumableStateMachine`1& sm) in /Users/u/code/fsharp3/src/FSharp.Core/tasks.fs:line 177
      --- End of stack trace from previous location ---
at Program.webApp@110-2.MoveNext() in /Users/u/code/vahter-bot/src/VahterBanBot/Program.fs:line 115

Notice that it will now print a line exception originated from (topmost line in the stack trace above).

I want to additionally look into using the following attribute if available in code generated state machines:

https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.stacktracehiddenattribute?view=net-6.0

Checklist

@T-Gro @psfinaki @0101 @abonie @KevinRansom I am not sure if we need a dedicated set of tests for this case. We don't usually do that, and how things are thrown (stack traces in particular) is an implementation detail. I will not add it unless any of you insist.

  • Test cases added (N/A)
  • Release notes entry updated:

Copy link
Contributor

github-actions bot commented Jan 22, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/FSharp.Core docs/release-notes/.FSharp.Core/8.0.300.md

@vzarytovskii
Copy link
Member Author

Note: this will only affect dynamically compiled state machines

@psfinaki
Copy link
Member

Great stuff. Regarding testing: we can always insist, the questions is - how hard is it to add them and how else verify that nothing explodes.

Personally, this feels safe to me, or more exactly small and isolated enough.

@vzarytovskii vzarytovskii marked this pull request as ready for review January 22, 2024 16:09
@vzarytovskii vzarytovskii requested a review from a team as a code owner January 22, 2024 16:09
@vzarytovskii
Copy link
Member Author

/run fantomas

  Co-authored-by: vzarytovskii <[email protected]>
Copy link
Contributor

@vzarytovskii
Copy link
Member Author

Great stuff. Regarding testing: we can always insist, the questions is - how hard is it to add them and how else verify that nothing explodes.

Will be easy to add, but might be tricky to verify w.r.t. line numbers and such, will probably be more code for testing infrastructure than the change itself.

@0101
Copy link
Contributor

0101 commented Jan 22, 2024

Will be easy to add, but might be tricky to verify w.r.t. line numbers and such, will probably be more code for testing infrastructure than the change itself.

Shouldn't be too bad. You can access the stack trace programmatically with System.Diagnostics.StackTrace and maybe just check the method name from the first frame, or whatever else you're interested in. Don't have to care about line numbers or any other specifics.

@vzarytovskii
Copy link
Member Author

Will be easy to add, but might be tricky to verify w.r.t. line numbers and such, will probably be more code for testing infrastructure than the change itself.

Shouldn't be too bad. You can access the stack trace programmatically with System.Diagnostics.StackTrace and maybe just check the method name from the first frame, or whatever else you're interested in. Don't have to care about line numbers or any other specifics.

I decided not to do that for now, since we don't have such tests anyway.

@vzarytovskii vzarytovskii merged commit f50f8e8 into dotnet:main Jan 23, 2024
28 checks passed
psfinaki added a commit that referenced this pull request Jan 25, 2024
* 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 <[email protected]>

* Build benchmarks in CI (#16518)

* Remove profiling startpoint project

* Add bench build job

* Up

* up

* up

---------

Co-authored-by: Kevin Ransom (msft) <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

---------

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 <[email protected]>

* Improve AsyncMemoize tests (#16580)

---------

Co-authored-by: Eugene Auduchinok <[email protected]>
Co-authored-by: Vlad Zarytovskii <[email protected]>
Co-authored-by: Petr <[email protected]>
Co-authored-by: Kevin Ransom (msft) <[email protected]>
Co-authored-by: Petr Pokorny <[email protected]>
Co-authored-by: Florian Verdonck <[email protected]>
Co-authored-by: dawe <[email protected]>
Co-authored-by: Tomas Grosup <[email protected]>
Co-authored-by: Martin <[email protected]>
Co-authored-by: Brian Rourke Boll <[email protected]>
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 <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

4 participants