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.

Sign up for GitHub

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

Jump to bottom

(experimental) forbid setter call on init properties #11564

Closed
wants to merge 8 commits into from
Closed

(experimental) forbid setter call on init properties #11564

wants to merge 8 commits into from

Conversation

smoothdeveloper
Copy link
Contributor

@smoothdeveloper smoothdeveloper commented May 13, 2021 •
edited
Loading

This is to explore addressing fsharp/fslang-suggestions#904

current plan:

  • enrich the checks around 810,tcPropertyCannotBeSet1,"Property '%s' cannot be set" message.
  • look into AssignedItemSetter
  • putting tests around expected error messages.

@smoothdeveloper
Few areas of interest to explore forbidding calling setter on init pr…
293d5cc 
…operties.
@smoothdeveloper
prevent the property assignment syntax to type check, the set_PropNam…
b0cbfc6 
…e still passes through

not sure why I'm having None when TcGlobals is loading the attribute
@smoothdeveloper
Copy link
Contributor Author

The main check is in place, but I still need to fiddle about the way the modreq stuff works, I initially believed it was an attribute.

Main blocker: in the current added test, the csharp compiler doesn't seem to be the right version for handling 'init' keyword:

// TestFramework.fs line 288
let CSC = requirePackage ("Microsoft.Net.Compilers" ++ "2.7.0" ++ "tools" ++ "csc.exe")

@KevinRansom / @brettfo is it something you would update for me in main branch or any other preferred route?

Other questions to nail the code the best for later review:

  • is there any precedent of modreq metadata in our IL reading and TcGlobals infrastructure?
  • AFAIU, unlike how we do with attributes in TcGlobals, kept as BuiltinAttribInfo, are we fine with just string matching (it seems good enough per the way C# compiler does) if there is no infrastructure around modreq
  • since the feature is just a type checking, and there is no runtime implication (setter can still be called by reflection or if the compiler let it go in type checking), do we keep the set_PropertyName escape hatch (it just works since Fixed extra emitted pop when consuming IL types with init properties #11552 is merged) to still be able to call the setter this way?

pieces of information for my own reference:

@brettfo
Copy link
Member

brettfo commented May 14, 2021

@smoothdeveloper That's certainly an old C# compiler. I have no issue with updating to the latest stable of 3.9.0, but I'd like to give @KevinRansom and @TIHan a chance to say if they want to stick with 2.7.0.

If we do update that version, it'll have to be changed both here and here (and possibly here).

@KevinRansom
Copy link
Member

KevinRansom commented May 14, 2021 via email

@brettfo
Copy link
Member

brettfo commented May 17, 2021

I found one more location where the path to csc.exe needs to be updated:

fsharp/eng/Build.ps1

Line 500 in 3cf3076

$env:CSC_PIPE = "$nugetPackages\Microsoft.Net.Compilers\2.7.0\tools\csc.exe"

@smoothdeveloper
Copy link
Contributor Author

Thanks @brettfo / @KevinRansom, I've updated the pointers to csc.exe and it works as I expect.

I'll need to update FCS surface area (will do it on top of #11574 once merged).

Remaining decision is whether we still support set_PropertyName or not.

@TIHan
Copy link
Contributor

TIHan commented Oct 25, 2021

Any updates regarding this? It's still experimental, but what would we need to do to make it past that?

@reinux
Copy link

reinux commented Nov 14, 2021

Might be a bit of an ask, but if the breaking-ness of this change is what's holding it up, it would be nice to have it produce a warning in the meantime. Then I can be sure I'm not shooting myself in the foot.

@smoothdeveloper
Copy link
Contributor Author

@TIHan, if anyone can give it a rebase, I can't work on it right now.

The changes to compiler were functionally OK AFAIR.

@reinux in current implementation, you can still call set_ member instead:

fsharp/tests/fsharp/typecheck/init-property/niceinit.fsx

Line 7 in 5bcccd0

niceinit2.set_Val 5;;

This is open for discussion if we should remove it.

@reinux
Copy link

reinux commented Nov 16, 2021

@TIHan, if anyone can give it a rebase, I can't work on it right now.

The changes to compiler were functionally OK AFAIR.

@reinux in current implementation, you can still call set_ member instead:

fsharp/tests/fsharp/typecheck/init-property/niceinit.fsx

Line 7 in 5bcccd0

niceinit2.set_Val 5;;

This is open for discussion if we should remove it.

By current implementation, I'm assuming you mean the PR, right? Anything that'll help move this PR along 😁

Just a fairly unsophisticated, average user's opinion to add to the discussion:

I do worry a little these days that F# is straying from C# a bit, especially with nullability and such. With C#'s feature set catching up, F#'s biggest pitch is its cleanliness, so although I can see some situations where set_ could be useful, it might make the most sense to try to adhere to C#'s rules a bit.

@KevinRansom KevinRansom reopened this Nov 22, 2021
@smoothdeveloper smoothdeveloper mentioned this pull request Apr 22, 2022
Open
@smoothdeveloper smoothdeveloper mentioned this pull request Jul 7, 2022
Closed
@vzarytovskii vzarytovskii mentioned this pull request Jul 11, 2022
Merged
6 tasks
@vzarytovskii
Copy link
Member

Fixed in #13490

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
F# Compiler and Tooling
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@smoothdeveloper @brettfo @KevinRansom @TIHan @reinux @vzarytovskii