WIP: Traits for IL fields

[vzarytovskii]

Description

Implements https://github.com/fsharp/fslang-suggestions/issues/1323, still missing couple of pieces + I don't like how trait info is constructed for members (with get_ and set_) for "property-looking" accessors.

Checklist

• [ ] Test cases added
• [ ] Release notes entry updated:
• [ ] RFC added
• [ ] Add feature flag as an escape hatch for users who need to maintain backwards compatibility.
• [ ] Restrict the use of this feature in fslib
• [x] Decide whether both ldfld and stfld should be supported.
• [ ] (?) Add support for delayed (setters), so the syntax of foo.Field <- "bar" can be used. Currently it's not possible to use it for neither fields nor propertoes (set_* syntax has to be used.

[github-actions[bot]]

:heavy_exclamation_mark: Release notes required

@vzarytovskii,

[!CAUTION] No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

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

You can open this PR in browser to add release notes: open in github.dev

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.100.md	No release notes found or release notes format is not correct

[vzarytovskii]

The problem with implementation is that pickled data format has to be changed, meaning it won't be consumable by old compilers, our options here are:

Option 1. We do pickle new data and we just make it a breaking change. We did it before. We did it in F#7 with new IWSAMs and SRTP syntax.

Option 2. Same as Option 1, but we put a warning for a while, saying "hey, if you use it, compilers with lang version < 9.0 won't be able to consume it".

Option 3. We have another resource for the metadata, like we have for nullables. Downsides of that - well, bigger assembly size, more things to keep in mind when we strip/trim stuff, more room for error when we pickle-unpickle.

Option 4. Same as Option 3, but we have a separate metadata for every feature (or set of features we add). For new SRTP features - one, for anonymous unions - another one, etc. Downsides are the same as 3, but multiplied by number of new resources we have.

Option 5. Don't do anything, don't add new language features which require change of format

Option 5 is non-productive, and included just for completeness, Options 3 and 4 are less risky in terms of backwards compatibility, but will impact assembly size (potentially build times as well), also, as mentioned are error-prone. Options 1 and 2 are the easiest ones, and we did it before multiple times, second one thought can be a bit noisy.

It's not a problem with only thins change, but a problem in general - every time we want to extend pickled data, we need to make such change.

Maybe any opinions from @dsyme @KevinRansom @T-Gro @0101 @gusty @abelbraaksma @baronfel @kerams @Happypig375 @TheAngryByrd?

[T-Gro]

I would choose Option 1 here (was chosen in the past), and be careful not to use it for FSharp.Core

[vzarytovskii]

One more note is that if we decide that from now (if the feature is in) the following trait member Id: int will satisfy not only getters, record fields and anonymous record fields, but also IL fields from now on, it will be a very implicit change in behaviour, which might not be known for the author library.

This is why I can't really decide what shall we do here.

[vzarytovskii]

I think for now, I will go with Option 1 and see if it brings any additional issues with fslib or in tests.

[kerams]

Does the first option imply that we're making a breaking change for this feature only and that we're going to have the same discussion for the next one, or that we're going to accept any format-breaking feature into F# 9 and that we're going to have the same discussion in F# 10 timeframe?

I'm asking because there were at least 2 features (https://github.com/fsharp/fslang-suggestions/issues/670, https://github.com/fsharp/fslang-suggestions/issues/965) that I started implementing and then dropped after it had dawned on me that that a pickling change is required.

[gusty]

Is is feasible/reasonable to implement a lang switch to turn-off specific features that would eventually produce an incompatible binary?

For instance, for F#8 there are a lot of features I would like to use, apart from IWSAMs but I can't because of the breaking change to F#6 consumers and below.

If there was such a switch available I would turn off IWSAMs and be able to use all the other lang features.

[vzarytovskii]

Does the first option imply that we're making a breaking change for this feature only and that we're going to have the same discussion for the next one, or that we're going to accept any format-breaking feature into F# 9 and that we're going to have the same discussion in F# 10 timeframe?

Likely feature-by-feature basis, based on: how likely it's going to break existing library code with "just" recompilation, and can it affect fslib having different metadata.

For the particular features you've mentioned - with generic attributes, it should be fine, since it's unlikely to leak with existing code.

For the quotations - I'm not entirely sure, since I don't have enough knowledge of what it involves.

Problem with current PR is that in some corner cases, we can start finding solutions for IL fields we haven't before (though unlikely in my opinion), which can result in additional metadata written.

[vzarytovskii]

Is is feasible/reasonable to implement a lang switch to turn-off specific features that would eventually produce an incompatible binary?

