Breaking change for DU with [<RequireQualifiedAccess>] formatting in 4.7.2

[atsapura]

In FSharp.Core 4.7.0 the name of Discriminated Union is ignored in text formatting even if [<RequireQualifiedAccess>] is present.

However in 4.7.2 this name is added as a prefix for union case.

Repro steps

1. Use FSharp.Core 4.7.2
2. Declare a union type with RequireQualifiedAccess attribute:

[<RequireQualifiedAccess>]
type ResponseGroup =
        | ItemLarge
        | None

3. Format it using %A as in sprintf "%A" ResourceGroup.None

Expected behavior

Snippet above yields "None" as it would in 4.7.0

Actual behavior

It yields "ResponseGroup.None"

Known workarounds

• Override ToString() and use %O instead of %A
• Remove RequireQualifiedAccess if that's applicable.
• Rollback to FSharp.Core 4.7.0

Related information

• Windows 10 Pro
• Net core 3.1

[forki]

Yes it's weird that this changes with fsharp.core update and not sdk update.

[cartermp]

@forki confusingly (as is always the case with anything related to FSharp.Core), formatting with %A is not actually implemented in the compiler but is in a file shared with FSharp.Core. So an FSharp.Core update will lead to these changes.

This is due to #7710, which I'm inclined to keep in. It also changes the output of cases that use UseNullAsTrueValue and the formatting of tuples. I think it's a good change overall, though it is breaking if you depend on the output of %A for a construct. This is akin to depending on reflection internals though, which is why I don't feel strongly that this should specifically be reverted.

[forki]

I don't think it should be reverted. We already changed all our code and I consider it my own mistake to rely on tostring for output. But I think we should have communicated it better. I think this will hit a lot of people. Phillip Carter <notifications@github.com> schrieb am Mo., 1. Juni 2020, 20:58:

…

@forki <https://github.com/forki> confusingly (as is always the case with anything related to FSharp.Core), formatting with %A is not actually implemented in the compiler but is in a file shared with FSharp.Core. So an FSharp.Core update will lead to these changes. This is due to #7710 <#7710>, which I'm inclined to keep in. It also changes the output of cases that use UseNullAsTrueValue and the formatting of tuples. I think it's a good change overall, though it is breaking if you depend on the output of %A for a construct. This is akin to depending on reflection internals though, which is why I don't feel strongly that this should specifically be reverted. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9357 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOANHSDQ7OUL6LOPRBME3RUP265ANCNFSM4NP7SEKA> .

[forki]

It's especially unfortunate that it was changed in a Bugfix release Steffen Forkmann <sforkmann@gmail.com> schrieb am Mo., 1. Juni 2020, 22:00:

…

I don't think it should be reverted. We already changed all our code and I consider it my own mistake to rely on tostring for output. But I think we should have communicated it better. I think this will hit a lot of people. Phillip Carter ***@***.***> schrieb am Mo., 1. Juni 2020, 20:58: > @forki <https://github.com/forki> confusingly (as is always the case > with anything related to FSharp.Core), formatting with %A is not > actually implemented in the compiler but is in a file shared with > FSharp.Core. So an FSharp.Core update will lead to these changes. > > This is due to #7710 <#7710>, which > I'm inclined to keep in. It also changes the output of cases that use > UseNullAsTrueValue and the formatting of tuples. I think it's a good > change overall, though it is breaking if you depend on the output of %A > for a construct. This is akin to depending on reflection internals though, > which is why I don't feel strongly that this should specifically be > reverted. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#9357 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAOANHSDQ7OUL6LOPRBME3RUP265ANCNFSM4NP7SEKA> > . >

[atsapura]

@cartermp
I don't insist on reverting it or any other course of action, but to give you the better picture, it didn't break any code, but we used it for url building, so it broke communication between applications rather than 1 application.

Another thing to consider - it would probably be nice to have a way of serializing just a union name even when RequireQualifiedAccess is set, since code behavior is one thing and string representation is another. Besides, existing json serializers, as I understand, will continue serialize them as cases (e.g. FSharpLu.Json in compact mode), rather then putting type name as prefix as well, so it's lost consistency there too.

[abelbraaksma]

I was under the impression that #7710 only addressed %A formatting, but I just noticed that it also changed the default for %O (on Slack I actually advised someone to use %O without mentioning to override).

A lot of stuff depends on ToString(). I agree that things shouldn't depend on %A', which is kinda like asking the compiler to create some form of useful output, but ToString() should be well-defined.

