Fix Issue 18651 - ice: assert in glue.d:777 when building these three…

[BorisCarvajal]

… trivial files

The problem was that some functions were not with sematic3 pass done because their symbol parent was deferred and runDeferredSemantic3() wasn't run. In this case were two auto generated functions (__xopEquals, __xtoHash that start only with semantic2) of some instantiation of SortedRange.

The test case on bugzilla uses Phobos and consumes 550MB of RAM, moreover, I think crafting another reduced case could be really time consuming, just in this example there were like ten thousand symbols deferred but still only two functions left over.

[dlang-bot]

Thanks for your pull request and interest in making D better, @BorisCarvajal! 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
✓	18651	major	ice: assert in glue.d:777 when building these three trivial files

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

[Geod24]

The test in bugzilla does use phobos, but it doesn't so much but an import.
Personally I'd prefer to have a test case for this considering it's likely that such a regression creeps back, but I'll let others chime in.

[BorisCarvajal]

@Geod24 , indeed the test just imports two packages but the feature of concern (-deps) runs semantic3 on most if not all the Phobos modules, if that's not a problem I'll add it.

[Geod24]

@BorisCarvajal : I guess the ideal would be to come up with a test that depends on deferred sema3 being run but not Phobos. How time consuming would that be ?

[BorisCarvajal]

I don't know. But I've uploaded the original test case, it's better than nothing.

[MoonlightSentinel]

Please let me have a look before merging, this test shouldn't pass ATM ....

EDIT:
TEST_OUTPUT isn't enforced because this PR targets stable and the d_do_test extensions weren't merged yet. So we should probably have a master -> stable merge before proceeding with this PR.

[BorisCarvajal]

@MoonlightSentinel, I'll use -deps=file to make the compilation silent, the output is long and can vary.

[Geod24]

So we should probably have a master -> stable merge before proceeding with this PR.

stable => master is a free for all, but master => stable happens at specific time.
I'm a bit fuzzy on the details now, but usually only Martin does it.

[MoonlightSentinel]

Dustmite found an interesting reduction:

// a.d:
import stdx.datetime;

// b.d:
import c;

// c.d:
import stdx.algorithm;

const var = sort(string[].init);

// stdx/algorithm.d:
struct SortedRange(Range)
{
    Range _input;
}

auto sort(Range)(Range)
{
    return SortedRange!Range();
}

auto uniq(Range)(Range)
{
    return SortedRange!Range();
}

// stdx/datetime.d:
void parseTZConversions()
{
    import algorithm; // Does not exist.

    string[] value;
    value.sort.uniq;
}

dmd -deps a.d b.d

This PR fixes the segfault but does not recognize the invalid import in parseTZConversions.

[BorisCarvajal]

Interesting reduction by the mighty Dustmite.
But doesn't look like an invalid import.

[MoonlightSentinel]

parseTZConversions imports algorithm, not stdx.algorithm from stdx/algorithm.d.

[BorisCarvajal]

@MoonlightSentinel , DMD loads the import just fine.
stdx/algorithm.d has no explicit module declaration, hence datetime.d from the same directory can import it with its plain name algorithm.
Whether you should use the prefix stdx. or not seems irrelevant to this issue.

Thanks anyway, I'll update the test to the reduced code.

(Note: This is different from your recent issue report, in which case the import file doesn't exist at all)

[MoonlightSentinel]

Whether you should use the prefix stdx. or not seems irrelevant to this issue.

Agreed, itis just odd and feels like it should not work.

(Note: This is different from your recent issue report, in which case the import file doesn't exist at all)

Yeah, that was the first failed reduction.

[MoonlightSentinel]

Please use explicit imports instead of an additional import path (it avoids confusion about the location of these files).

You could also create an additional directory (imports/test18651) for all addtional files used by this test.

[MoonlightSentinel]

LGTM

[Geod24]

Amazing work, thanks