It is possible for sure, it will, however complicate feature matrix (especially during testing) and will require decoder ring at some point of what features should be turned on or off for code to compile correctly.

[T-Gro]

@kerams :

It depends on the implementation specifics of the pickling change. For both of the suggestions: If the break in pickled format would occur ONLY to users who very explicitly opt-in using the new feature on a major version change, it could go in.

We just do not want silent accidental breaks for people who are not opting in, which might be the case with this Vlad's PR.

[edgarfgp]

I would choose Option 1 here (was chosen in the past), and be careful not to use it for FSharp.Core

Agreed. Option 1

[abelbraaksma]

Maybe any opinions from [...]

Sorry, a bit late to the party, but going over the comments and your options, I think Option 3 and up are not feasible / maintainable in the long run, so imo, these are are a no-go either way.

Option 1 would lead to silent errors. I.e., as @gusty mentioned, you use F# 9 or up to compile your lib, but at some point you get mysterious complaints from users that cannot consume certain functions. This is undesirable. Compiling the same source with a new version of F# should (imo) not lead to binary incompatibility issues.

Which leaves Option 2. Most lib writers for public NuGet available packages will be forever thankful. They'll likely have warnings-as-error anyway and need to be consumable with older versions. Same is true for the warning on FSharp.Core never to use this feature.

In fact, I would argue that we should backport such warning into the compiler for what's commented above as "we did this before".

it will be a very implicit change in behaviour, which might not be known for the author library.

as @vzarytovskii mentioned, this is precisely why I would caution for introducing this entirely silently.

[vzarytovskii]

In fact, I would argue that we should backport such warning into the compiler

Backporting is too much pain, and I personally would avoid it unless it's something hugely breaking.

Which leaves Option 2. Most lib writers for public NuGet available packages will be forever thankful. They'll likely have warnings-as-error anyway and need to be consumable with older versions.

Problem of Option 2 is that it will be too noisy, it will pop up for potentially every singe use of SRTP if it will be finding new cases for fields.

We also don't have notion of TFM version in compiler, we compile agains set of dlls instead, so we won't be able to warn about this.

We have done Option 1 before, for now I will go with it, until this feature is ready for review.

[vzarytovskii]

When dealing with setting fields, we will likely to hit https://github.com/dotnet/fsharp/issues/8899 btw

[abelbraaksma]

Backporting is too much pain, and I personally would avoid it unless it's something hugely breaking.

"backporting" was the wrong term, I actually meant: we should not shy away too easily from adding (level 5?) warnings to places where we missed them previously.

But that's another discussion anyway.

Problem of Option 2 is that it will be too noisy, it will pop up for potentially every singe use of SRTP if it will be finding new cases for fields.

I assumed it would only be raised on the definitional side? But if that's not possible, I understand not wanting to raise warnings.

[vzarytovskii]

Open question: currently instance.Property <- "value" syntax is not supported for both properties and fields in compiler due to how delayed code is generated. set_Property(...) is/should be used instead. Question is - do we fix/support it in this PR or in the separate one?

[github-actions[bot]]

[!CAUTION] Repository is on lockdown for maintenance, all merges are on hold.

[edgarfgp]

Open question: currently instance.Property <- "value" syntax is not supported for both properties and fields in compiler due to how delayed code is generated. set_Property(...) is/should be used instead. Question is - do we fix/support it in this PR or in the separate one?

I think set_Property(...) is good enough as an initial support for this. But if this feature is aiming to land in F# 9 we should include instance.Property <- "value" even if that is in a separate that will be covered by the same RFC.

[vzarytovskii]

Open question: currently instance.Property <- "value" syntax is not supported for both properties and fields in compiler due to how delayed code is generated. set_Property(...) is/should be used instead. Question is - do we fix/support it in this PR or in the separate one?

I think set_Property(...) is good enough as an initial support for this. But if this feature is aiming to land in F# 9 we should include instance.Property <- "value" even if that is in a separate that will be covered by the same RFC.

Issue is it was never supported, so it's nice to have, but is orthogonal to supporting field.

[edgarfgp]

The instance.Property <- "value" syntax might be also be tricky as will be blocked by https://github.com/dotnet/fsharp/issues/8899 ?, as you mention in one of your previous comments. I guess this bug should be fixed first .

[vzarytovskii]

The instance.Property <- "value" syntax might be also be tricky as will be blocked by #8899 ?, as you mention in one of your previous comments. I guess this bug should be fixed first .

Well, they caused by the same "issue", but <- syntax can be supported by workarounding it. They also a bit orthogonal. In other words, we can make <- work without fixing the whole "under-the-hood-struct-copying" issue.