Skip to content

Preview>V10:AllowAccessModifiersToAutoPropertiesGettersAndSetters #18736

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

Merged

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented Jul 7, 2025

No description provided.

Copy link
Contributor

github-actions bot commented Jul 7, 2025

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@T-Gro
Copy link
Member Author

T-Gro commented Jul 7, 2025

@ijklam :

I just got back to this and there is one specific case related to printing (affects error messages) where I am not certain this leads to an improvement. Visible at E_GenericFunctionValuedStaticProp01_fs test, mismatch being:

actual:
(Error 3068, Line 15, Col 10, Line 15, Col 25, "The function or member 'toList' is used in a way that requires further type annotations at its definition to ensure consistency of inferred types. The inferred signature is 'static member list1.toList: ('a list1 -> 'a list) with private get'.")
expected:
(Error 3068, Line 15, Col 10, Line 15, Col 25, "The function or member 'toList' is used in a way that requires further type annotations at its definition to ensure consistency of inferred types. The inferred signature is 'static member private list1.toList: ('a list1 -> 'a list)'.")

The affected simply exposes a private member:

type 'a list1 = One of 'a | Many of 'a * 'a list1
  with
    static member private toList = function
      | One x -> [x]
      | Many(x, xs) -> x :: list1.toList xs

Before, it was reported as a static member private, which is also how it is authered.
After, it is reported as `static member .... with private get' which is not how the code is authored.

Also the affected code is not an auto property - in "layman expectations", it should not be affected by a feature for auto props?

@T-Gro
Copy link
Member Author

T-Gro commented Jul 7, 2025

In this PR, I will see if a small change in niceprint can cover for the use cases which feel worse ( less consistent between authored code and printed result, as one type of measure). Will ping you once the changes are done for your approval.

T-Gro added 3 commits July 8, 2025 13:39
…ertiesGettersAndSetters

The change AllowAccessModifiersToAutoPropertiesGettersAndSetters affected printing of indexed members and also lead to dropping of "with get,set" for auto properties - the current `replace with ` logic has some flaws.

If those are fixed, the behavior can be brought back - pay special attention to indexer related tests, in error messages and generated signatures. Special care also for properties where getter and setter have differing types.
@T-Gro T-Gro marked this pull request as ready for review July 8, 2025 12:22
@T-Gro T-Gro requested a review from a team as a code owner July 8, 2025 12:22
@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Jul 8, 2025
@T-Gro
Copy link
Member Author

T-Gro commented Jul 8, 2025

/run ilverify

@T-Gro T-Gro requested a review from abonie July 8, 2025 14:52
@ijklam
Copy link
Contributor

ijklam commented Jul 8, 2025

The idea of the changes to NicePrint in the original implementation is to make the properties whose getter and setter has different access modifiers can be printed in one line, and the access modifiers are printed before the get and the set. And this modification changed the print behavior to the other cases (the properties ① which has only getter or setter, or ② whose getter and setter has the same access modifier), which might be no wanted.
Such as

type A =
    static member B1 with internal get() = 0 and set (_: int) = ()
    static member val internal B2 = 0 with get, set
    static member val internal B3 = 0 with get

(* Will print
type A =
  static member B1: int with internal get, set
  static member B2: int with internal get, internal set
  static member B3: int with internal get
*)

We can limit the behavior to the other cases if we don't want it, like

type A =
    static member B1 with internal get() = 0 and set (_: int) = ()
    static member val internal B2 = 0 with get, set
    static member val internal B3 = 0 with get

(* Will print
type A =
  static member B1: int with internal get, set
  static member internal B2: int with get, set
  static member internal B3: int with get
*)

@T-Gro
Copy link
Member Author

T-Gro commented Jul 8, 2025

I tried to keep as much of the "desired" behavior in place, the main reverts I had to do are around indexer properties.
I am definitely open if another PR comes in a decides to complete this fully while either keeping tests green or changing them to obviously still correct implementations.

Right now my main concern was to avoid regressions for F# 10 ( loss of information about indexer arguments and their types was one).
For that reason, I had to revert the printing behavior to the older one which can decide to print getter/setter separately on two lines.

I will enable the feature as it stands with this PR to make sure it gets enabled with F# 10.

@T-Gro T-Gro enabled auto-merge (squash) July 8, 2025 19:01
@github-project-automation github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling Jul 10, 2025
@T-Gro T-Gro merged commit 8f042b1 into main Jul 10, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants