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

Many match clauses #16430

Closed
wants to merge 3 commits into from
Closed

Many match clauses #16430

wants to merge 3 commits into from

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Dec 13, 2023

Description

I'm investigating whether we can delay the creation of the decision tree in a match expression.
While doing this, I learned that you can't create a match clause with 9000 clauses.
It leads to a stackoverflow exception, I'm going to do some digging if that can be avoided.

Fixes # (issue, if applicable)

Checklist

  • Test cases added

  • Performance benchmarks added in case of performance changes

  • Release notes entry updated:

    Please make sure to add an entry with short succint description of the change as well as link to this pull request to the respective release notes file, if applicable.

    Release notes files:

    • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
    • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/Language/preview.md
    • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

    Examples of release notes entries:

    If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

@vzarytovskii
Copy link
Member

I'm investigating whether we can delay the creation of the decision tree in a match expression.

I've been looking into it some time ago, and as an addition to delaying it we we might want to think of a different path in some cases, when we don't don't pattern matching compilation.

Maybe there's a way to just put a "too compilcated" limit on pattern match compilation, active if any errors are present in any of the input patterns.

@vzarytovskii
Copy link
Member

We either can:

  1. Optionally move pattern match compilation to a post-pass, for the cases where we're not interested in code or diagnostics. This will mean adding TClause into the Typed Tree (but not pickled format) and adjust all routines that examine the TypedTree.
  2. Don't move it to a post phase, but just don't do pattern compilation at all. Meaning optionally skip it if tooling "asks", or skip it when producing types only, for checking later files.

If you'd like to, I can experiment with it.

@nojaf
Copy link
Contributor Author

nojaf commented Dec 18, 2023

Hmm, interesting proposals. I guess 2. might be a lot more practical.
How would that look like? An additional flag in TcEnv somewhere?

If you'd like to, I can experiment with it.

Yeah, for sure! That be great.

@auduchinok
Copy link
Member

Don't move it to a post phase, but just don't do pattern compilation at all. Meaning optionally skip it if tooling "asks", or skip it when producing types only, for checking later files.

We need to do it for the current file for the warnings about exhaustiveness. But they are optional in the way: they aren't required for the type inference and name resolution, so could be called later, like we've discussed it with @nojaf.

It also seems that the whole post inference phase could move to the optionally requested call if it doesn't change any state for the subsequent files.

@vzarytovskii
Copy link
Member

How would that look like? An additional flag in TcEnv somewhere?

Yeah, I'm thinking that for now. We can adjust as we go.

@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 18, 2023

Don't move it to a post phase, but just don't do pattern compilation at all. Meaning optionally skip it if tooling "asks", or skip it when producing types only, for checking later files.

We need to do it for the current file for the warnings about exhaustiveness.

Yes, but if it's (match) is big, and we have TC errors in project, it will likely never finish or will put huge pressure on the FCS (as it does now).

But they are optional in the way: they aren't required for the type inference and name resolution, so could be called later, like we've discussed it with @nojaf.

Yes, we can, as a PoC, have a flag for turning them off now, and later on try and expose it as a separate call/move to other step.

It also seems that the whole post inference phase could move to the optionally requested call if it doesn't change any state for the subsequent files.

Yes, but this is orthogonal to the issue. It can be part of some sort of "background" checking.

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.

3 participants