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

Allow parameter-less CustomOperation #16475

Conversation

vzarytovskii
Copy link
Member

@vzarytovskii vzarytovskii commented Dec 29, 2023

Description

Implements fsharp/fslang-suggestions#1250

I can't think of the need of putting it under language version, since if new fslib is used with old compiler, said compiler will be unable to produce warning that new attribute constructor won't have any effect

Checklist

  • Test cases added
    • Add lower and upper case methods
    • Add Quoted method names test, disallow if needed this is allowed now, so we'll allow it in the new attribute.
    • Add operators methods irrelevant, since operators are static methods.
  • Add RFC (does this need an RFC?) this doesn't need an RFC, since it's a very minor change.
  • Disallow empty string as name (allowed now, but unusable, should it be an equivalent of unit constructor?) empty name is the equivalent of parameterless attribute.
  • Release notes entry updated

@vzarytovskii vzarytovskii marked this pull request as ready for review December 30, 2023 13:26
@vzarytovskii vzarytovskii requested a review from a team as a code owner December 30, 2023 13:26
Copy link
Contributor

github-actions bot commented Dec 30, 2023

❗ 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.200.md
src/Compiler docs/release-notes/.FSharp.Compiler.Service/8.0.200.md

@vzarytovskii
Copy link
Member Author

Only took like a month to figure release notes automation out. @nojaf if you'll have a minute, could you please check the format and see if it needs and adjustment?

@nojaf
Copy link
Contributor

nojaf commented Jan 2, 2024

Only took like a month to figure release notes automation out. @nojaf if you'll have a minute, could you please check the format and see if it needs and adjustment?

The example looks off compared to https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

@vzarytovskii
Copy link
Member Author

Right now, the following code

open System
type C() =
    [<CustomOperation("foo bar")>]
    member _.M() = ()
    member _.Zero _ = ()
    member _.Yield _ = ()
    
let c = C()

c { ``foo bar`` () }
c { ``foo bar`` }

will yield the following compiler diagnostics:

error FS3099: 'foo bar' is used with an incorrect number of arguments. This is a custom operation in this query or computation expression. Expected 0 argument(s), but given 1.
error FS0501: The member or object constructor 'M' takes 0 argument(s) but is here given 1. The required signature is 'member C.M: unit -> unit'.
error FS0501: The member or object constructor 'M' takes 0 argument(s) but is here given 1. The required signature is 'member C.M: unit -> unit'.

Which is a bit confusing. @T-Gro @0101 What do you think if we disallow spaces in the parameters of the attribute, and in method names if attribute is parameter-less? Like introduce a new error (or warning) under preview version which will let user know that builder will compile but won't work?

@T-Gro
Copy link
Member

T-Gro commented Jan 2, 2024

Right now, the following code

open System
type C() =
    [<CustomOperation("foo bar")>]
    member _.M() = ()
    member _.Zero _ = ()
    member _.Yield _ = ()
    
let c = C()

c { ``foo bar`` () }
c { ``foo bar`` }

will yield the following compiler diagnostics:

error FS3099: 'foo bar' is used with an incorrect number of arguments. This is a custom operation in this query or computation expression. Expected 0 argument(s), but given 1.
error FS0501: The member or object constructor 'M' takes 0 argument(s) but is here given 1. The required signature is 'member C.M: unit -> unit'.
error FS0501: The member or object constructor 'M' takes 0 argument(s) but is here given 1. The required signature is 'member C.M: unit -> unit'.

Which is a bit confusing. @T-Gro @0101 What do you think if we disallow spaces in the parameters of the attribute, and in method names if attribute is parameter-less? Like introduce a new error (or warning) under preview version which will let user know that builder will compile but won't work?

Error has the problem with existing code (builder author, not consumer) compiling fine, warning at build time of the builder could work.

@vzarytovskii
Copy link
Member Author

Error has the problem with existing code (builder author, not consumer) compiling fine, warning at build time of the builder could work.

Diagnostic will only affect builder author, not consumer. So existing compiled builders should work fine (if they anyhow using custom operations with spaces inside).

@T-Gro
Copy link
Member

T-Gro commented Jan 2, 2024

Error has the problem with existing code (builder author, not consumer) compiling fine, warning at build time of the builder could work.

Diagnostic will only affect builder author, not consumer. So existing compiled builders should work fine (if they anyhow using custom operations with spaces inside).

It's just the unpleasant situation for a builder author to update .NET SDK, and suddenly getting a new error.

@vzarytovskii
Copy link
Member Author

Error has the problem with existing code (builder author, not consumer) compiling fine, warning at build time of the builder could work.

Diagnostic will only affect builder author, not consumer. So existing compiled builders should work fine (if they anyhow using custom operations with spaces inside).

It's just the unpleasant situation for a builder author to update .NET SDK, and suddenly getting a new error.

I don't think it's used anywhere (as it can't be utilised in any way), it will be under version flag as well. And it makes code more correct. I'm fine with warning as well.

@0101
Copy link
Contributor

0101 commented Jan 2, 2024

I don't think it's the space, I see the same error even without it. Can we make it work? Or just give diagnostic that tells you to write _ instead of () in the method definition. Whatever is easiest.

@vzarytovskii
Copy link
Member Author

Well, it seems that you can, in fact, call custom operations:

open System
type C() =
    [<CustomOperation("foo bar")>]
    member _.M(()) = ()
    member _.Zero _ = ()
    member _.Yield _ = ()
    
let c = C()

c { ``foo bar`` }

compiles just fine, so we can't disallow it.

I will just add the check for the new case (parameterless constructor).

(https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AbEAzAzgHwgAcYA7AAgGUBPXAFxgFsBYAKDupPIGEAKASnIBeNuTHkA2gB5uAV3oRGAeRJQAhnQCWEUrwBE2CBHLA1UPfwB8AXVHjGTYDCjkA+gDoAsrwGCh5ATsxB0YnFw8ALWdjV2EA/iDyELC3dwBNTRgMABM3OMDWcXI2DBg6cjA4vgTWNkqAb3IAAybDY1MoFvIAXzYgA)

@vzarytovskii
Copy link
Member Author

Actually, disregard my question about spaces in the CustomOperationAttribute, since code I wrote in the example is incorrect.

@vzarytovskii
Copy link
Member Author

Ok, this is ready, I decided not to put Experimental attribute on it, since it's too noisy for normal usage. As well as permit parameterless attribute on the following

[<CustomOperation>]
member _.``foo bar`` _ = ()

It will work exactly the same way it works now with

[<CustomOperation("foo bar")>]
member _.FooBar _ = ()

@vzarytovskii vzarytovskii enabled auto-merge (squash) January 2, 2024 19:47
@vzarytovskii vzarytovskii merged commit ec08a4c into dotnet:main Jan 3, 2024
27 checks passed
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.

None yet

5 participants