Fix Issue #17712 - Add test cases

[ntrel]

This bug seems to have been fixed, so this adds the 2 test cases to the test suite. Those test cases were causing a link error about missing symbols.
https://issues.dlang.org/show_bug.cgi?id=17712

[dlang-bot]

Thanks for your pull request and interest in making D better, @ntrel! 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
✓	17712	regression	[REG 2.074] [LINK] Undefined reference to std.conv.toChars!(10, char, 1, uint).toChars(uint)

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

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 + dmd#11300"

[PetarKirov]

It would be interesting to bisect the dmd commit which fixed this bug. In any case, nice catch @ntrel!

[PetarKirov]

Hmm, I just saw that the second test case uses phobos. I think it's also likely that a change to phobos between v2.081 and v2.092.0 could have changed the outcome of the test case. @ntrel do you think you could try to minimize the second test case, so it doesn't depend on phobos? That way no changes to phobos could not affect this test in the future.

[schveiguy]

I think the point is that nobody knows what causes the bug if it still exists. So it's difficult to find a test case that doesn't depend on Phobos when we can't get the bug to trigger with the actual code that used to trigger it.

Technically I would think this test goes into Phobos, but I don't know if Phobos has infrastructure to run tests like DMD does. Are there no test cases in dmd that depend on Phobos?

The point of this test is to prevent changes to Phobos causing this problem to reoccur.

[MoonlightSentinel]

There are some tests but we are trying to get rid of Phobos inside of the test suite to make those tests reliable.

[MoonlightSentinel]

No phobos addtions to the test suite

(I'll try to find a reduced test case using dustmite)

[schveiguy]

I'm happy to have someone change to a test that doesn't use phobos. But the problem I see is, the test doesn't fail, and we don't know what circumstances trigger the bug. So any test we add that isn't exactly like the original tests that failed gives us little chance to find the bug later if it happens to reoccur.

If we are against tests that use Phobos more than testing for regressions, then we should simply remove the test, not insert a test that nobody knows whether it's effective.

[ghost]

digger reports 2nd case was fixed by

digger: c2eefb0d8d850715f492197b0b1a033d9ad5411a is the first good commit
commit c2eefb0d8d850715f492197b0b1a033d9ad5411a
Author: Razvan Nitu <razvan.nitu1305@gmail.com>
Date:   Sat Apr 27 15:22:34 2019 +0300

    dmd: Merge pull request #9636 from JinShil/dep_visible
    
    https://github.com/dlang/dmd/pull/9636
    
    Remove visiblity and lookup deprecation (Take 2)

diff --git a/dmd b/dmd
index ff11ca9a2..d51135331 160000
--- a/dmd
+++ b/dmd
@@ -1 +1 @@
-Subproject commit ff11ca9a275b7bd0c017fe9815b24cef89d6dd50
+Subproject commit d51135331f26ac43dedf30a789f3ce5a06fc57ff
digger: Bisection completed successfully.

and first by (but not relevant as it's a phobos workaround)

digger: 88546219d3aaecab4e5c31f6e22597a944a7e138 is the first good commit
commit 88546219d3aaecab4e5c31f6e22597a944a7e138
Author: The Dlang Bot <code+dlang-bot@dawg.eu>
Date:   Fri Sep 7 22:20:09 2018 +0200

    phobos: Merge pull request #6659 from shove70/patch-1
    
    https://github.com/dlang/phobos/pull/6659
    
    Fix Issue 17712 - [REG 2.074] [LINK] Undefined reference to std.conv.toChars!(10, char, 1, uint).toChars(uint)
    merged-on-behalf-of: Nathan Sashihara <n8sh@users.noreply.github.com>

diff --git a/phobos b/phobos
index f03cce20e..217c9af28 160000
--- a/phobos
+++ b/phobos
@@ -1 +1 @@
-Subproject commit f03cce20ea9a67d560331f3358ee774eb1516303
+Subproject commit 217c9af28f6fc9a4f8dc6f11d0eae0ed58805c43
digger: Bisection completed successfully.

[ibuclaw]

I'm happy to have someone change to a test that doesn't use phobos. But the problem I see is, the test doesn't fail, and we don't know what circumstances trigger the bug. So any test we add that isn't exactly like the original tests that failed gives us little chance to find the bug later if it happens to reoccur.

This is what dustmite is for. One can simply get an old version of dmd/phobos with the bug (in a container if need be) and reduce the test until it is stripped of all external dependencies.

[ntrel]

I don't know if Phobos has infrastructure to run tests like DMD does.

I don't think so. I had a look at porting dmd/test to Phobos but it's a bit complicated.

digger reports 2nd case was fixed by

#9636

Great!

[ghost]

given the dmd PR found by digger (#9636) and assuming its diagnostic is correct, my analysis is that speculative instantiation failed because of the bug that gave access to private members of public imports. So given a public and a private overload, the one actually used was not marked as necessary to be codegen-ed and the one that was not used was marked. If this is true then the tests proposed by this PR are not useful because for example a regression affecting 17712 would be detected if this test, added along with #9636, starts regressing.

[ntrel]

So I made a pull to put the test cases in Phobos:
dlang/phobos#7536
But there it seems b17712.d is failing even without reverting #6659! Here it seems to pass OK.

[ntrel]

Also, Kagamin has a reduced test case for b17712.d, haven't tried it:
https://forum.dlang.org/post/tajisdafczvdabngaaun@forum.dlang.org

[MoonlightSentinel]

Dustmite found a test case (using DMD 2.082.1) that still fails today:
https://issues.dlang.org/show_bug.cgi?id=17712#c21

[schveiguy]

Nice. We should probably close this for now then. And I wonder if that shouldn't just be a new bug report to refocus energy on it.