Is it possible that this was inadvertent? I don't see it mentioned in the PR at all, it only seems to address %A in the description.

[forki]

It definitely also changed ToString Abel Braaksma <notifications@github.com> schrieb am Di., 2. Juni 2020, 00:52:

…

I was under the impression that #7710 <#7710> only addressed %A formatting, but I just noticed that it also changed the default for %O (on Slack I actually advised someone to use %O without mentioning to override). A lot of stuff depends on ToString(). I agree that things shouldn't depend on %A', which is kinda like asking the compiler to create some form of useful output, but ToString() should be well-defined. Is it possible that this was inadvertent? I don't see it mentioned in the PR at all, it only seems to address %A. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9357 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOANGLK7YWM5XOXJVBMRDRUQWJBANCNFSM4NP7SEKA> .

[charlesroddie]

@abelbraaksma they are the same.

type DU = | Case

SharpLab gives:

[CompilerGenerated]
public override string ToString()
{
    return ExtraTopLevelOperators.PrintFormatToString(new PrintfFormat<FSharpFunc<DU, string>, Unit, string, string, DU>("%+A")).Invoke(this);
}

Bad architecture which will have to change as AOT and linkers become more common.

[abelbraaksma]

@charlesroddie, I see, I didn't check the auto-generated code. That suggests that overriding ToString still works and will change the output when used with %O. At least there's a workaround if people want the old style back, but it'll require some work.

