WIP: Traits for IL fields #16481
WIP: Traits for IL fields #16481
Conversation
❗ Release notes requiredCaution 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:
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
|
|
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? |
|
I would choose Option 1 here (was chosen in the past), and be careful not to use it for FSharp.Core |
|
One more note is that if we decide that from now (if the feature is in) the following trait This is why I can't really decide what shall we do here. |
|
I think for now, I will go with Option 1 and see if it brings any additional issues with fslib or in tests. |
|
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 (fsharp/fslang-suggestions#670, fsharp/fslang-suggestions#965) that I started implementing and then dropped after it had dawned on me that that a pickling change is required. |
|
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. |
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. |
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. |
|
@kerams : It depends on the implementation specifics of the pickling change. 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. |
| @@ -1435,6 +1436,8 @@ let p_trait_sln sln st = | |||
| p_byte 6 st; p_tup5 p_ty (p_option p_ILTypeRef) p_ILMethodRef p_tys p_ty (a, b, c, d, e) st | |||
| | FSMethSln(a, b, c, Some d) -> | |||
| p_byte 7 st; p_tup4 p_ty (p_vref "trait") p_tys p_ty (a, b, c, d) st | |||
| | ILFieldSln(a, b, c, d) -> | |||
There was a problem hiding this comment.
You;ll also need a producing case in u_trait_sln
There was a problem hiding this comment.
Yep, couple of things are missing here
Agreed. Option 1 |
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 In fact, I would argue that we should backport such warning into the compiler for what's commented above as "we did this before".
as @vzarytovskii mentioned, this is precisely why I would caution for introducing this entirely silently. |
Backporting is too much pain, and I personally would avoid it unless it's something hugely breaking.
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. |
|
When dealing with setting fields, we will likely to hit #8899 btw |
"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.
I assumed it would only be raised on the definitional side? But if that's not possible, I understand not wanting to raise warnings. |
|
Open question: currently |
|
Caution Repository is on lockdown for maintenance, all merges are on hold. |
I think |
Issue is it was never supported, so it's nice to have, but is orthogonal to supporting field. |
|
The |
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 |
Description
Implements fsharp/fslang-suggestions#1323, still missing couple of pieces + I don't like how trait info is constructed for members (with
get_andset_) for "property-looking" accessors.Checklist
ldfldandstfldshould be supported.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.