Skip to content

[FS-1140] add Boolean-returning and return-type-directed partial active patterns#16473

Merged
vzarytovskii merged 34 commits into
dotnet:mainfrom
ijklam:main
Feb 2, 2024
Merged

[FS-1140] add Boolean-returning and return-type-directed partial active patterns#16473
vzarytovskii merged 34 commits into
dotnet:mainfrom
ijklam:main

Conversation

@ijklam

@ijklam ijklam commented Dec 28, 2023

Copy link
Copy Markdown
Contributor

Description

Implements this

Current progress:
图片

Checklist

  • RFC added

  • Test cases added

  • Performance benchmarks added in case of performance changes

  • Release notes entry updated:

    Please make sure to add an entry with short succinct 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.

    Information about the release notes entries format can be found in the documentation.
    Example:

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

@vzarytovskii

vzarytovskii commented Dec 28, 2023

Copy link
Copy Markdown
Member

This will need a small RFC, describing the approach and codegen, so we don't lose it, since it's a change to spec.

@ijklam

ijklam commented Dec 29, 2023

Copy link
Copy Markdown
Contributor Author

This will need a small RFC, describing the approach and codegen, so we don't lose it, since it's a change to spec.

RFC added

@ijklam

ijklam commented Dec 30, 2023

Copy link
Copy Markdown
Contributor Author

图片

Now these cases will be not allowed. Maybe it needs some more proper error message?

@github-actions

github-actions Bot commented Dec 30, 2023

Copy link
Copy Markdown
Contributor

❗ 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
LanguageFeatures.fsi docs/release-notes/.Language/preview.md

@ijklam ijklam marked this pull request as ready for review December 30, 2023 16:34
@ijklam ijklam requested a review from a team as a code owner December 30, 2023 16:34
Comment thread src/Compiler/FSComp.txt Outdated
Comment thread src/Compiler/TypedTree/TypedTree.fs Outdated
Comment thread tests/fsharp/Compiler/Language/StructActivePatternTests.fs Outdated
Comment thread src/Compiler/Checking/NameResolution.fs Outdated
Comment thread src/Compiler/TypedTree/TypedTree.fs Outdated
Comment thread src/Compiler/Checking/PatternMatchCompilation.fs Outdated
Comment thread src/Compiler/Checking/CheckExpressions.fs
Comment thread tests/fsharp/Compiler/Language/StructActivePatternTests.fs
Comment thread tests/fsharp/Compiler/Language/BoolPartialActivePatternTests.fs Outdated
Comment thread src/Compiler/TypedTree/TypedTree.fs Outdated
@ijklam

ijklam commented Jan 20, 2024

Copy link
Copy Markdown
Contributor Author

Could someone fix the test FSharp.Compiler.UnitTests.AssemblySigning.AssemblyKeyNameAttribute NETCOREAPP? Or tell me how to fix it?

@psfinaki

Copy link
Copy Markdown
Contributor

@Tangent-90 maybe this helps.

@vzarytovskii

Copy link
Copy Markdown
Member

Is this still WIP as title suggests, or ready for review?

@ijklam

ijklam commented Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

Is this still WIP as title suggests, or ready for review?

It may need the RFC to be accepted first?

@ijklam ijklam changed the title [WIP] add bool partial active pattern [FS-1140] add Boolean-returning and return-type-directed partial active patterns Jan 23, 2024
@ijklam

ijklam commented Jan 23, 2024

Copy link
Copy Markdown
Contributor Author

@vzarytovskii Title changed, it's now ready

@vzarytovskii

Copy link
Copy Markdown
Member

图片

Now these cases will be not allowed. Maybe it needs some more proper error message?

@Tangent-90 I need a bit more clarity here. You mean it won't be allowed only in case of bool, right? Still going to work for options?

@vzarytovskii

Copy link
Copy Markdown
Member

图片
Now these cases will be not allowed. Maybe it needs some more proper error message?

@Tangent-90 I need a bit more clarity here. You mean it won't be allowed only in case of bool, right? Still going to work for options?

Yeah, just tested on your branch, was worried for a moment that it's a breaking change :)

@ijklam

ijklam commented Jan 29, 2024

Copy link
Copy Markdown
Contributor Author
图片 Now these cases will be not allowed. Maybe it needs some more proper error message?

@Tangent-90 I need a bit more clarity here. You mean it won't be allowed only in case of bool, right? Still going to work for options?

The behavior of options and voptions doesn't changed.

图片

@baronfel

baronfel commented Feb 1, 2024

Copy link
Copy Markdown
Member

@Tangent-90 I just want to say that this is awesome, the idea is awesome, you are awesome, and I can't wait to use this.

@vzarytovskii vzarytovskii merged commit 4390d68 into dotnet:main Feb 2, 2024
@vzarytovskii

vzarytovskii commented Feb 2, 2024

Copy link
Copy Markdown
Member

@Tangent-90 thanks a lot for this contribution!

FSharp """let (|IsA|_|) x = x = "A"

match "A" with
| IsA result -> "A"

@auduchinok auduchinok Mar 8, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we try to resolve result to another symbol here (see the error on line 101)? Lowercase names at argument positions are never being resolved to any other symbol in patterns and always create new local values (or are ignored during recovery).

This is very inconsistent to the rest of the language. It should probably be an error about an extra arg instead, definitely not the 'unresolved name' one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is because active pattern returning _ option or _ voption can capture return value by the last argument, but active pattern returning bool doesn't. The test is to check it works.

New error added, see #16846.

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.

8 participants