(I habitually add ToString and ToDebuggerDisplay, unless for scripting etc, so I'm not affected)

Btw & OT, this default ToString implementation is also very slow, I'm sure we can do better?

[cartermp]

The default ToString should ideally not be using reflection. Unfortunately, reflection was introduced rather virally in the past, and it will be a challenge to root it out.

[isaacabraham]

Ouch, This has the potential to have lots of breaking changes which will only be found at runtime.

[isaacabraham]

I actually think that this behaviour should be reverted. Regardless of whether people should be using ToString() or not, I see it in the wild often.

[isaacabraham]

In fact we've just identified one of our projects that has lots of RQA DUs and, yes, we use ToString() on it to generate lookup keys. This would break that catastrophically.

[charlesroddie]

we use ToString() on it to generate lookup keys.

@isaacabraham I assume you are using DUs with no data, which are like enums. The change to include the type name adds a bit of inconsistency compared to enums. Enums also require qualified access but don't display the type name:

type E = | A = 0
E.A.ToString() // "A"

I would agree it should be reverted as it doesn't bring any significant advantage and breaks some people.

[abelbraaksma]

I would agree it should be reverted as it doesn't bring any significant advantage and breaks some people.

I would opt for %A as it is now, as that has always been decidedly more of a "try your best, give me something useful" and imo should include the RQA moniker, and %O and ToStringas it was, as I reckon that's the main backwards compat issue and affects serializing.

That would also give us the best of both worlds.

[Shmew]

Yeah just came across this when updating a work project caused all my tests to start failing. I do think it makes sense to include the type when it has the attribute, but at the same time this was not something you'd expect when updating a patch version.

[isaacabraham]

@cartermp is there a reason why this won't be reverted?

[cartermp]

Yes - although it was a breaking change, it was made and shipped in an LTS of Visual Studio (VS 16.7) and changing it would be another breaking change. Considering the folks here have all reacted to the change, it's just more churn.

[isaacabraham]

@cartermp that's certainly one way of looking at it. The flipside is:

• The folks that are unaware of the change i.e. those that are not aware of this could be adversely affected
• The "double churn" is a good point - but I'd suggest that a "temporary" breaking change that lasted for e.g. 3 months (possibly a hot fix could be released that resolved this?) might be preferable and reduce the chance of people falling foul of this compared to the current state.
• Appreciate that it went out with an LTS release but - well, so what? If it's a breaking change, it's a breaking change - it should be reverted. Especially as this is a runtime breaking change, not a compile time one. This pattern of using ToString() for DUs is not an unusual pattern.

All IMHO.

[atsapura]

@cartermp
The biggest problem I see is that now in core lib there's now way to get previous behavior -- not with ToString() nor with %A. Also I think @isaacabraham has a good point -- 3 months of a breaking change is better (IMHO at least) than forever breaking.

[ForNeVeR]

I would also be for reverting the breaking change.

Regarding the point on change being included into LTS version of Visual Studio, I have a couple of points, too.

1. VS 16.7 is pretty new, right? How many people are already on it, is it that much? How many will stuck on this exact release later? And even if there're much people, which means they're so eager to update, then it's because they trust us to not make the breaking changes in the language.
2. How hard would it be for these people who're stuck with this LTS release to update to a new FSharp.Core? LTS means there will be 16.7.1, 16.7.2 etc., right? So, if we consider this change as a bug and release an update, then it will eventually be picked up in the LTS branch, too. Right?
3. And even so: AFAIK, you could freely update the version of FSharp.Core if you need, so people could easily update to a version with a bugfix.

Whatever the decision here will be, you can already see this was a somewhat sensitive change. So I'd say we have to define this behavior in the spec, and follow the spec in future. Even if you insist on marking this behavior as "undefined" (ah, these C++ vibes) before this change vent live, then we better write a spec for the new behavior.

[7sharp9]

The behaviour in 4.7.2 is how I would expect it to work, if there's a require qualified attribute then you would expect the type name to be there. The previous behaviour seem to be a bug to me.

[charlesroddie]

@7sharp9 I don't think the current behaviour is a big problem, since string functions are broken in F# so shouldn't be relied on, but it doesn't seem consistent to me, because Enums have usage very similar to DUs with RQA but don't print the type name, and using RQA on modules doesn't inject the module name into the ToStrings of contents.

type E = | A = 0
E.A.ToString() // "A", not "E.A"

[<RequireQualifiedAccess>]
module M =
    type DU = | Case
M.Case.ToString() // doesn't start with "M."

[Shmew]

I don't care either way as long as it's consistent, if we're keeping this behavior IMO we should change anything that uses [<RequireQualifiedAccess>] to prefix the module/type/whatever name.

since string functions are broken in F# so shouldn't be relied on

@charlesroddie what do you mean?

[isaacabraham]

@7sharp9 Whether or not it's considered a bug in previous behaviour is more or less irrelevant IMHO. The main thing is that it's a breaking change that's been slipped in without any advance warning. You won't get a compiler break but a runtime error.

Imagine you're storing this in a database in a serialised form. The only time you'll realise this is when reading data back in.

[atsapura]

@cartermp Maybe there should be a way to configure this behavior with a parameter, e.g.

type RequireQualifiedAccessBehavior = CodeOnly | CodeAndString

[<RequireQualifiedAccess(CodeOnly)>]
type CodeOnlyExample = One | Two
let codeOnlyString = sprintf "%O" CodeOnlyExample.One // yields "One"

[<RequireQualifiedAccess(CodeAndString)>]
type CodeAndStringExample = One | Two
let codeOnlyString = sprintf "%O" CodeAndStringExample .One // yields "CodeAndStringExample.One"

As for empty constructor I suggest leave the previous behavior since it's a breaking change.
But I don't insist, at this point I just want to have a way to get previous behavior without reinventing the wheel myself.

[cartermp]

Thinking about this some more I think it's best to revert this specifically for the union cases. #10083 is the PR - I'll re-open this until it is merged.

[dsyme]

My comment in #10083

---

I think we should be more nuanced here

1. I can see the case for reverting for generated ToString() - that should not have changed

2. I'm less keen on reverting for %A. We make no guarantees about %A except that the output is human readable, and the new output is definitely more human readable. .

3. I'm against reverting for FSI stdout output printing (which the current PR is doing). That has to prioritise human readability and also the ability to round-trip some data, so the qualifications should stay

Technically separating 1/2 from 3 is simple. Separating 1 from 2/3 is a little harder (it needs another flag passed through to printf in the format string, the only way to plumb such flags through).

[abelbraaksma]

@dsyme, I agree, in fact, your comment mirrors my observation early in this thread (#9357 (comment)), where I notice that the original issue that caused this was only intended to change %A, and not %O (see #7710).

I think it makes sense, it's much more common that serialization in part relies on ToString, it is less common, and probably should be added to the docs as advisory, that it depends on %A, a format that has changed several times in subtle ways over the years.

Furthermore, making this distinction allows users choice over both formats, giving easy transition were needed, and both can be independently overridden in DUs and other classes (ie, StructuredFornatDisplay iirc, and overriding ToString resp.).

Currently, the default ToString implementation calls %A, if we just emit different code there, that'd fix this, no?

[charlesroddie]

the default ToString implementation calls %A, if we just emit different code there

This is very important to do. I've posted an issue - sorry a little long -, that suggests doing this, i.e. generating specialized ToString() code on record/DU types.