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

Fix StackOverflow in non-recursive bindings checker #16908

Merged
merged 5 commits into from
Mar 21, 2024

Conversation

vzarytovskii
Copy link
Member

@vzarytovskii vzarytovskii commented Mar 20, 2024

Description

Fixes #16867 by making main recursive checking function non-cancellable (and tail recursive). It's hard to make builder work under stack guards, since it needs to be calling guard from Bind, ReturnFrom and BindReturn.

This also adds additional activity logging for the stack guards (only when subscribed).

@dotnet/fsharp-team-msft do we want to backport it to VS17.9?

Checklist

  • Test cases added not applicable
  • Performance benchmarks added in case of performance changes not applicable
  • Release notes entry updated

@vzarytovskii vzarytovskii requested a review from a team as a code owner March 20, 2024 14:32
Copy link
Contributor

github-actions bot commented Mar 20, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

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

@vzarytovskii
Copy link
Member Author

/run fantomas

  Co-authored-by: vzarytovskii <1260985+vzarytovskii@users.noreply.github.com>
Copy link
Contributor

Copy link
Contributor

@brianrourkeboll brianrourkeboll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Test cases added not applicable

Would a test that type-checks a module with a few thousand let-bindings not make sense?

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Mar 20, 2024

  • Test cases added not applicable

Would a test that type-checks a module with a few thousand let-bindings not make sense?

We have no way of running fscAnyCpu (and it's only reproduces on full CLR on any cpu) outside of fsharpqa, and I don't want to add it there since we try to get rid of it.

@T-Gro
Copy link
Member

T-Gro commented Mar 20, 2024

The description is a little misleading - the behavior is still cancellable, it just does not use the cancellable-builder/CE anymore, right?

@vzarytovskii vzarytovskii merged commit 1be52aa into dotnet:main Mar 21, 2024
32 checks passed
@vzarytovskii
Copy link
Member Author

The description is a little misleading - the behavior is still cancellable, it just does not use the cancellable-builder/CE anymore, right?

Well, yes and no. Before it could cancel on more occasions. Now it's only explicit in the beginning + when running cancellable inside.

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.

Error MSB6006: "fscAnyCpu.exe" exited with code -1073741571
5 participants