Skip to content

Move language features from preview to langversion 10 #18708

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

T-Gro
Copy link
Member

@T-Gro T-Gro commented Jun 23, 2025

No description provided.

Copy link
Contributor

github-actions bot commented Jun 23, 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 Jun 26, 2025

@ijklam , @edgarfgp :

I noticed a lot of changes triggered for signature generation and printing due to AllowAccessModifiersToAutoPropertiesGettersAndSetters (from #16861 ).

I will raise a PR just for enabling that to see the impact in isolation, would then kindly ask you for help in identifying if those changes are desired or not.
On a first glance, I think I noticed:

  • 'with get` added to get-only properties in signatures, which is no bad, but also not necessary
  • occassionally added access modifiers
  • Unification of previously 2 get/set property lines in a signature into a common with get,set one ;; noticed this in the case of indexed properties (there might be some special cases when one side is indexed and the other not)

=> I think its OK and it prints a different, but equally correct thing. Just want to be safe ...

@T-Gro
Copy link
Member Author

T-Gro commented Jun 27, 2025

EDIT: I was fully ignorant of the comment which was there, @Martin521 already thought of that scenario upfront.
Issue resolved :))

@Martin521 :
May I ask for your assistance here as well, please?

When moving nowarn from preview to stable, a .fsx behavior broke - it relied on #loading a file which had #nowarn in it, with the assumption that the #nowarn is imported into the same global scope.

Is the break in behaviour here expected and part of breaking changes, or accidental and this feature was (#loading nowarns) was not considered?

    // #nowarn seen in closed .fsx is global to the closure
    [<Fact>]
    member public this.``Fsx.NoError.ScriptClosure.TransitiveLoad16``() =  
        use _guard = this.UsingNewVS()
        let solution = this.CreateSolution()
        let project = CreateProject(solution,"testproject")    
        let thisProject = AddFileFromText(project,"ThisProject.fsx",
                                      ["#nowarn \"44\""
                                       ])  
        let script1 = AddFileFromText(project,"Script1.fsx",
                                      ["#load \"ThisProject.fsx\"" // Should bring in #nowarn "44" so we don't see this warning:
                                       "[<System.Obsolete(\"x\")>]"
                                       "let fn x = 0"
                                       "let y = fn 1"
                                       ])                                                   

        let script1 = OpenFile(project,"Script1.fsx")   
        MoveCursorToEndOfMarker(script1,"let y = f") 
        TakeCoffeeBreak(this.VS)
        AssertNoErrorsOrWarnings(project)
        // AssertExactlyOneErrorSeenContaining(project, "This construct is deprecated. x")  // This is expected for langVersion >= 10.0

@Martin521
Copy link
Contributor

Yes, this by design.

@ijklam
Copy link
Contributor

ijklam commented Jun 27, 2025

* 'with get` added to get-only properties in signatures, which is no bad, but also not necessary

It is not necessary but more explicit.

* occassionally added access modifiers

Could you please show the cases when there are unnecessary access modifiers added?

* Unification of previously 2 get/set property lines in a signature into a common `with get,set` one ;; noticed this in the case of indexed properties (there might be some special cases when one side is indexed and the other not)

It will only unify the getter and setter which has the same indexer type for indexed properties.

图片

And the indexed-and-normal-mixed properties cannot be compiled.

图片

@T-Gro
Copy link
Member Author

T-Gro commented Jun 27, 2025

@ijklam : I will finish the existing block of features first and then open a PR separately for it, to make it more apparent.
Thanks for your explanations so far - I see all of them as legit 👍

@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Jun 27, 2025
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
Status: New
Development

Successfully merging this pull request may close these issues.

3 participants