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

Make API usage of FSharpExprPatterns possible with typed union fields #16121

Merged
merged 6 commits into from
Nov 27, 2023

Conversation

dawedawe
Copy link
Contributor

Hey everyone,

FSC offers the FSharpExprPatterns Active Patterns API to easily traverse the TAST.
Unfortunately, the very common F# construct of a typed union field triggers an exception when traversed:

type T = Case1 of string

I hope the testcase in this PR helps to show the issue.

The reason is that DUs are processed by FSharp.Compiler.AugmentWithHashCompare to add the members CompareTo, GetHashCode, Equals

let dflt = if isNil nullary then None else Some (mbuilder.AddResultTarget(mkZero g m))

The TDSwitch that is constructed there has no default DecisionTree, it's None.
Later this is the reason that the exception is thrown at

| None -> wfail( "FSharp.Compiler.Service cannot yet return this kind of pattern match", m)

The exception throwing code is quite old, so I was wondering if we could improve this today.
But I'm really unsure what a good handling should look like. So this PR has just my best guess (commented out) for a change in Exprs.fs to make traversal possible for code that contains a typed union field.

I'd be very grateful for any pointers how to improve the current situation.

@dawedawe dawedawe requested a review from a team as a code owner October 12, 2023 16:44
@dawedawe
Copy link
Contributor Author

So, uhm, anyone interested in this?
As this API is really nice otherwise, I'm pretty sure there are API users out there who could profit from an improvement.

@psfinaki
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

So this can go I think? @T-Gro?

@dawedawe
Copy link
Contributor Author

Hey @T-Gro
I'm not quite sure if I understood your last comment correctly. Is this now in an acceptable state for you?

@T-Gro T-Gro merged commit b7e7479 into dotnet:main Nov 27, 2023
24 checks passed
@dawedawe dawedawe deleted the exprs_patterns branch November 27, 2023 12:21
webwarrior-ws added a commit to webwarrior-ws/FSharpLint that referenced this pull request Jan 16, 2024
Workaround for bug in FSharp.Compiler.Services that sometimes
results in "FSharp.Compiler.Service cannot yet return this kind of pattern match at ..."
exception (see ``Regression found when parsing Console/Program_fs``
test case).
Issue in F# github repo: [1]. There is a merged PR that should
address this bug: [2]. But at the moment latest
FSharp.Compiler.Services on nuget (43.8.101) doesn't include
that fix.

[1] dotnet/fsharp#8854
[2] dotnet/fsharp#16121
@knocte
Copy link

knocte commented Jan 18, 2024

@T-Gro thanks for merging. BTW, just curious, if this was included in a commit from about 2 months ago, and the last release of FCS (43.8.101) was published 9 days ago, why is this fix not included in this last release?

@knocte
Copy link

knocte commented Jan 30, 2024

@dawedawe do you know? ^

@vzarytovskii
Copy link
Member

vzarytovskii commented Jan 30, 2024

@T-Gro thanks for merging. BTW, just curious, if this was included in a commit from about 2 months ago, and the last release of FCS (43.8.101) was published 9 days ago, why is this fix not included in this last release?

X.Y.xx1 releases are fix releases, and all additional features are not backported to those, only critical fixes.

@knocte
Copy link

knocte commented Jan 30, 2024

X.Y.xx1 releases are fix releases, and all additional features are not backported to those, only critical fixes.

Thanks for your answer Vlad. Do you know then which release number would then include this, and roughly when is it going to hit nuget.org?

@vzarytovskii
Copy link
Member

X.Y.xx1 releases are fix releases, and all additional features are not backported to those, only critical fixes.

Thanks for your answer Vlad. Do you know then which release number would then include this, and roughly when is it going to hit nuget.org?

I think 8.0.200 should include it, you can test new versions here if you'd like to: https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet8/NuGet/FSharp.Compiler.Service/overview/43.8.200-preview.24061.1

@vzarytovskii
Copy link
Member

As for release date - I don't know, SDK publishes all of our packages, so it's tied to their release.

@knocte
Copy link

knocte commented Jan 30, 2024

I think 8.0.200 should include it

How does that version translate to FCS version numbers in nuget?

@vzarytovskii
Copy link
Member

I think 8.0.200 should include it

How does that version translate to FCS version numbers in nuget?

It should be 43.8.200

@knocte
Copy link

knocte commented Jan 30, 2024

It should be 43.8.200

Cool thanks!

webwarrior-ws added a commit to webwarrior-ws/FSharpLint that referenced this pull request Feb 15, 2024
Remove workaround introduced in [1] for bug in FCS [2] that was
fixed in FCS 43.8.200.

[1] 106be3f
[2] dotnet/fsharp#16121
webwarrior-ws added a commit to webwarrior-ws/FSharpLint that referenced this pull request Mar 4, 2024
Remove workaround introduced in [1] for bug in FCS [2] that was
fixed in FCS 43.8.200.

[1] 106be3f
[2] dotnet/fsharp#16121
webwarrior-ws added a commit to webwarrior-ws/FSharpLint that referenced this pull request Mar 5, 2024
Remove workaround introduced in [1] for bug in FCS [2] that was
fixed in FCS 43.8.200.

[1] 106be3f
[2] dotnet/fsharp#16121
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.

6 participants