Issue 17269: formattedWrite of struct with Nullable value fails

[CromFr]

The bug was caused by formatElement implicitly converting Nullable!string to string, that resulted in an error when the Nullable was null.

New behavior is to use the toString method when available, instead of implicitly converting the value.

[dlang-bot]

Thanks for your pull request and interest in making D better, @CromFr! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	17269	normal	formattedWrite of struct with Nullable string fails

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#5797"

[atilaneves]

Why not use a struct with Nullable!string?

[CromFr]

If I use a Nullable!string, it causes an assert failure on another unittest block

expected = `void delegate() @system`, result = `void delegate()`

I don't really understand why, so I wrote a custom struct to:

• Show the alias this and the implicit conversion problem
• Do not depend on Nullable if it is changed in the future

[wilzbach]

Huh? Nullable!string works without your PR: https://run.dlang.io/is/Jjzpvo

[wilzbach]

Could you provide a test case where it fails and maybe open another bug report?

[CromFr]

you need to put the Nullable!string inside a struct to see it fail:

struct NullableStr {
   Nullable!string value;
}
NullableStr f;
format("%s", f).writeln;

[MetaLang]

This should've been fixed by #2587... Looks like this is a case that was missed by the tests. I don't have time to investigate at the moment, but I don't think this is the correct solution so please don't pull this yet.

[MetaLang]

Urgh, it's all StringTypeOf's fault. That template is way too permissive.

Looks like formatValue uses this same method so we might as well keep it consistent. Sorry for the noise.

[CromFr]

fyi the first fix I thought of was to modify StringTypeOf to be false if the struct is implicitly convertible to string using an alias this to a function that can throw.
But since Nullable does an assert, it wouldn't work (and modifying StringTypeOf has too many consequences)

[wilzbach]

Looks reasonable to me as well, though as noted Nullable!string already works.

[wilzbach]

Reworded the commit message, s.t. the dlang bot recognizes it and it can be auto-closed and part of the changelog (and also rebased to upstream/master).

[wilzbach]

I added the initial example of as an additional unittest example - this is considered a good practice.
Anyhow now we are failing due to:

core.exception.AssertError@std/format.d(4097): expected = `void delegate() @system`, result = `void delegate()`

[CromFr]

I encountered the same unittest error when initially trying to fix formattedWrite

Since it didn't make much sense to me, I changed the test to use a custom struct that has the same problem as Nullable!string and the unittest error disappeared, so I concluded that it was because of a weird import conflict or something like that, without much more investigations

[quickfur]

Rebased. Hopefully it will go through this time.

[quickfur]

While it would be nice to track down the exact cause of the error caused by testing Nullable directly, I don't think that should hold up this PR. LGTM.

[quickfur]

Hmph. Seems that avoiding Nullable doesn't fix this problem. :-( Gotta get to the bottom of that @system delegate issue.

Update: Nevermind, it is the Nullable unittest that's triggering this problem. Why, is beyond me. Gonna keep looking...

[quickfur]

I think there's a compiler bug associated with this. Moving the offending unittest above the unittest that imports Nullable fixes the problem. Unfortunately, it's a rather elusive one, as adding version(none) to preceding unittests makes the problem go away. Very strange.

[CromFr]

I think dlang-bot should add a message when the PR becomes un-mergeable. I didn't receive any notification on this :/

[quickfur]

@CromFr BTW, I already rebased your pull, you may want to fetch from dlang/phobos to update your local branch if you're planning to adding more commits.

[quickfur]

Managed to reduce the bug to a minimal(-ish) case: https://issues.dlang.org/show_bug.cgi?id=18269

A truly bizarre compiler bug where unrelated declarations can affect the type of a delegate!

[MetaLang]

That's a nasty one. Thanks for the reduction @quickfur.

Anyone have a suggestion for how to proceed? I don't want this to fall by the wayside because of an obscure compiler bug.

[quickfur]

One way to deal with this, if it's deemed urgent to merge this PR sooner rather than later, is to version out the unittest with a comment indicating that it's being blocked by issue 18269. That way, once 18269 is fixed we can turn on the unittest again.

[wilzbach]

How about moving the unittest to a different module?

[quickfur]

On Tue, Jan 23, 2018 at 07:11:46PM +0000, Sebastian Wilzbach wrote: How about moving the unittest to a different module?

[...] I suppose we could try that too. No idea whether it will work, though. The compiler bug seems highly sensitive and fragile to small changes. I'd like to dig to the bottom of it sometime, but it's deep in the complicated and messy IFTI code, and I'm not sure I'll be able to figure it out!

…

--T

[quickfur]

In the meantime, perhaps we could change the order of the offending unittests so as the bypass the compiler bug? I'm not sure when the compiler bug will be fixed; it's a pretty tricky one involving an ambiguity in the global stringtable.

[wilzbach]

Yeah that's why I thought that putting it in a separate module will help as it reduces the chances of someone accidentally running into this in the near future again.

[quickfur]

Unfortunately, the problem is cross-module. The original problem was caused by the import of std.variant, which in turn contains a template argument containing void delegate(). Basically, there's some kind of ambiguity in the compiler's stringtable, where both void delegate() and void delegate() @system have the same deco in their mangling, and whichever gets seen by the compiler first gets into the stringtable, and from that point on becomes the "official" string representation of that type. So no, putting it in a different module won't help as long as the order of unittests (as seen by the compiler) is wrong. See: https://issues.dlang.org/show_bug.cgi?id=18269

[berni44]

@RazvanN7 With #7771 fixing the problem, after fixing the merge conflict (formatspec is meanwhile scope), IMHO this can be merged now.

[RazvanN7]

@CromFr Please rebase this and we will get it in.

[CromFr]

Should be good now :)

[berni44]

@RazvanN7 circleci seems not to respond... Is there anything, that can be done?

[RazvanN7]

Superweird. I can't even access the details page.

[RazvanN7]

@berni44 I just restarted it. Let's hope it doesn't hang again.