Fix broken --precompiledLib switch

[DunetsNM]

This fixes two problems that broke --precompiledLib switch after upgrading from Fable 3 to Fable 4, both caused by change in data format that affects deserialization of precompiled inline_exprs_0 json files.

• MemberRefInfo.Attributes is a sequence of abstract Attribute type, which can't be deserialized even when collection is empty. Build crashes on attempt to link precompiledLib with:

error EXCEPTION: Deserialization of interface types is not supported. Type 'Fable.AST.Fable.Attribute'. Path: $[0] | LineNumber: 0 | BytePositionInLine: 2.
at System.Text.Json.ThrowHelper.ThrowNotSupportedException(ReadStack& state, Utf8JsonReader& reader, NotSupportedException ex)

• ValueKind.NumberConstant stores the value as obj which deserializes from precompiledLib as ... JsonElement, compilation proceeds but at the end fails complaining that JsonElement is not a good type for a numeric value (rightfully so).

This pull request fixes both issues (at least now project builds with --precompiledLib switch)

[DunetsNM]

while I'm here, maybe it makes sense to separate part of Fable.AST module that meant to be json-serializable from everything else? How to?

[ncave]

@DunetsNM Please keep in mind that any Fable AST change is a breaking change for plugins so it requires a major version update.

[DunetsNM]

@DunetsNM Please keep in mind that any Fable AST change is a breaking change for plugins so it requires a major version update.

@ncave Should I change the version in Fable.AST.fsproj right in this PR ? Judging by history version increases committed separately / along with releases

If it has to be in this PR , I suppose we keep major version at 4 (as it seems to be anchored to Fable 4 ?) i.e. 4.4.0 => 4.5.0 ? Again, here I assume that based on previous history (quick search reveals examples of technically breaking changes that were not versioned by a strict SemVer e.g. see diff between "Release 3.7.1" and "Release 3.7.2" commits , it has AST breaking changes in diff)

[DunetsNM]

@ncave I restored the char/number workaround in its original, tests should pass now. I tried to fix it via TypeCast which broke the build and found myself in a rabbit hole trying to understand how it works. So sticking with good old "if it works don't touch it"

[ncave]

@DunetsNM See DunetsNM#1

[DunetsNM]

@ncave I fixed style warnings, don't have anything else to push here. Let me know if you want me to tidy up anything.

[ncave]

LGTM, the only outstanding issue is the (potential) breaking change for Fable plugins, as mentioned above.
Not sure what semver change that requires.

++ @MangelMaxime, if you know of any list of Fable plugins (looking up in GitHub only brings very old ones that probably have to be updated anyway to work with Fable 4).

[DunetsNM]

These plugins were updated to Fable 4 (I use a fork of those) however they shouldn't break as they don't inspect Attributes or NumberConstant case directly

https://github.com/Zaid-Ajaj/Feliz/tree/master/Feliz.CompilerPlugins

[MangelMaxime]

The only Fable plugin I am aware of is Feliz.CompilerPlugins.

If the tests are passing then I think it means that this plugin does not need an update because we have it tested as part of tests/React/Fable.Tests.React.fsproj

For the version in the past, I upgraded only the major version so in this case it would become 4.5.0.

[DunetsNM]

@MangelMaxime @ncave gentlemen when can I expect a new Fable nuget to be published (with this fix)? Don't want to be pushy, just wondering if I have time to wait for it or rather should make a private nuget from my fork in the meantime

[MangelMaxime]

I was looking into the PR and I have questions.

MemberRefInfo.Attributes is not the only place which currently have the signature abstract Attributes: Attribute seq.

For this reason, I suspect there are still situation where the --precompiledLib could still fails, no ?

Reading the exception error it seems like the problem is that the problem is because Attribute is an interface?

type Attribute =
    abstract Entity: EntityRef
    abstract ConstructorArgs: obj list

By looking into the Fable.AST this is not the only interface we have does that mean we don't have the problem with the others interfaces because the generated JSON doesn't include all the AST but only a subset of the AST which consist only of records, DUs and primitives types ?

Should we try to use a record instead of an interface for type Attribute? This way we would have the same signature for all Attributes: ... members/properties ?

Looking online, it also seems possible to create a custom converter to teach System.Text.Json how to deal with an interface. However, I am not sure how well it would played with the abstract ConstructorArgs: obj list property.

I am asking all this questions, in case we have a way to fix the issues in a consistent way and also without loosing information in the AST. I unsure if this is important or not, if you should it does not harm or it is too complex to do please feel free to tell me.

[DunetsNM]

because the generated JSON doesn't include all the AST but only a subset of the AST which consist only of records, DUs and primitives types ?

I believe this is the case. Serialized json data has type of (string * Fable.InlineExpr)[] which consists only of primitive and F# algebraic types. I did only informal analysis, perhaps we can write a property-based test that roundtrips random data?

Addition of Attributes is traced back to #3480 fix and introduction of obj in NumberConstant is the fix of #2614

[DunetsNM]

Speaking about losing the information in AST - my impression is that full information about the Attributes is not required in AST. I don't know if there's already 3rd party code that relies on the full Attributes info (probably not) but it was definitely an overkill for its original purpose: they were bolted on just to detect presence of Fable.Core.ParamObjectAttribute.
Arguably it could be even less coupling than that e.g. HasParamObjectAttribute: bool, I'd prefer to expose bare minimum in published types and enrich it only when there's a use case for it.

In similar manner NumberConstant was relaxed to a have obj field only to hack in char value into it (which is replaced with a cleaner way here thanks to @ncave )

[MangelMaxime]

Addition of Attributes is traced back to #3480 fix and introduction of obj in NumberConstant is the fix of #2614

I now remember indeed working on this issue (I mean git history doesn't lie 🤣).

And indeed, I used the existing Attribute interface because it was available and never though it would break something as the CI passed etc.

I agree that I don't think people rely on the ConstructorArgs information. Fable plugin is not something really common. Because, this PR don't break anything at least according to the CI I think we can merge it @ncave was already ok with it yesterday.

I will probably just add a comment on the AttributeFullNames property to link back to this PR.

Thanks a lot for the time you spend on this issue.

[MangelMaxime]

@DunetsNM This is now available in Fable 4.18.0