Allow access modifiers to auto properties getters and setters

[ijklam]

Description

Implements this suggestion

Checklist

• RFC added

• Put it under preview flag

• 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:

  • More inlines for Result module. (PR #16106)
  • Correctly handle assembly imports with public key token of 0 length. (Issue #16359, PR #16363)
    *while! (Language suggestion #1038, PR #14238)

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

[github-actions]

❗ 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]

Where should I add tests to?

[vzarytovskii]

Where should I add tests to?

They should go to ComponentTests probably.

[edgarfgp]

Where should I add tests to?

I think this would be a good place https://github.com/dotnet/fsharp/tree/main/tests/FSharp.Compiler.ComponentTests/Conformance/BasicGrammarElements/MemberDefinitions

[ijklam]

Should this also be allowed for abstract properties? Such as:

typeIA=abstractB:intwith get,internal set

[ijklam]

Should this also be allowed for abstract properties? Such as:

typeIA=abstractB:intwith get,internal set

It seems that abstract members doesn't allow access modifiers

[ijklam]

By the way, access modifiers before getter and setter will be ignored and produce a warning in signature files

[ijklam]

will anyone make reviews on this pr?

[vzarytovskii]

will anyone make reviews on this pr?

Yes, we will shortly, we're a bit short-handed now, a bunch of folks are off.

[edgarfgp]

By the way, access modifiers before getter and setter will be ignored and produce a warning in signature files

Would this mean that a private val member with private getter and public setter would be usable from the outside ?

typeA()=member valprivateX=0withpublic get,private set
leta= A()
a.X // ? 

If that is the case we should add a compiler error saying that there are conflicting access modifiers.

[ijklam]

typeA()=member valprivateX=0withpublic get,private set

typeA()=// Error: When the visibility for a property is specified, setting the visibility of the set or get method is not allowed.member valprivateX=0withpublic get,private set
// Okmember valX=0withpublic get,private set
// Okmember valprivateX=0with get, set

member val X = 0 with public get, private set in signature files should be written as following, and can be usable from outside of A

typeA=new: unit -> A
memberpublicD:int with getmemberprivateD:int with set

[ijklam]

I found that in ILSpy, member val private A = 0 with get, set and member val A = 0 with internal get, private set both produce a internal property. Is it correct behavior?

internalintA{[CompilerGenerated][DebuggerNonUserCode]get{return A@;}[CompilerGenerated][DebuggerNonUserCode]set{
A@ = value;}}

[vzarytovskii]

I found that in ILSpy, member val private A = 0 with get, set and member val A = 0 with internal get, private set both produce a internal property. Is it correct behavior?

internalintA{[CompilerGenerated][DebuggerNonUserCode]get{return A@;}[CompilerGenerated][DebuggerNonUserCode]set{
A@ = value;}}

Yes, current compiler always emits everything (private and internal) as internal.

[ijklam]

@vzarytovskii Still has a test failed.

Failed FSharpChecker.TransparentCompiler.File is not checked twice [291 ms] Error Message: Assert.Equal() Failure Expected: FSharpList<JobEvent> [Weakened, Requested, Started, Finished] Actual: FSharpList<JobEvent> [Weakened, Requested, Started] Stack Trace: at FSharpChecker.TransparentCompiler.File is not checked twice() in /home/vsts/work/1/s/tests/FSharp.Compiler.ComponentTests/FSharpChecker/TransparentCompiler.fs:line 242 at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor) at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr) 

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[vzarytovskii]

Good stuff as always, thanks!

[psfinaki]

Good stuff, great testing. Sorry for nitpicking

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[psfinaki]

/azp run

[github-actions]

Caution

Repository is on lockdown for maintenance, all merges are on hold.

[auduchinok]

By the way, access modifiers before getter and setter will be ignored and produce a warning in signature files

Why? Would it be possible to support signatures too?

member val X = 0 with public get, private set in signature files should be written as following, and can be usable from outside of A

Can we fix this and allow the same syntax, please?

@vzarytovskii I think we should not release the feature without a proper signature support.

[ijklam]

By the way, access modifiers before getter and setter will be ignored and produce a warning in signature files

Why? Would it be possible to support signatures too?

That might so much thing to change... It also relates to the generation of signature files.