Fix Issue 17548 - [REG2.072.0] Forward reference error with scope function parameters

[JohanEngelen]

This PR

• reverts Make Module.semantic override the base class's virtual function #6001 and [Trivial] Fix C++ header after PR #6001. #6038 , because Make Module.semantic override the base class's virtual function #6001 caused a regression : https://issues.dlang.org/show_bug.cgi?id=17548 .
• adds the testcase from dlang issue 17548
• renames Module.semantic*, such that it is clear that it is not accidental that Dsymbol.semantic* are not overridden (this was the original intent of Make Module.semantic override the base class's virtual function #6001).

[dlang-bot]

Thanks for your pull request, @JohanEngelen! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Fix	Bugzilla	Description
✗	17548	[REG2.072.0] Forward reference error with scope function parameters

[WalterBright]

renames Module.semantic*, such that it is clear that it is not accidental that Dsymbol.semantic* are not overridden (this was the original intent of #6001).

What's the problem with overriding it? It looks strange to have two different semantic2() functions.

[JohanEngelen]

What's the problem with overriding it? It looks strange to have two different semantic2() functions.

Indeed. Before #6001, there were two different (but identically named) semantic* functions for a Module. #6001 tried to resolve it by making the Module.semantic* override Dsymbol's. But that apparently broke things. So I reverted that. But now with the revert, a different test (compilable/ice10598.d) breaks...
The frontend code is so opaque that I cannot make any judgement on what the correct code should be.

[Syniurge]

Thanks for looking into it Johan. This fixes my particular case, but Module.doSemanticPass() can still be hit at the wrong time with the same outcome if we add an import in S2:

struct S2 {
    // void bar(int arg = .fwdref1.cnst) {}
    S1 s;
    import fwdref2;
}

Importing itself is contrived, but it could be importing a fwdref3 module that imports fwdref2 and it would be the same.

[JohanEngelen]

Note: I just wanted to unbreak things that got broken by my PR. Now it looks like there is a bigger problem lurking, which does need a proper fix, rather than a revert of #6001. :(
Because I am currently not actively working on figuring out what's wrong (and probably won't be any time soon), shall I close this PR?

[Syniurge]

Still helped :)

Now it looks like there is a bigger problem lurking, which does need a proper fix

Always that feeling with that kind of issue.

I pushed a tentative one line fix but fearing it may miss the bigger picture I wanted to see other analysis/suggestions.

[MartinNowak]

Any idea what caused the regression? Looks a bit like Module.semantic(Scope *) now gets used as Dsymbol.semantic(sc) somewhere, because of the apparent reentrant semantic calls. Before #6001 this just resolved to Package.semantic(Scope*) which doesn't do much.
We might want to add an early return to reentrant Module.semantic* calls, e.g. here.
The Module.semantic* methods already prevent reentrant runs.